-
-
Notifications
You must be signed in to change notification settings - Fork 435
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Insert context keys before compiling UI scripts #4372
Conversation
Signed-off-by: Jimmy Tanagra <[email protected]>
@@ -211,7 +212,7 @@ protected void resetExecutionContext(ScriptEngine engine, Map<String, ?> context | |||
try { | |||
if (compiledScript.isPresent()) { | |||
logger.debug("Executing pre-compiled script of rule with UID '{}'", ruleUID); | |||
return compiledScript.get().eval(engine.getContext()); | |||
return compiledScript.get().eval(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had this line that way because what if eval is called with a different ScriptEngine?
Of course this should not happen at all because where should the child classes of AbstractScriptModuleHandler get a different ScriptEngine from, but given the method signature it would be possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI just thinking about that, I’m fine with that change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM and thanks for the fix — when creating the PR that caused this, I already expected such regressions and therefore pinged the JRuby maintainers, but it seems you didn’t notice that.
I did notice it and implemented openhab/openhab-addons#17140 but I've only just noticed this error today. It's a bit strange, because when I did the addons PR 17140, I would've tested it, but perhaps the issue is triggered with a more complex script than what I used to test for that PR. |
I'm sorry, but upon further testing, this actually didn't fix the problem with JRuby at all. It only appeared to work, because when I tested it, the script was eval'ed without compiling it. Should we revert this now or later when a solution is found? In the mean time, the other alternative is to not implement Compilable for JRuby addon for now until this bug is fixed in JRuby itself. |
Let's revert it now... |
This reverts commit 6e6f000.
This reverts commit 6e6f000. Signed-off-by: Jimmy Tanagra <[email protected]>
This reverts commit 6e6f000. Signed-off-by: Jimmy Tanagra <[email protected]>
Regression from #4289
This is needed, similar to #3464, except in this case, JRuby would throw an
ArrayIndexOutOfBounds
exception without this fix. This is possibly a bug in JRuby script engine, but this PR avoids encountering that problem.The error was:
Note that line 215 is changed not because it fixes this problem, but because it's unnecessary to pass the engine context.
See https://docs.oracle.com/en/java/javase/17/docs/api/java.scripting/javax/script/CompiledScript.html#eval()