Looks like a reasonable compromise! +1
-Sundar On 6/15/2016 8:50 PM, Hannes Wallnöfer wrote: > 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 >>>>>
