JSR 223 different behaviour between compiled execution and evaluated #5876
Comments
|
Huh yeah that doesn't seem right. I wonder if it's something to do with how it's compiling the code? Like perhaps it's compiling |
|
@headius I give you just little more context, I'm using this code to run as a Script element in Log4j2 and I can't change the way the variables are declared because it's done by Log4J itself. |
|
given how log4j2 uses script engine I can see a clear difference (and some problematic expectations) when using the JRuby engine vs. using the Javascript engine: based on the "THREADING" parameter reported by the script engine factory impl it will either isolate the script into a thread-local or ( ScriptEngineManager manager = new ScriptEngineManager();
ScriptEngine engine = manager.getEngineByName("jruby");
System.out.println("JRuby THREADING: " + engine.getFactory().getParameter("THREADING"));
// JRuby THREADING: THREAD-ISOLATED ScriptEngineManager manager = new ScriptEngineManager();
ScriptEngine engine = manager.getEngineByName("javascript");
System.out.println("JS THREADING: " + engine.getFactory().getParameter("THREADING"));
// JS THREADING: nullJRuby by default uses a single (shared) runtime and thus this can be fixed by using a different local context behavior (leading to multiple runtimes) : @Test
public void testEvaluateCompiledThreadedConcurrent() throws Exception {
System.setProperty("org.jruby.embed.localcontext.scope", "concurrent");
//System.setProperty("org.jruby.embed.localcontext.scope", "threadsafe");
ScriptEngineManager manager = new ScriptEngineManager();
ScriptEngine engine = manager.getEngineByName("jruby");
CompiledScript compiled = ((Compilable) engine).compile("puts \"concurent> \" + java.lang.Thread.currentThread.getName() + ' ' + message");
for (int i=0; i<3; i++) {
final int tid = i;
new Thread(() -> {
Thread.currentThread().setName("concurrent thread-" + tid);
Bindings bindings = new SimpleBindings();
bindings.put("message", "jruby threaded " + tid);
try {
//compiled.eval(bindings); // DOES NOT WORK !
engine.eval("puts \"Y>\" + message", bindings);
} catch(ScriptException ex) {
System.out.println("concurrent thread failed: " + ex + " " + tid);
ex.printStackTrace();
}
}).start();
}
Thread.currentThread().join(2000);
}... and also reproduces the original issue of different behavior with evaluated vs compiled execution. in conclusion, the confusion comes from the fact that a single runtime is used by default with JSR223, thus a (global) binding for one engine will have potential consequences for others. but also due the fact of setting |
|
managed to reproduce as expected (actually confused why I had the simple compiled test passing previously). the compiled script eval issue boils down to the JSR223 embed case creating a root+eval scope, in case of the "pre-compilation" (AOT parsing and keeping the AST node really) the local variable names are not known and thus the eval scope ends up with no variable names compared to |
|
There's an opportunity here to make adjustments in this behavior. Perhaps it would make more sense to do what other 223 engines do and have each engine get its own runtime? |
when pre-parsing (compiling) scripts the parser will assume potential loval variables to be method calls, since the eval scope won't provide any local variable names (compared to directly eval-ing with bindings passed in). see jrubyGH-5876
when pre-parsing (compiling) scripts the parser will assume potential loval variables to be method calls, since the eval scope won't provide any local variable names (compared to directly eval-ing with bindings passed in). see jrubyGH-5876
Yes that would make sense as a default, to me at least. Ideally also not causing the global runtime to be set due javax.script. Back to the issue here, I've added tests (in #6457) to avoid further confusion (there was a Also, realized we have some options on "fixing" the compiled script not having local-variable hints.
Down side here is - we do not parse and thus at least validate syntax on Personally am not strong for the "delayed compilation" but it seems like a reasonable fix and actually what users might expect from a compiled script provided with local-variables despite the introduced negatives. Opinions? |
|
It is unfortunate that you can bind new variables after forcing a script to compile, but I worry about deferring the compilation until first use. The delayed syntax checking you mention is one obvious issue, but also the compilation step is typically used by scripters to ensure they have something that is "ready-to-execute". Introducing a potential error and a parse/compile delay into the first invocation would not meet that expectation. It might be acceptable to allow a recompile when evaluating a script against variable bindings that contain new variables, since that will potentially change the behavior of the code. If no late binding is provided, or the late binding contains no new variable names, we can just go with the initially-compiled code. I am curious how other 223 languages handle this, since I would expect several of them to require variables to be bound before execution. Perhaps Python or Groovy have a similar issue here? |
Hmm, this sounds a bit against having smt to execute but I get the slight difference - we would at least validate syntax.
Believe Groovy does only allow method calls wout parenthesis when there isn't ambiguity, but I am certainly no expert. |
Now that I think about it I remember the only cases where you can make a call without parens is if you are accessing a property (a field or a dynamically dispatched property lookup) which would have to be qualified with a This may have changed over the years but it is likely that no other commonly-used language has the binding ambiguity that we do. This is a bit of a conundrum, and lazily compiling might be the only option. I wish I had noticed this flaw in the design of JSR-223 back in the day. Any thoughts on this @enebo? |
|
What about automatically (when a new system property is set) prepend variables from bindings with $ or @? It's a workaround, but would at least allow usage in projects that allow any JSR223 language to be used, but without any specific Ruby support. The script would have to know this is happening, and be written to expect it. |
|
@ccutrer Perhaps we can flesh out this idea a bit more? If you evaluate some ruby code at that point you enumerate bindings and then only convert those? More specifically anything in the file or just only anything at the top-level script scope? -Djruby.js223.special.bindings.somename=true foo = calculate(bar)This would read what was in bar and also write back to foo. Would this work? proc {
foo = calculate(bar)
}.callThis second question is mostly a matter of seeing how much we should restrict this. I would think we would not read bar and write foo in this second example and limit the specialness of this to just the Ruby top-level script scope. I definitely think enforcing this outside the main file and non-top-level scopes would cause strange side-effects (like load a gem which assigns 'foo'). |
|
I was thinking the opposite direction: -Djruby.js223.bindings.localinstancevars=true ...
final ScriptEngine engine = compiledScript.getEngine();
final ScriptContext ctx = engine.getContext();
ctx.setAttribute("input", source_value, ScriptContext.ENGINE_SCOPE);
Object result = compiledScript.eval();Ruby: directions = %w[N NE E SE S SW W NW N].freeze
# note that the script uses `@input`, even though the Java
# supplied `input`
if @input == "NULL" || @input == "UNDEF"
"-"
else
cardinal = directions[(@input.to_f / 45).round]
"#{cardinal} (#{@input.to_i}°)"
endIt's up to the user to know to adjust their scripts when setting that property. This avoids having to parse and adjust the actual script, which as you pointed out could be problematic. |
|
@ccutrer Can you open up a new issue on this? I think this should be pretty doable. It would definitely give more flexibility to just using jsr223 instead of forcing people to our internal redbridge apis. Others can weigh in on whether they can think of any problems in the new issue. |
|
Oooh, I found a workaround. If you use |
Environment
Provide at least:
jruby -v) and command line (flags, JRUBY_OPTS, etc)jruby-9.2.8.0
uname -a)Linux 5.0.0-27-generic JRUBY-4445 #28-Ubuntu SMP Tue Aug 20 19:53:07 UTC 2019 x86_64 x86_64
x86_64 GNU/Linux
JVM: OpenJDK 12.0.2
Other relevant info you may wish to add:
Expected Behavior
Both
evalinvoked onScriptEngineand onCompiledScriptprints same value of local variableDescribe your expectation of how JRuby should behave, perhaps by showing how CRuby/MRI behaves.
Both ways of execution should output the same string
Provide an executable Ruby script or a link to an example repository.
NB put the tests in two separate unit test or run one at a time
Actual Behavior
Describe or show the actual behavior.
the test
testEvaluateCompiledfails with error while the other print correctly the string>>>>>>global variableProvide text or screen capture showing the behavior.
The text was updated successfully, but these errors were encountered: