Issue1426

In Subversion revision 6633.

The issue is that sometimes a ScriptEngine can be re-used, but called
with a different context. Why do this? Lots of reasons! In applications
that use scripts a lot, it's a waste of resources to recreate the
ScriptEngine all over again. Instead, it is reused, while only a
different context is used. This is how I've implemented in the
Scripturian project, and it works fine for every other JSR-223-compliant
engine supported (include Jython 2.2 with its JSR-223 support library).

This was quite easy to fix. I'm unsure how to submit a patch, but I hope
this will suffice! Here is PyScriptEngine with my addition:

    private Object eval(PyCode code, ScriptContext context) throws
ScriptException {
        try {
            interp.setIn(context.getReader());
            interp.setOut(context.getWriter());
            interp.setErr(context.getErrorWriter());
            List<Integer> scopes = context.getScopes();
            for (int scope : scopes) {
            	this.context.setBindings( context.getBindings( scope ),
scope );
            }
            return interp.eval(code).__tojava__(Object.class);
        } catch (PyException pye) {
            throw scriptException(pye);
        }
    }

On second look, three more fixes are needed to take account of the
context. First, in these two methods:

    private PyCode compileScript(String script, ScriptContext context)
throws ScriptException {
        try {
            String filename = (String)
context.getAttribute(ScriptEngine.FILENAME);
            List<Integer> scopes = context.getScopes();
            for (int scope : scopes) {
            	this.context.setBindings(context.getBindings(scope), scope);
            }
            if (filename == null) {
                return interp.compile(script);
            } else {
                return interp.compile(script, filename);
            }
        } catch (PyException pye) {
            throw scriptException(pye);
        }
    }

    private PyCode compileScript(Reader reader, ScriptContext context)
throws ScriptException {
        try {
            String filename = (String)
context.getAttribute(ScriptEngine.FILENAME);
            List<Integer> scopes = context.getScopes();
            for (int scope : scopes) {
            	this.context.setBindings(context.getBindings(scope), scope);
            }
            if (filename == null) {
                return interp.compile(reader);
            } else {
                return interp.compile(reader, filename);
            }
        } catch (PyException pye) {
            throw scriptException(pye);
        }
    }

Two problems with my patch: it is not thread-safe. Also, it alters the
engine's context and does not return it to its original value. I will
try to think of a way to make it better.

Finally, this method must be overridden to allow the engine context to
change:

    public void setContext(ScriptContext context) {
    	super.setContext(context);
    	interp.setLocals(new PyScriptEngineScope(this, context));
    }

...which just makes eval even more expensive.  I'll take a look at this, 
thanks.

As it stands, the code does very little, but adding synchronization
would definitely slow things down. A better plan for thread safety is in
order!

Note that PyScriptEngineScope also synchronizes on the context, which
can be both bad for performance, and can also cause problems if
applications do their own work with the context.

Feel free to include me on your thoughts in this. I have a lot of
experience with JSR-223, and am very eager to get Jython 2.5 running
well in Scripturian!

I did that synchronization because other JSR 223 implementations did.  If you 
could lay out a general idea of how it should work (or if you just want to fix 
it), let me know - patches gratefully accepted.  It's sometimes a bit hard to 
figure out from the JSR spec how these things actually get used.

I'm traveling this weekend but should be able to look at the ScriptContext 
issues on Monday.

Thanks!

I will give this some more thought. You're right about everything:
JSR-223 is a very loose spec (too loose). I've had serious problems
getting everything to work with Scripturian, and have even given up on a
few engines for some features. And, #2, even when they do work, they
solve thread-safety with simple synchronization.

Let's try together to make Jython's JSR-223 implementation better than
others. :)

Happy travels!

This came up again on the mailing list:

http://article.gmane.org/gmane.comp.lang.jython.user/8466

