Pull requests

#19 Declined
Repository
tlecomte tlecomte
Branch
subclassAudits
Repository
jython jython
Branch
default

(PySystemStateCloser.ShutdownCloser) Use Runnable instead of Thread

Author
  1. tlecomte
Reviewers
Description

Dear all,

This is a pull request to fix a leak I have observed when using Jython embedded in an application capable of reloading its classloader. When there is a SecurityManager, the ShutdownCloser thread from PySystemStateCloser remains forever in a field named subclassAudits in java.lang.Thread, preventing proper garbage-collection.

The problem is explained here: http://code.google.com/p/guava-libraries/issues/detail?id=92#c86

The solution is simple: ShutdownCloser can implement Runnable instead of extending Thread. Note that there are other classes extending Thread in Jython, so it may be interesting to apply the same change to them (I have not investigated further myself).

Please note that this pull request also contains the commit for https://bitbucket.org/jython/jython/pull-request/15/ensure-that-the-site-module-is-imported This is just because I forgot to branch right after forking Jython...

  • Learn about pull requests

Comments (5)

  1. tlecomte author

    Here is an image showing the path to the GC root from the classloader, showing that a reference to the ShutdownCloser thread prevents an unused classloader from being garbage-collected. This path was obtained Yourkit, with Jython embedded in Icy (an open-source image-analysis program). Two classloaders are shown : the top one is the live one, the bottom one is not used anymore and should be garbage-collected. The failure to garbage-collect it ultimately causes an out-of-memory in the PermGenSpace of Icy. As shown, a reference to the ShutdownCloser thread is being kept in a field named subclassAudits. This is not directly Jython's fault but rather a bug in Java. Wayway, defining ShutdownCloser as a Runnable instead of a Thread solves this problem.

    Yourkit Path from Classloader to GC Root

  2. Jim Baker

    Thanks very much for submitting your PR. This is a very straightforward way to fix the problem, given the context explained in the cited Guava issue. Also it seems to be cleaner to simply implement a Runnable than subclass a Thread.

    +1

  3. Jim Baker

    So I applied your changeset in 7273:55f7eaddc954, however, I add to export it, then apply via patch, because it was using a named branch.

    To remove this PR from the queue, I'm going to have to "decline" it.