Issue2321

Py.setSystemState(PySystemState newSystemState) places the new value into a ThreadState object that has only a weak reference.  It ceases to exist almost immediately, resulting in Py.setSystemState() doing nothing.  Almost every Python execution has a new default PySystemState.

The fancy two-layer cache in ThreadStateMapping is not necessary and it's causing this bug.

Agreed, it's overzealous. The problem here is that it should be 

    private static final Map<Thread, ThreadState> cachedThreadState =
        new MapMaker().weakKeys().makeMap();

(so not weakValues())

We still want ThreadState to GC upon the corresponding Thread be GC'ed, so weakKeys() makes sense.

A weak key map of Thread to ThreadState is exactly what ThreadLocal does.  The intermediate Object[1] reference and MapMaker use makes no sense.  Only an instance of ThreadLocal<ThreadState> is needed here.

Kevin, ThreadStateMapping.scopedThreadState uses an additional level of indirection to avoid having a ThreadLocal directly refer to a ThreadState, which is necessary to avoid a resource leak where the Thread object has a reference to the ClassLoader of the entire Jython runtime, thereby preventing the runtime from being unloaded until the thread exits. See http://blog.crazybob.org/2006/07/hard-core-java-threadlocal.html and #1327, as well as r7273 (https://hg.python.org/jython/rev/55f7eaddc954).

Kevin, I have given some more thought to this problem. We have two sets of objects - Threads and ThreadState - which we are mapping with ThreadStateMapping.cachedThreadState. Threads may either run a single task, or be used in a thread pool to avoid the overhead of constructring new threads for a given task, with very different lifetimes. ThreadState objects are lightweight and simply track a mapping to PySystemState, along with some other dynamically scoped data, such as call_depth.

Because there are independent lifetimes on both sides, the mapping should be weak keys AND weak values (so I was wrong in msg9795). Otherwise a ThreadState could stop a Thread from being GC'ed, and vice versa. In particular, we don't want a long running Thread object in a thread poool to not be able to unload the Jython runtime, because it's strongly referring to a ThreadState, which in turn strongly refers to a PySystemState.

Interestingly, there's a small but significant performance improvement in benchmarks (my recollection is 10% or so on pystone) because even though the ThreadState can generally be recreated as necessary, just to avoid additional lookups.

However, this weak key/weak value mapping will raise an issue with code patterns like so, if there's no other reference to the PySystemState than a newly allocated ThreadState:

Py.setSystemState(new PySystemState());

In the Jython codebase, there's one example of the above fragment of code in restarting an interpreter. In this case it looks like the initialization will be done twice. However, this doesn't appear to do anything wrong, just slightly less efficient. In all other cases in the Jython codebase, such as by using a PythonInterpreter, a PySystemState always is being referred to with a strong reference, so there's no loss of this state.

We decided in 2.7 that we would make backwards breaking changes that we could not do in 2.5, as discussed in #1327, if it solves long standing memory leaks and other problems.

Oops, didn't include some details - ThreadStates can generally be recreated of course if there's no dynamic scope (so call_depth, etc) because that's just a function of being in the thread stack. The fact that weakKeys/weakValues can still work here is that even a generational GC is not reclaiming ThreadState objects as fast a tight performance loop as seen in pystone.

We still might want to expand on the caching behavior in some way in 2.7.1

Code is breaking where the provided PySystemState must differ in some way from the default prototype - argv, path, stdout, stderr, stdin, etc.  Every call to Py.getSystemState() scattered throughout Jython potentially returns a new object until something calls ThreadState.enterRepr().  In debugging, many PySystemState instances may come and go before reaching that point.

Some refactoring is needed in Jython.  The problem is that ThreadState has become the real execution state but it's not very well exposed or managed.  PySystemState is well exposed but it's just a field in a temporary object.

My current workaround is to get my hands on the ThreadState and keep a hard reference:

final ThreadState ts= Py.getThreadState(newSystemState);
ts.systemState = newSystemState; 
 ... save ts somewhere so it doesn't evaporate ...

It's non-obvious and what it does to ThreadState.systemState isn't really legit.  It leaves a very high risk of future code calling Py.setSystemState(), because that looks right, and then failing for mysterious reasons.

