Issue2127

classification
Title: Weakref and zombie threads
Type: Severity: normal
Components: Core Versions: Jython 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: indra, rchyla, zyasoft
Priority: high Keywords: patch

Created on 2014-04-20.05:58:20 by rchyla, last changed 2014-07-01.04:05:25 by zyasoft.

Files
File name Uploaded Description Edit Remove
TestJythonWeakref.java rchyla, 2014-04-20.05:58:19
GlobalRef.patch indra, 2014-04-26.11:23:00
Messages
msg8294 (view) Author: roman (rchyla) Date: 2014-04-20.05:58:19
Hi, I've started using Jython (from inside Java) and discovered a problem; when a weakref is imported (eg. by 'import logging') objects are not being recycled 

Please look at the attached unittest to reproduce the problem - I'm using jython2.7.b1

You will need to use http://labs.carrotsearch.com/randomizedtesting.html

This is what I'm seeing:


Apr 20, 2014 1:54:17 AM com.carrotsearch.randomizedtesting.ThreadLeakControl checkThreadLeaks
WARNING: Will linger awaiting termination of 1 leaked thread(s).
Apr 20, 2014 1:54:37 AM com.carrotsearch.randomizedtesting.ThreadLeakControl checkThreadLeaks
SEVERE: 1 thread leaked from SUITE scope at org.jython.monty.TestJythonWeakref: 
   1) Thread[id=13, name=weakref reaper, state=WAITING, group=TGRP-TestJythonWeakref]
        at java.lang.Object.wait(Native Method)
        at java.lang.ref.ReferenceQueue.remove(ReferenceQueue.java:135)
        at java.lang.ref.ReferenceQueue.remove(ReferenceQueue.java:151)
        at org.python.modules._weakref.GlobalRef$RefReaperThread.collect(GlobalRef.java:212)
        at org.python.modules._weakref.GlobalRef$RefReaperThread.run(GlobalRef.java:221)
Apr 20, 2014 1:54:37 AM com.carrotsearch.randomizedtesting.ThreadLeakControl tryToInterruptAll
INFO: Starting to interrupt leaked threads:
   1) Thread[id=13, name=weakref reaper, state=WAITING, group=TGRP-TestJythonWeakref]
Apr 20, 2014 1:54:40 AM com.carrotsearch.randomizedtesting.ThreadLeakControl tryToInterruptAll
SEVERE: There are still zombie threads that couldn't be terminated:
   1) Thread[id=13, name=weakref reaper, state=WAITING, group=TGRP-TestJythonWeakref]
        at java.lang.Object.wait(Native Method)
        at java.lang.ref.ReferenceQueue.remove(ReferenceQueue.java:135)
        at java.lang.ref.ReferenceQueue.remove(ReferenceQueue.java:151)
        at org.python.modules._weakref.GlobalRef$RefReaperThread.collect(GlobalRef.java:212)
        at org.python.modules._weakref.GlobalRef$RefReaperThread.run(GlobalRef.java:221)
msg8311 (view) Author: Indra Talip (indra) Date: 2014-04-26.11:23:00
From what I can reason from the code and the test case the issue is that the RefReaperThread is leaked rather than objects in the ReferenceQueue not being GC'd.  

The attached patch *should* allow the reference reaper thread created by GlobalRef to die *and* respawn if necessary. It does this be changing the RefReaper to a Runnable (attempting to avoid the issue detailed in https://bitbucket.org/jython/jython/pull-request/19/pysystemstateclosershutdowncloser-use/diff) and implementing Callable so that it can be registered with the PySystemState as a resource closer and thus be cleaned up when PySystemState.cleanup() is called.

I think I managed to run rchyla's test case successfully once I added a call to the interpeter's cleanup method, i.e.:
            interp.exec("import weakref");
            interp.cleanup();


I'm not sure of the best way to incorporate a test for this into the build so any pointers as to how/where to add a test would be great.
msg8526 (view) Author: Jim Baker (zyasoft) Date: 2014-05-22.00:49:05
Target beta 4
msg8823 (view) Author: Jim Baker (zyasoft) Date: 2014-06-25.06:22:14
Applied Indra's patch in https://bitbucket.org/jimbaker/jython-resource-leaks, with the intent to merge this into trunk sometime soon, maybe this week

Note that there are a number of failing tests. I don't think we need to support recursion_limit, that's clearly a CPython-implementation detail. It's interesting to see that the change in ThreadStateMapping to use a weakkey/weakval map instead of a ThreadLocal is enough to cause a failure in test_weakset. However, I really doubt that _IterationGuard is as threadsafe as it claims, at least as it interacts with the remove method, given that 

                if self._iterating:
                    self._pending_removals.append(item)
                else:
                    self.data.discard(item)

is clearly not guaranteed to be atomic. The solution is to revisit this implementation with the use of Guava MapMaker.
msg8824 (view) Author: Jim Baker (zyasoft) Date: 2014-06-25.06:27:24
Re incorporating Roman's test - it does look good, with the call to cleanup (soon to be a synonym for close). I especially like the idea of using com.carrotsearch.randomizedtesting.annotations to check for thread leaks. Just a matter of figuring out the necessary changes to our build.xml and placement into our existing JUnit tests
msg8829 (view) Author: Jim Baker (zyasoft) Date: 2014-06-28.03:07:40
Fixed as of http://hg.python.org/jython/rev/4e538d0ebbd7
History
Date User Action Args
2014-07-01 04:05:25zyasoftsetstatus: pending -> closed
2014-06-28 03:07:40zyasoftsetstatus: open -> pending
resolution: accepted -> fixed
messages: + msg8829
2014-06-25 06:27:24zyasoftsetmessages: + msg8824
2014-06-25 06:22:15zyasoftsetmessages: + msg8823
2014-05-22 00:49:05zyasoftsetmessages: + msg8526
2014-05-09 18:30:12zyasoftsetnosy: + zyasoft
2014-05-04 19:55:10zyasoftsetpriority: high
resolution: accepted
components: - Library
versions: + Jython 2.7
2014-04-26 11:23:02indrasetfiles: + GlobalRef.patch
keywords: + patch
messages: + msg8311
nosy: + indra
2014-04-20 05:58:20rchylacreate