It also brings up the various threading models described in JSR 223.  Should 
I implement a thread-isolated version or is it acceptable to create an engine 
per thread?  Python just has locals and globals, no "engine scope" and 
"global scope" and "thread scope" which JSR 223 wants...

The JSR is pretty terse when it comes to defining threading, but there
is a de-facto requirement which is obvious if one imagines use-cases
beyond the trivial. It must support multithreaded access to the
ScriptContext.

Here's a fact: without my patch, Jython breaks where are other JSR-223
implementations do not. Prudence now ships with my patched version:

http://threecrickets.com/prudence/

I didn't have time to work on this beyond my quick band aid. Nicholas,
what do you say? Do you want to take some time to work on this, or
should I try to slog through?

I tried, but got a bit frustrated as Jython SVN changed before my eyes
and broke my code... It seems this kind of work needs to be better
coordinated with the team. Also, there must be a multithreaded unit test
for this model, to make sure it doesn't break!

Absolutely, I agree that the behavior with respect to bindings is a bug and 
correctness is more important than speed; let's just get there first.  My question 
was only whether it'd be useful to develop an engine with a different JSR 223 
threading model.

If you'd like to write a test case and propose a patch I'd be happy to review it.  If 
you have any questions, please ask on the mailing list or IRC.  I can spend some time 
on this after December 19.

Sorry about the Subversion changes.  I use git-svn to work on Jython so I can merge 
upstream changes easily; you could do the same.  The rate of change in Jython is 
actually relatively low so it's unlikely there will be another conflict in this area, 
however.

Side note: according to JSR 223 specs an engine is supposed to
automatically provide the script context as "context" binding but jython
doe snot seems to do it (since I'm reusing the same ScriptContext object
i was expecting to workaround this issue by doing context.mybinding)

I don't understand what you mean.  Could you cite the specific section of 
the JSR 223 spec to which you're referring and/or give an example in code 
of what you are expecting to happen?

I mean when you are in your script, the variable "context" is supposed
to be the current ScriptContext object.

Here is the exact: "In all cases, the ScriptContext used during a script
execution must be a value in the Engine Scope of the ScriptEngine whose
key is the String “context”.". I found it in the PDF specs at
https://cds.sun.com/is-bin/INTERSHOP.enfinity/WFS/CDS-CDS_JCP-Site/en_US/-/USD/ViewFilteredProducts-SimpleBundleDownload
and in all others JSR 223 implementations i seen

Here's a patch! I don't seem to have permissions to attach files to this
issue, so I am pasting it here.

The patch is quite invasive, by necessity. I had to change some things
in the way PythonInterpreter works, so that it could work with
thread-bound state.

It already had support for thread-bound PySystemState (with a small bug
that I fixed, for setIn, setOut and setErr). But it didn't have support
for thread-bound locals. To do this, I added support for threadLocals to
ThreadState, and then enabled an optional "threadLocals" mode for
PythonInterpreter. The result is that, in "threadLocals" mode, multiple
threads can use the same PythonInterpreter.

Now, for JSR-223, the issue is that the Python locals come from outside,
via the ScriptContext. We have no control for its thread-safety. Indeed,
the JSR-223 is silent on these matters. So, I removed all
synchronization from the PyScriptEngineScope code. If callers are using
us multi-threaded, it's up to them to protect things. (For example, they
can provide us with a thread-safe instance of ScriptContext.) In any
case, it was wrong to synchronize on the ScriptContext instance, since
it is external to us.

I hope this all makes sense.

-Tal


### Eclipse Workspace Patch 1.0
#P jython-trunk
Index: src/org/python/util/PythonInterpreter.java
===================================================================
--- src/org/python/util/PythonInterpreter.java	(revision 6958)
+++ src/org/python/util/PythonInterpreter.java	(working copy)
@@ -32,6 +32,8 @@
     protected PySystemState systemState;
 
     PyObject locals;
+    
+    protected boolean threadLocals = false;
 
     protected CompilerFlags cflags = new CompilerFlags();
 
