Issue1216

classification
Title: importing Java classes doesn't work if Jython is under 'small' class loader
Type: behaviour Severity: normal
Components: Core Versions: 2.5b0
Milestone:
process
Status: closed Resolution: accepted
Dependencies: Superseder:
Assigned To: Nosy List: cgroves, doublep, leosoto
Priority: Keywords: patch

Created on 2008-12-29.15:01:34 by doublep, last changed 2010-02-01.15:55:22 by leosoto.

Files
File name Uploaded Description Edit Remove
jython-classloading-fix.diff doublep, 2008-12-29.15:01:33 proposed fix (against 2.5b0 sources)
Messages
msg3984 (view) Author: (doublep) Date: 2008-12-29.15:01:33
Jython doesn't use Thread.currentThread().getContextClassLoader() when
importing Java classes from Python code.  So, if Jython itself lives in
a 'small' classpath that cannot see the main code classes, the classes
will not be importable from Python code, at least not without extra
black magick.

Using the thread-context class path is a common idiom used by many
libraries that can dynamically load user classes.

Problems with class loaders are typical to web application, especially
large.  They often have convoluted hierarchy of class loaders.  E.g. I
have something like this:

    .... < Class loader X < ... < Main class loader

Jython is visible to X, while main classes become visible only to
higher-level class loader 'Main'.  So, currently Jython simply doesn't
see the application classes and fails to import them.

The attached patch does solve my problem.
msg3989 (view) Author: Charlie Groves (cgroves) Date: 2008-12-30.00:02:46
Patch applied on trunk in r5812. Thanks!

I changed the implementation a bit, but I don't think it'll make a
difference. Where your patch immediately went to the context class
loader if there wasn't a class loader on sys, the version I committed
tries imp.getSyspathJavaLoader first. Without that, classes on sys.path
will never be found as the context loader either finds it or throws a
ClassNotFoundException.

I also added a test in Lib/test_java_integration.py that imports a class
that's only available on the context class loader and subclasses it, so
I think it's working.
msg3996 (view) Author: (doublep) Date: 2008-12-30.08:33:13
Thanks.  I will test if this is released as some 2.5.x.
msg5471 (view) Author: Leonardo Soto (leosoto) Date: 2010-01-31.04:12:51
I think I understand the problem and the feature request. But since I'm messing with class loading in Jython right know:

Any reason why using PySystemState#setClassLoader wasn't enough for this use case?

I'm about to make a case for ignoring the context classloader on the basis that PySystemState's classLoader does a better job at covering this kind of problems. Perhaps this is a use case that would prove me wrong?
msg5472 (view) Author: (doublep) Date: 2010-02-01.10:33:06
> Any reason why using PySystemState#setClassLoader wasn't enough for this use case?

I wasn't even aware of it.

> I'm about to make a case for ignoring the context classloader on the basis that PySystemState's classLoader does a better job

Please don't.  Current Jython just works for me.  You basically want to stop using common idiom in preference of custom solution.  What you will achieve is:
- Jython will _not_ just work for many people out of the box; they will have to actively look for a solution after spending time investigating what's wrong to begin with;
- many people will overlook PySystemState#setClassLoader just as I did.

On the upside you will probably have simpler code inside of Jython.

In the end you will make using Jython harder (mainly due to debugging, not because of one extra statement).  It will also become "less standard" by not using a mechanism already present in Java.

What is the problem with using PySystemState class loader else falling back to thread context class loader?
msg5473 (view) Author: Leonardo Soto (leosoto) Date: 2010-02-01.14:13:21
Thanks for making that point.

The downside of the context class loader is that Jython can't determine who set it. Assuming that a Jython user set it to make more classes available is not very safe. JavaEE containers differ on what they do with the context class loader (and some do really evil things), so you can't just trust that it will be a child of the classloaders you have tried first.

