Issue2142

classification
Title: Set Thread classloader when entering Jython context
Type: Severity: normal
Components: Core Versions: Jython 2.7
process
Status: open Resolution: remind
Dependencies: Superseder:
Assigned To: Nosy List: rec, zyasoft
Priority: Keywords: patch

Created on 2014-05-13.09:04:18 by rec, last changed 2014-06-26.22:00:40 by zyasoft.

Files
File name Uploaded Description Edit Remove
contextclassloader.diff zyasoft, 2014-06-26.22:00:39
Messages
msg8388 (view) Author: Richard Eckart de Castilho (rec) Date: 2014-05-13.09:04:18
It appears to be a common and good practice to set the context classloader of the current thread when entering a context that offers access to new classes, e.g. in Jython via the syspath. 

E.g. the Spring framework does its class/resource lookup by taking the context classloader into account if it is set. The Groovy language sets the context classloader when entering into a Groovy context.

Such a behaviour can allow Java code invoked from within the Jython context access to classes/resources available through the syspath.

Our concrete use-case is that we build Jython scripts that use jip [1] to dynamically download and add Maven dependencies, e.g.:

  require('commons-lang:commons-lang:2.6')
  from org.apache.commons.lang import StringUtils

We do not only access classes from such dependencies in Jython, but e.g. access them via reflection from Java code invoked via Jython or access resources stored in such JARs from Java code invoked via Jython (e.g. via Spring).

To make this work, we currently need to add the following line to the beginning of each script

  Thread.currentThread().contextClassLoader = getSyspathJavaLoader()

It would be great if Jython did set/restore the context classloader automatically when entering the Jython context, so that we do not have to do this manually.

[1] https://pypi.python.org/pypi/jip
msg8698 (view) Author: Jim Baker (zyasoft) Date: 2014-06-19.04:26:16
Lots of merit to this idea. We will need to understand performance/other implications before we can apply.
msg8789 (view) Author: Richard Eckart de Castilho (rec) Date: 2014-06-22.20:44:57
@Jim: thanks for noticing this request. Can you be specific on what exactly you worry about? I feel that "performance/other implications" a very broad statement that is hard to operate on.
msg8793 (view) Author: Jim Baker (zyasoft) Date: 2014-06-23.04:40:25
My concerns are:

1. We would need to do for all Python threads upon entry into Python context (possibly calling back from Java). The natural point to do this in this method in org.python.core.Py:

public static final ThreadState getThreadState(PySystemState newSystemState)

Because of the way ThreadState is passed through calls, this will not be called all the time, but it's certainly a hot path.

2. Such a hard reference from a thread to a class loader will prevent the class loader from being GCed (see this relevant email thread http://sourceforge.net/p/jython/mailman/message/32486365/), unless there's some sort of cleanup. See for example this highly relevant discussion on the Tomcat wiki: http://wiki.apache.org/tomcat/MemoryLeakProtection#cclThreadSpawnedByWebApp
msg8826 (view) Author: Jim Baker (zyasoft) Date: 2014-06-26.22:00:38
Because of the enabling work in https://bitbucket.org/jimbaker/jython-resource-leaks, I was able to solve the performance and resource leak issues I mentioned earlier, with the attached contextclassloader.diff

However this currently causes a number of failures that I haven't been able to fix in a number of tests. test_chdir demonstrates:

======================================================================
FAIL: test_compile_dest (__main__.PyCompileTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "dist/Lib/test/test_chdir.py", line 407, in test_compile_dest
AssertionError: '__pyclasspath__/tmp1bIis4chdir_test$py.class' != 'tmp1bIis4chdir_test$py.class'

It's interesting, because there's no obvious reason for the connection between SyspathJavaLoader, a ClassLoader, with ClasspathPyImporter, which is an importer of Python code.

At the very least try out this diff, and see if it helps. The specific revision I'm using is 7328:06d196031fa1 of the above fork.
History
Date User Action Args
2014-06-26 22:00:40zyasoftsetfiles: + contextclassloader.diff
keywords: + patch
messages: + msg8826
2014-06-23 04:40:25zyasoftsetmessages: + msg8793
2014-06-22 20:44:57recsetmessages: + msg8789
2014-06-19 04:26:16zyasoftsetresolution: remind
messages: + msg8698
nosy: + zyasoft
2014-05-13 09:04:32recsetcomponents: + Core
versions: + Jython 2.7
2014-05-13 09:04:19reccreate