Kevin, I understand your concerns, and any input you might have into a better API would be highly appreciated. However, I think maintaining the hard reference explicitly can be a reasonable workaround for the moment, at least compared to the alternative that forced us to make these changes.

Per discussion, we need a better API for setting PySystemState, and likely API deprecation. We should also better document expectations of how to use the API - much of our API is really not public, because of the limitations of Java - but this API is.

Hello! This bug is biting my application which uses Py.setSystemState(). I was wondering what would be the best fix and I wonder if the following would be OK:

We change the code to be that of msg9795 (no weak values) but then we store the provided *PySystemState* to a WeakReference with a ReferenceQueue.

And then we use the associated ReferenceQueue to detect when the PySystemState is garbage collected so that we can remove the associated ThreadState from the map...

BTW, I was never really clear whether sharing the PySystemState between two threads was safe or not (this is where documenting the API would be helpful!).

Jan, PySystemState can be shared by multiple threads and is designed to do so. Many applications only need to have one instance of a PySystemState, regardless of the number of threads they may be running.

Jim, great this was my assumption... but I had not found a formal answer so far... What about the suggested solution? I am trying to hack an fix... would you like me to do it properly and submit a pull request? I was thinking of having ThreadState extend WeakReference<PySystemState> to find it again in the reference queue... anything wrong with that?

Letting ThreadState extend WeakReference is not a viable option because  Py.setSystemState(...) needs to replace the reference held in the ThreadState... which is not possible in a WeakReference. Before I dig any further into the issue, I'll wait for your advice (which can be to just let *you* fix it). Please let me know.

Jan, I think the answer is "you should fix it", not Jython. Basically all references to PySystemState should be owned by the container, whether that is org.python.util.jython, app containers like Jetty (via ModJy or Fireside), JSR223 (see PyScriptEngine), or any embedding solution.

However, there may be some common ways that this ownership should be managed, so that's where we can come up with a better API.

Jim, I am afraid that Jython has to do something about it :) Please read carefully.

The issue is that the map is *not* a "weak-map" Thread -> PySystemState...

It is a "weak-map" Thread -> ThreadState

What is occurring is the following:
1. Py.setSystemState calls ThreadStateMapping.getThreadState 
2. which is creating a ThreadState instance... that 
3. *never gets* out of Py.setSystemState!!
4. As a local variable, it is garbage collected very early
5. The ThreadState is evicted from the "weak-map" since the value is GCed
6. Any time later, if I call Py.getSystemState, the "weak-map" does not find an entry for my thread
7. Py.getSystemState returns Py.defaultSystemState

So how do you want us to fix anything without a change in Jython?? I already keep a strong ref to the PySystemState... there's nothing I can do more outside of Jython.

Jan, do you think you can create a patch that will fix this problem? We can then use this as a starting point for determining what the better API should be.

Hi Jim, this is done in the pull-request 62: https://bitbucket.org/jython/jython/pull-request/62
With this, there is no change in the API. However, I might have missed something so don't hesitate to review with care! I have left a few comments/questions in the code...
Best regards

Jan, thanks! I will review this PR today.

Deferring review due to lack of time today, but definitely this week

Hi Jim! Have you had any chance to do the review?
Please forgive my insisting, but I believe that the original bug title "Py.setSystemState() is a no-op" was more accurate... it is not just a documentation issue but a major regression for those who use that API function: it is a blocker for our transition to Jython 2.7.x
Cheers

Jan, my apologies, I have been sick with a virus for the last 2 weeks, so this has impacted my productivity substantially. Presumably I will get better soon, and then I can do the detailed review that is required.

Sorry to read that! I wish you a very fast recovery!

Hello Jim! Hope you recovered from your sickness! Would you have a few minutes now? BTW, are there any plans for 2.7.1? Thanks in advance!

I am changing the title because it looks that a doc issue has low priority and this is a serious regression wrt Jython 2.5.x

Finally reviewed Jan's https://bitbucket.org/jython/jython/pull-requests/62/suggested-fix-for-2321/diff and with a couple of minor changes, this looks good. Looking forward to trying this out in beta 3!

See this patch, based on JanVok's PR and modified by me per my review comments

Fixed as of https://hg.python.org/jython/rev/dcff83c1c4ac