Anyway, we will do all what we can to keep this, at least for backwards compatibility.
msg5474 (view) Author: (doublep) Date: 2010-02-01.14:20:15
> The downside of the context class loader is that Jython can't determine who set it.

It doesn't have to, really.  Context class loader is supposed to be "the right thing".  If it happens to be not the right thing for any reason, users can always override it with PySystemState#setClassLoader you mentioned, right?

> JavaEE containers differ on what they do with the context class loader

They presumably know how to find classes of the application better.  Jython should "trust" their decision.  If that happens to be wrong, it is a bug in the container, not in Jython; and, in any case, there is still a way to override.
msg5475 (view) Author: (doublep) Date: 2010-02-01.14:29:45
To clarify: as I understand it context class loader serves as a sort of implementation of pseudo-method "findThatClassForMe".  It should be implemented by whatever party is capable of doing that --- e.g. the webserver (container).  Jython, whoever, is not such a party.  It should rely on whatever "knows" how to do that, not try to reinvent the wheel; it would actually be impossible to know what to do with every container out there.
msg5476 (view) Author: Leonardo Soto (leosoto) Date: 2010-02-01.15:02:13
It's not that easy. Ant is a practical example that just using context classloader can be wrong. 

When running Jython tasks such as Jycompile Ant sets the right current classpath but doesn't update the Thread's context class loader.

Just assuming that the context class loader is always set always a sensible value is not safe in the real world.
msg5477 (view) Author: (doublep) Date: 2010-02-01.15:14:25
I suggest that you file a bug against Ant then.  Having to manually configure each other library is bad, especially when there already is a standard mechanism to avoid it.

Off top of my head I can name at least three libraries used in out application that need class loader in order to work and have no problem picking it up from WebLogic: MyFaces, Hibernate and JasperReports.  Brief look at source code suggests they all use context class loader.
msg5478 (view) Author: Leonardo Soto (leosoto) Date: 2010-02-01.15:17:41
You refer to it as a "standard mechanism". Do you have any pointers backing that?
msg5479 (view) Author: (doublep) Date: 2010-02-01.15:36:18
I admit I don't.  After a brief search this is the best I could find: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=4452022

Note that it suggests using ClassLoader.getSystemClassLoader() or else Thread.currentThread().getContextClassLoader().  I actually didn't even know about the former, but my understanding is that it is a bootstrap classloader and is thus useful only for JRE itself.
msg5480 (view) Author: Leonardo Soto (leosoto) Date: 2010-02-01.15:55:22
Yeah, the devil is on the details.

The best resource I have found is <http://www.javaworld.com/javaworld/javaqa/2003-06/01-qa-0606-load.htm>. On page 3 it tells the sad truth: If the current classloader and the context classloader aren't on a (direct or indirect) parent-child relationship, there is no way to be safe. 

Whichever you pick may end up causing problems (i.e., not finding classes). And trying both (what Jython is doing now to support this feature request) may end up causing another kind of problems (basically, loading classes twice).
History
Date User Action Args
2010-02-01 15:55:22leosotosetmessages: + msg5480
2010-02-01 15:36:19doublepsetmessages: + msg5479
2010-02-01 15:17:42leosotosetmessages: + msg5478
2010-02-01 15:14:25doublepsetmessages: + msg5477
2010-02-01 15:02:13leosotosetmessages: + msg5476
2010-02-01 14:29:45doublepsetmessages: + msg5475
2010-02-01 14:20:15doublepsetmessages: + msg5474
2010-02-01 14:13:22leosotosetmessages: + msg5473
2010-02-01 10:33:07doublepsetmessages: + msg5472
2010-01-31 04:12:51leosotosetnosy: + leosoto
messages: + msg5471
2008-12-30 08:33:13doublepsetmessages: + msg3996
2008-12-30 00:02:47cgrovessetstatus: open -> closed
resolution: accepted
messages: + msg3989
nosy: + cgroves
2008-12-29 15:01:35doublepcreate