@@ -83,8 +85,13 @@
         systemState.modules.__setitem__("__main__", module);
         locals = dict;
     }
+    
+    public void setThreadLocals(boolean threadLocals) {
+    	this.threadLocals = threadLocals;
+    }
 
     protected void setState() {
+    	// The thread-bound system state may not have been set yet for
this thread
         Py.setSystemState(systemState);
     }
 
@@ -95,7 +102,7 @@
      *            Python file-like object to use as input stream
      */
     public void setIn(PyObject inStream) {
-        systemState.stdin = inStream;
+    	Py.getSystemState().stdin = inStream;
     }
 
     public void setIn(java.io.Reader inStream) {
@@ -119,7 +126,7 @@
      *            Python file-like object to use as output stream
      */
     public void setOut(PyObject outStream) {
-        systemState.stdout = outStream;
+    	Py.getSystemState().stdout = outStream;
     }
 
     public void setOut(java.io.Writer outStream) {
@@ -137,7 +144,7 @@
     }
 
     public void setErr(PyObject outStream) {
-        systemState.stderr = outStream;
+    	Py.getSystemState().stderr = outStream;
     }
 
     public void setErr(java.io.Writer outStream) {
@@ -153,7 +160,7 @@
      */
     public PyObject eval(String s) {
         setState();
-        return __builtin__.eval(new PyString(s), locals);
+        return __builtin__.eval(new PyString(s), getLocals());
     }
 
     /**
@@ -161,7 +168,7 @@
      */
     public PyObject eval(PyObject code) {
         setState();
-        return __builtin__.eval(code, locals, locals);
+        return __builtin__.eval(code, getLocals(), getLocals());
     }
 
     /**
@@ -169,7 +176,7 @@
      */
     public void exec(String s) {
         setState();
-        Py.exec(Py.compile_flags(s, "<string>", CompileMode.exec,
cflags), locals, locals);
+        Py.exec(Py.compile_flags(s, "<string>", CompileMode.exec,
cflags), getLocals(), getLocals());
         Py.flushLine();
     }
 
@@ -178,7 +185,7 @@
      */
     public void exec(PyObject code) {
         setState();
-        Py.exec(code, locals, locals);
+        Py.exec(code, getLocals(), getLocals());
         Py.flushLine();
     }
 
@@ -187,7 +194,7 @@
      */
     public void execfile(String filename) {
         setState();
-        __builtin__.execfile_flags(filename, locals, locals, cflags);
+        __builtin__.execfile_flags(filename, getLocals(), getLocals(),
cflags);
         Py.flushLine();
     }
 
@@ -197,7 +204,7 @@
 
     public void execfile(java.io.InputStream s, String name) {
         setState();
-        Py.runCode(Py.compile_flags(s, name, CompileMode.exec, cflags),
locals, locals);
+        Py.runCode(Py.compile_flags(s, name, CompileMode.exec, cflags),
getLocals(), getLocals());
         Py.flushLine();
     }
 
@@ -225,11 +232,16 @@
 
 
     public PyObject getLocals() {
-        return locals;
+		return threadLocals ? Py.getThreadState().threadLocals : locals;
     }
 
     public void setLocals(PyObject d) {
-        locals = d;
+    	if(threadLocals) {
+    		Py.getThreadState().threadLocals = d;
+    	}
+    	else {
+    		locals = d;
+    	}
     }
 
     /**
@@ -242,7 +254,7 @@
      *            appropriate Python object.
      */
     public void set(String name, Object value) {
-        locals.__setitem__(name.intern(), Py.java2py(value));
+    	getLocals().__setitem__(name.intern(), Py.java2py(value));
     }
 
     /**
@@ -254,7 +266,7 @@
      *            the value to set the variable to
      */
     public void set(String name, PyObject value) {
-        locals.__setitem__(name.intern(), value);
+    	getLocals().__setitem__(name.intern(), value);
     }
 
     /**
@@ -265,7 +277,7 @@
      * @return the value of the variable, or null if that name isn't
assigned
      */
     public PyObject get(String name) {
-        return locals.__finditem__(name.intern());
+        return getLocals().__finditem__(name.intern());
     }
 
     /**
@@ -280,7 +292,7 @@
      * @return the value of the variable as the given class, or null if
that name isn't assigned
      */
     public <T> T get(String name, Class<T> javaclass) {
-        PyObject val = locals.__finditem__(name.intern());
+        PyObject val = getLocals().__finditem__(name.intern());
         if (val == null) {
             return null;
         }
Index: src/org/python/core/ThreadState.java
===================================================================
--- src/org/python/core/ThreadState.java	(revision 6958)
+++ src/org/python/core/ThreadState.java	(working copy)
@@ -24,6 +24,8 @@
     public TraceFunction tracefunc;
 
     public TraceFunction profilefunc;
+    
+    public PyObject threadLocals;
 
     private LinkedList<PyObject> initializingProxies;
 
Index: src/org/python/jsr223/PyScriptEngineScope.java
===================================================================
--- src/org/python/jsr223/PyScriptEngineScope.java	(revision 6958)
+++ src/org/python/jsr223/PyScriptEngineScope.java	(working copy)
@@ -38,15 +38,13 @@
     @ExposedMethod
     public PyObject scope_keys() {
         PyList members = new PyList();
-        synchronized (context) {
-            List<Integer> scopes = context.getScopes();
-            for (int scope : scopes) {
-                Bindings bindings = context.getBindings(scope);
-                if (bindings == null)
-                    continue;
-                for (String key : bindings.keySet())
-                    members.append(new PyString(key));
-            }
+        List<Integer> scopes = context.getScopes();
+        for (int scope : scopes) {
+            Bindings bindings = context.getBindings(scope);
+            if (bindings == null)
+                continue;
+            for (String key : bindings.keySet())
+                members.append(new PyString(key));
         }
         members.sort();
         return members;
@@ -63,12 +61,10 @@
     }
 
     public PyObject __finditem__(String key) {
-        synchronized (context) {
-            int scope = context.getAttributesScope(key);
-            if (scope == -1)
-                return null;
-            return Py.java2py(context.getAttribute(key, scope));
-        }
+        int scope = context.getAttributesScope(key);
+        if (scope == -1)
+            return null;
+        return Py.java2py(context.getAttribute(key, scope));
     }
 
     @ExposedMethod
@@ -77,12 +73,10 @@
     }
 
     public void __setitem__(String key, PyObject value) {
-        synchronized (context) {
-            int scope = context.getAttributesScope(key);
-            if (scope == -1)
-                scope = ScriptContext.ENGINE_SCOPE;
-            context.setAttribute(key, value.__tojava__(Object.class),
scope);
-        }
+        int scope = context.getAttributesScope(key);
+        if (scope == -1)
+            scope = ScriptContext.ENGINE_SCOPE;
+        context.setAttribute(key, value.__tojava__(Object.class), scope);
     }
 
     @ExposedMethod
@@ -91,11 +85,9 @@
     }
 
     public void __delitem__(String key) {
-        synchronized (context) {
-            int scope = context.getAttributesScope(key);
-            if (scope == -1)
-                throw Py.KeyError(key);
-            context.removeAttribute(key, scope);
-        }
+        int scope = context.getAttributesScope(key);
+        if (scope == -1)
+            throw Py.KeyError(key);
+        context.removeAttribute(key, scope);
     }
 }
Index: src/org/python/jsr223/PyScriptEngine.java
===================================================================
--- src/org/python/jsr223/PyScriptEngine.java	(revision 6958)
+++ src/org/python/jsr223/PyScriptEngine.java	(working copy)
@@ -2,9 +2,12 @@
 
 import java.lang.reflect.Method;
 import org.python.core.*;
+
 import java.io.Reader;
 import java.lang.reflect.InvocationHandler;
 import java.lang.reflect.Proxy;
+import java.util.List;
+
 import javax.script.AbstractScriptEngine;
 import javax.script.Bindings;
 import javax.script.Compilable;
@@ -21,20 +24,21 @@
 
     private final PythonInterpreter interp;
     private final ScriptEngineFactory factory;
-    private final PyModule module;
 
     PyScriptEngine(ScriptEngineFactory factory) {
         this.factory = factory;
-        interp = new PythonInterpreter(new PyScriptEngineScope(this,
context));
-        module =
(PyModule)Py.getSystemState().modules.__finditem__("__main__");
+        interp = new PythonInterpreter();
+        interp.setThreadLocals(true);
     }
-
+    
     public Object eval(String script, ScriptContext context) throws
ScriptException {
         return eval(compileScript(script, context), context);
     }
 
     private Object eval(PyCode code, ScriptContext context) throws
ScriptException {
         try {
+            // These affect states that are all thread-bound:
+            interp.setLocals(new PyScriptEngineScope(this, context));
             interp.setIn(context.getReader());
             interp.setOut(context.getWriter());
             interp.setErr(context.getErrorWriter());
@@ -67,6 +71,7 @@
     private PyCode compileScript(String script, ScriptContext context)
throws ScriptException {
         try {
             String filename = (String)
context.getAttribute(ScriptEngine.FILENAME);
+            interp.setLocals(new PyScriptEngineScope(this, context));
             if (filename == null) {
                 return interp.compile(script);
             } else {
@@ -80,6 +85,7 @@
     private PyCode compileScript(Reader reader, ScriptContext context)
throws ScriptException {
         try {
             String filename = (String)
context.getAttribute(ScriptEngine.FILENAME);
+            interp.setLocals(new PyScriptEngineScope(this, context));
             if (filename == null) {
                 return interp.compile(reader);
             } else {
@@ -93,6 +99,7 @@
     public Object invokeMethod(Object thiz, String name, Object...
args) throws ScriptException,
             NoSuchMethodException {
         try {
+            interp.setLocals(new PyScriptEngineScope(this, context));
             if (!(thiz instanceof PyObject)) {
                 thiz = Py.java2py(thiz);
             }
@@ -109,6 +116,7 @@
     public Object invokeFunction(String name, Object... args) throws
ScriptException,
             NoSuchMethodException {
         try {
+            interp.setLocals(new PyScriptEngineScope(this, context));
             PyObject function = interp.get(name);
             if (function == null) {
                 throw new NoSuchMethodException(name);
@@ -120,6 +128,7 @@
     }
 
     public <T> T getInterface(Class<T> clazz) {
+        PyModule module =
(PyModule)Py.getSystemState().modules.__finditem__("__main__");
         return getInterface(module, clazz);
     }
 
@@ -138,6 +147,7 @@
             new InvocationHandler() {
                 public Object invoke(Object proxy, Method method,
Object[] args) throws Throwable {
                     try {
+                        interp.setLocals(new
PyScriptEngineScope(PyScriptEngine.this, context));
                         PyObject pyMethod =
thiz.__findattr__(method.getName());
                         if (pyMethod == null)
                             throw new
NoSuchMethodException(method.getName());

Thanks!  I will look at this in the next couple days...

Could you attach your patch? It's not applying because the lines wrapped.

Any luck? I'll just DropBox it for you:

http://dl.dropbox.com/u/122806/jsr223-patch.txt

Any luck? I'll just DropBox it for you:

http://dl.dropbox.com/u/122806/jsr223-patch.txt

Thanks, that works... will look at it in the morning.

I took a look at your patch. I'm not sure this is the right way to solve things, and I started on a better way but did not get finished in time. I will post what I have tomorrow; I think the best way to continue from there is to write some more detailed tests.

Sure, whatever you think best.

Keep in mind that the problem is not just for JSR-223, but for embedding Jython in general.

Sure - I agree this is a useful feature for embedders, and I've kept it at the level of PythonInterpreter for the use of non-JSR 223 clients.

(Sorry to say "I'll post it tomorrow" but the code is on my laptop which has a dead battery and my charger is at work...)

Take your time! I'm just happy that this is taken seriously. I've been distributing a patched version of Jython with Prudence for far too long. Would be very nice to finally be in line with the main.

Attached. I may be able to get back to this in the next week, but I can't promise anything.

I made a bit more time (at the expense of many, many other things I should be doing...) to fix this up.  I think it's fine for JSR 223 purposes now.  At some point (probably March) I will refactor this so the thread-local behavior is in a PythonInterpreter subclass.  Another useful behavior for non-JSR 223 embedders, I think, would be to have shared globals and per-thread locals.

Do note that it doesn't create per-thread locals by default to preserve the existing behavior, but if you create a Bindings object and bind it, it should work.

I included a simple threaded test case.  If this doesn't work for you, it would help if you could modify ScriptEngineTest.java with a failing test case.

You can build & run this single test case class as follows:

ant -emacs singlejavatest -Dtest=ScriptEngineTest

Thanks for the taking the time, Nicholas. I'm surprised that embedded Jython isn't a common enough use case to make this a priority, but am glad for the attention!

Has your code been merged into trunk, or should I apply the patch?

Nope, I haven't merged into trunk yet - waiting for feedback from you among other people :-)

I think a lot of the Jython embedders haven't caught up with 2.5 yet; we still get a lot of questions about 2.1 and 2.2.

Committed in r6962.

Thanks, Nicholas. I am on a much-needed vacation and didn't have time to look at your patch, but will gladly evaluate and test when I get back.

Unfortunately, I can't get revision 6976 to work at all.

I'm getting an exception in the creation of the PythonPOSIXHandler. After some digging, I managed to spit out this stack trace:

java.lang.NoSuchFieldError: LoadNow
	at org.python.posix.POSIXFactory$1.<init>(POSIXFactory.java:13)
	at org.python.posix.POSIXFactory.<clinit>(POSIXFactory.java:11)
	at org.python.modules.posix.PosixModule.<clinit>(PosixModule.java:62)
	at org.python.modules.Setup.<clinit>(Setup.java:29)
	at org.python.core.PySystemState.initBuiltins(PySystemState.java:1001)
	at org.python.core.PySystemState.doInitialize(PySystemState.java:837)
	at org.python.core.PySystemState.initialize(PySystemState.java:756)
	at org.python.core.PySystemState.initialize(PySystemState.java:706)
	at org.python.core.PySystemState.initialize(PySystemState.java:699)
	at org.python.core.PySystemState.initialize(PySystemState.java:693)
	at org.python.core.PySystemState.initialize(PySystemState.java:689)
	at org.python.core.ThreadStateMapping.getThreadState(ThreadStateMapping.java:17)
	at org.python.core.Py.getThreadState(Py.java:1307)
	at org.python.core.Py.getThreadState(Py.java:1303)
	at org.python.core.Py.getSystemState(Py.java:1323)
	at org.python.util.PythonInterpreter.<init>(PythonInterpreter.java:102)
	at org.python.util.PythonInterpreter.threadLocalStateInterpreter(PythonInterpreter.java:77)
	at org.python.jsr223.PyScriptEngine.<init>(PyScriptEngine.java:27)
	at org.python.jsr223.PyScriptEngineFactory.getScriptEngine(PyScriptEngineFactory.java:85)
	at javax.script.ScriptEngineManager.getEngineByName(ScriptEngineManager.java:225)

If r6974 works for you, I'm guessing the JFFI/ctypes merge broke something.  Please file a separate bug on this issue.