I’ve uploaded a new webrev that reintroduces the Global.initscontext field instead of reusing the current context thread local so it will behave the same from all threads. Please review.
http://cr.openjdk.java.net/~hannesw/8150219/webrev.01/ Thanks, Hannes > Am 14.06.2016 um 10:22 schrieb Hannes Wallnöfer > <hannes.wallnoe...@oracle.com>: > > Thanks for the comments, Sundar. > > I agree that using the existing ScriptContext thread local is a bit of a > hack. I chose this path because it allowed me to keep the changes minimal and > fix the problem for the most predominant and reasonable use case. > > What I absolutely do not agree with is to have a method to evaluate a script > with a non-default ScriptContext and then ignore that ScriptContext on > invocation of a function obtained from that evaluation. This breaks about > every assumption a somewhat experienced JavaScript developer would have, and > it also is a regression against JDK up to u71. > > Note that my change does not go back to the behavior of pre-u72, so engine’s > default ScriptContext is still used for all globals created the „normal“ way > (e.g. using NashornScriptEngine.createBindings method, or the default global > created in NashornScriptEngine constructor). The only case we set the engine > this way is for evals with a non-default ScriptEngine which does not already > have a Nashorn global. This is of course also reflected by the tests you > added for JDK-8138616 still passing. > > I hope we can find some common ground here. > > Hannes > > >> Am 14.06.2016 um 04:32 schrieb Sundararajan Athijegannathan >> <sundararajan.athijegannat...@oracle.com>: >> >> Sorry, I'm not sure I'm comfortable with this fix :( >> >> Global.setScriptContext sets the ScriptContext for the *current thread* >> [because that Global class methods set thread local]. This means that >> this ReferenceError goes away only for the thread in a new Global is >> created and associated with that ScriptContext. For other threads, >> you'll continue to get ReferenceError. >> >> The root of the issue is the Invocable's methods are expected to use the >> default ScriptContext. That means that if you eval code or put globals >> in another ScriptContext, you can't do Invocable calls on it! That is >> the limitation of javax.script. While eval's have ScriptContext, >> Bindings accepting variants, there are no similar methods exists for >> Invocable and Compilable. These two use script engine's default >> ScriptContext! >> >> BTW, there is clear Nashorn alternative - which is to use >> jdk.scripting.api.scripting.ScriptObjectMirror's call or eval method. >> These take care of using the right global in which the function was >> defined - lexical scoping is respected as expected! >> >> Besides the current "fix" works only the particular thread which feels >> wrong as well. >> >> -Sundar >> >> On 6/14/2016 1:04 AM, Attila Szegedi wrote: >>> +1 >>> >>>> On 13 Jun 2016, at 18:56, Hannes Wallnöfer <hannes.wallnoe...@oracle.com> >>>> wrote: >>>> >>>> Please review 8150219: ReferenceError in 1.8.0_72: >>>> >>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8150219 >>>> Webrev: http://cr.openjdk.java.net/~hannesw/8150219/webrev.00/ >>>> >>>> This is a bit of a compromise that partially restores the behavior we had >>>> before JDK-8138616 by associating a ScriptContext with a Nashorn Global, >>>> but only in the very specific case where a script is evaluated with a >>>> non-default ScriptContext that does not have a Nashorn Global yet, and the >>>> Global is created for that specific ScriptContext. Also, we use the >>>> existing setScriptContext method and ThreadLocal field in Global instead >>>> of introducing an extra field as we had it before. >>>> >>>> The purpose is to make functions obtained from such globals use the >>>> ScriptContext they were evaluated with. >>>> >>>> Thanks, >>>> Hannes >>>> >> >
