366588 - Thread default context is used instead of context passed in call method

Status: RESOLVED INVALID

(Rhino Graveyard :: Core, defect)

Description

[Vlad Grynchyshyn]

User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.0; FunWebProducts; .NET CLR 1.0.3705; .NET CLR 1.1.4322; InfoPath.1)
Build Identifier: Rhino 1_6R5

Call method fails to pass context through all the execution process - thread default context is used in many cases.

There is a comment in the Context.java file, line 308... (comment for enter() method):
     * Instead of using <tt>enter()</tt>, <tt>exit()</tt> pair consider using
     * {@link #call(ContextAction)} which guarantees proper
     * association of Context instances with the current thread and is faster.
     * With this method the above example becomes:
... skip

Above is not true for multi-threading environment - there is no guarantee. In  web applications, when context and script objects are cached across web requests (and so may work in different threads), scripts often fail. A simple example of crash stack trace follows:

	at org.mozilla.javascript.Context.getContext(Context.java:2195)
	at org.mozilla.javascript.ScriptableObject.getDefaultValue(ScriptableObject.java:579)
	at org.mozilla.javascript.ScriptRuntime.toString(ScriptRuntime.java:675)
	at org.mozilla.javascript.NativeString.execIdCall(NativeString.java:222)
	at org.mozilla.javascript.IdFunctionObject.call(IdFunctionObject.java:127)
	at org.mozilla.javascript.optimizer.OptRuntime.call2(OptRuntime.java:76)
	at org.mozilla.javascript.gen.c42._c0(<inline>:1)
	at org.mozilla.javascript.gen.c42.call(<inline>)
	at org.mozilla.javascript.ContextFactory.doTopCall(ContextFactory.java:340)
	at org.mozilla.javascript.ScriptRuntime.doTopCall(ScriptRuntime.java:2758)
	at org.mozilla.javascript.gen.c42.call(<inline>)
	at org.mozilla.javascript.gen.c42.exec(<inline>)

As you can see from above trace, ScriptRuntime.toString call is NOT called with the context provided initially. When toString is called for expression or any other object that requires further work and context, and thread is different than the one in which initial context was created, script running fails.

I think the comment to enter() method should be updated to mention that it does not guarantees correct context use across all the script execution process if execution goes in different thread(s) than the thread which original context was created in. For such case, in each method that potentially can run scripts need to use Context.enter() and Context.exit(), otherwise crash happens often with stack trace like above.

Also, please note that ScriptRuntime.toString() runs using default thread context rather than the one provided with the call() method (I did code analyzing a bit).

So, alternatively, fix such method calls as in above crash stack log to actually use context provided in initial call instead of thread default context.


Reproducible: Always

Steps to Reproduce:
1. Take WebViewerExample from BIRT and modify it to cache RunTask object (report rendering task that also runs Rhino scripts and caches them as pre-compiled). It is prefered to deploy it on web server that uses multiple threads and often uses different thread for each different request (I used JBoss)
2. Create any BIRT report with chart that contains complex expressions like (... + ...).toString(), row["..."].toString() etc. Assure all works ok in report preview.
3. Run web requests that use cached RunTask object
Actual Results:  
Scripts fail to run. Example crash stack trace:

	at org.mozilla.javascript.Context.getContext(Context.java:2195)
	at org.mozilla.javascript.ScriptableObject.getDefaultValue(ScriptableObject.java:579)
	at org.mozilla.javascript.ScriptRuntime.toString(ScriptRuntime.java:675)
	at org.mozilla.javascript.NativeString.execIdCall(NativeString.java:222)
	at org.mozilla.javascript.IdFunctionObject.call(IdFunctionObject.java:127)
	at org.mozilla.javascript.optimizer.OptRuntime.call2(OptRuntime.java:76)
	at org.mozilla.javascript.gen.c42._c0(<inline>:1)
	at org.mozilla.javascript.gen.c42.call(<inline>)
	at org.mozilla.javascript.ContextFactory.doTopCall(ContextFactory.java:340)
	at org.mozilla.javascript.ScriptRuntime.doTopCall(ScriptRuntime.java:2758)
	at org.mozilla.javascript.gen.c42.call(<inline>)
	at org.mozilla.javascript.gen.c42.exec(<inline>)


Expected Results:  
Original execution context is passed along through all the process of script execution. Alternatively, fix warning/comment about the use of call method - that it needs enter()/exit() calls anyway in multi-threaded environment (no guarantee of contaxt pass through all the process), though it looks like a bit of mess for web developers anyway - better fix it instead of commenting out. (I spent few days researching this.)

Workaround for Rhino version 1_6R5: use enter()/exit() calls in each separate method that may be called in thread another than thread which original script context is created in.

Comment 1

[Attila Szegedi]

Your expectations are unfortunately wrong. ContextFactory.call(ContextAction) and Context.call(ContextAction) will associate a Context with the current thread. However, a Context instance is bound to a single thread and doesn't propagate across threads.

It is an error to cache Context instances across threads -- if your script execution spans multiple HTTP requests, you need to enter and exit a context in each of them. This can be conveniently done with a Filter where you'd do something like:

public void doFilter(final ServletRequest req, ServletResponse resp, 
    FilterChain chain) throws IOException, ServletException
{
    contextFactory.call(new ContextAction()
    {
        public void run(Context cx)
        {
            filter.doChain(req, resp);
        }
    }
}

(note I encourage you to instantiate a ContextFactory instead of relying on the singleton used by Context.enter() and Context.exit()

or

public void doFilter(final ServletRequest req, ServletResponse resp, 
    FilterChain chain) throws IOException, ServletException
{
    contextFactory.enter();
    try
    {
        filter.doChain(req, resp);
    }
    finally
    {
        contextFactory.exit();
    }
}

Comment 2

[Vlad Grynchyshyn]

Thanks for answer.

The problem is that I use another product that in turn uses Rhino, and need to have objects from that product cached. These, in turn, cause caching of Rhino context. So it is not my intention to cache Rhino context across multiple threads, it is how that product built. 

I cannot change that product and so I thought you would be interested in improving Rhino for working across multiple threads. I suppose this is not the last case when Rhino would be used across threads not intentionally. And also web applications usually require a lot of caching across multiple threads.

As you wish though.

Thanks for code samples!

Comment 3

[Attila Szegedi]

(In reply to comment #2)
> 
> I cannot change that product and so I thought you would be interested in
> improving Rhino for working across multiple threads. I suppose this is not the
> last case when Rhino would be used across threads not intentionally. And also
> web applications usually require a lot of caching across multiple threads.

Well, Context objects are single-threaded by design. If the product in question caches Context objects across threads, then they're using Rhino's API in a violating manner, and you should really file a bug with them (feel free to point them to this discussion). A Context object must never be cached, instead it should always be obtained using Context.getCurrentContext().