Bug 127963 - ContextFinder caches classes from Class#forName
Summary: ContextFinder caches classes from Class#forName
Status: RESOLVED FIXED
Alias: None
Product: Equinox
Classification: Eclipse Project
Component: Framework (show other bugs)
Version: 3.1.1   Edit
Hardware: PC Windows 2000
: P3 minor (vote)
Target Milestone: 3.4 M2   Edit
Assignee: equinox.framework-inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2006-02-15 00:09 EST by Yang Liu CLA
Modified: 2014-01-17 10:45 EST (History)
8 users (show)

See Also:


Attachments
patch (1.34 KB, patch)
2007-03-27 10:01 EDT, Thomas Watson CLA
no flags Details | Diff
patch (2.03 KB, patch)
2007-08-23 17:35 EDT, Thomas Watson CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yang Liu CLA 2006-02-15 00:09:06 EST
I noticed that Eclipse use the class "ContextFinder" as the default ContextClassLoader for all the threads. And "ContextFinder" as a special behavior of using the current stack to find a ClassLoader to load a class.

But this will have one problem: ContextFinder's lifecycle will accross the whole eclipse instance lifecycle, that is, it will never be garbage collected. So all the classes loaded by it will not be garbage collected!
Comment 1 Thomas Watson CLA 2006-02-15 09:03:49 EST
ContextFinder is just a delegating classloader it should not define any classes.  Instead it delegates to other bundle classloaders to define the classes.  Its pool of defined classes should always be zero so it should not prevent any classes from being garbage collected.  Do you have measurements that contradict this?
Comment 2 Yang Liu CLA 2006-02-16 03:23:00 EST
The cash is in the native code.

In my special case, the following scenario will happen:

1. I am using JNDI, thus com.sun.naming.internal.VersionHelper12#loadClass(className) is called.
2. VersionHelper12#loadClass will call Class#forName, which further call Class#forName0(classname, initialize, loader), here the "loader" is ContextFinder.
3. Class#forName0 is a native method, it maps to #Java_java_lang_Class_forName0, which will call another native method #JVM_FindClassFromClassLoader
4. #JVM_FindClassFromClassLoader will further call SystemDictionary::resolve_or_fail(name, loader, protection_domain, throwError != 0, CHECK_NULL), here "loader" is still ContextFinder
5. SystemDirectionary#resolve_or_fail will further call SystemDirectory#resolve_instance_class_or_null

In "SystemDirectory#resolve_instance_class_or_null" is where the cash will happen. It will cash the class loading result in the "dictionary()" hash table.
Comment 3 Thomas Watson CLA 2006-02-23 13:50:48 EST
Is the "dictionary()" hash table you are referring to a Hashtable slot on ClassLoader?  I tried to reproduce but I could not see the class getting cached in any java object table in ClassLoader.
Comment 4 Thomas Watson CLA 2006-02-23 14:39:41 EST
The cache must be in native code.  If Class#forName is used with the ContextFinder then a call to ContextFinder#findLoadedClass will return a non-null class object for the requested class.

This only appears to happen when Class#forName is called with the ContextFinder.  When ContextFinder#loadClass(String, boolean) is called normally (not from Class#forName) then the loaded class is not cached in the loaded classes cache for ContextFinder.

This seems to be a VM bug.  ContextFinder did not actually define the class, why would the VM cache the loaded class in ContextFinder.  If this is by design then it seems really strange that it should only happen for Class#forName and not for normal class loading.

I reproduced across Sun and IBM 1.4.2 VMs and Sun 1.5 VM.
Comment 5 Yang Liu CLA 2006-02-24 04:19:07 EST
Yes, the cache is in native code. The "directory()" is in native code.
Maybe should report a bug on JVM.
Comment 6 Pascal Rapicault CLA 2006-04-05 15:37:31 EDT
Yang, Tom, have you opened a bug report against Sun and or IBM for that?
Comment 7 Thomas Watson CLA 2007-03-26 10:15:40 EDT
We have managed to workaround this issue in bug 167695.  If we always delegate to the bundle classloaders first then we do not check the ContextFinder cache until after we have checked the bundles for the class.

I'm leaving this bug open because there are still issues with pinning classes which are loaded with Class.forName on the ContextFinder.  I can see this causing problems in the following cases.

1) A bundle is updated/uninstalled which has classes pinned in the ContextFinder.  The bundle's classloader will never get GC'ed.

2) One bundle X uses ContextFinder to load a class with Class.forName and finds the class in its private class space.  Another bundle Y uses ContextFinder to load the same class but it is not found in bundle Y's private class space.  ContextFinder then does a super.loadClass which then will find the cached version of the class from bundle X even though bundle Y should not have access to the class from bundle X.
Comment 8 Simon Kaegi CLA 2007-03-26 11:02:14 EDT
For the second issue I think we might be able to avoid the super.loadClass() by instead doing getParent().loadClass() or something similar.
Comment 9 Thomas Watson CLA 2007-03-27 10:01:51 EDT
Created attachment 62096 [details]
patch

Good idea, here is a patch that implements this.  Notice that it still calls super methods for resource loading.  I did not see the need to change resouce loads because they do not consult a cache like loadClass does.
Comment 10 Glyn Normington CLA 2007-04-11 10:26:24 EDT
Please note that the VM behaviour observed in this bug is actually correct.

The VM must cache a loaded class for a class loader in two situations:

1. any time defineClass is called, and

2. when the VM calls loadClass, e.g. during resolution or from Class.forName.

#1 is to prevent a defining class loader from giving inconsistent results for a particular class.

#2 is to prevent the VM having to deal with possibly inconsistent results of loadClass. (loadClass can produce inconsistent results e.g. if it delegates to different class loaders when asked to load the same class more than once.)

Of course, applications or custom class loaders can and do call loadClass directly. In this case, the VM cannot intervene to prevent any inconsistent results from loadClass causing confusion, but at least it won't be the VM getting confused.
Comment 11 BJ Hargrave CLA 2007-07-18 15:14:21 EDT
I found some interesting background links on this issue from a post in the JSR 294 mails list [3]

> ... the classic class loading problem [1] which lead to 
> loading constraints being added to the JVMS [2].
> 

[1] http://www.bracha.org/classloaders.ps
[2] http://java.sun.com/docs/books/jvms/second_edition/html/ConstantPool.doc.html#78621
[3] http://altair.cs.oswego.edu/pipermail/jsr294-modularity-eg/2007-April/000035.html
Comment 12 Glyn Normington CLA 2007-07-19 06:23:37 EDT
I'm not convinced the links in comment #11 are really relevant. Loading constraints are a way of ensuring that there is type consistency for parameters etc. loaded in one class loader and passed on a method call to a class loaded in another class loader.

This relates to the caching behaviour for VM initiated class loads (i.e. when the VM calls loadClass while resolving a class's references to other classes). I suspect the caching behaviour during resolution simplifies, or makes possible, the data structures needed to hold the loading constraints.

But this bug relates to the behaviour of Class.forName which is always called during class initialisation or execution rather than class resolution. The behaviour of Class.forName could, in principle, be modified without having to rework the loading constraints implementation. I am trying to feed the requirement for Class.forName not to cause caching in the initiating (but non-defining) class loader into Sun's class loader rearchitecture work for Java 7.
Comment 13 BJ Hargrave CLA 2007-07-19 08:45:55 EDT
The information in comment 11 may not be directly relavent to the pinning issue of this bug, but are generally relavent to the TCCL (ContextFinder) design of Equinox.

Class.forName does not result in a call to ClassLoader.loadClass directly and does not appear to be a synonmym for ClassLoader.loadClass since the VM internals are involved. The behaviour of Class.forName is more like a class load request during class resolution and is thus subject to the consistency checks in the cited paper.

It seems your requirement request for Java 7 would make a call to Class.forName more like a synonym for ClassLoader.loadClass which would not invoke the consistency checks.

In the pre-Java 7 world we are still in a spot of bother here...

Given the stack in comment 2, if the JNDI code used TCCL.loadClass instead of Class.forName(..., TCCL), then we would not be subject to the loading constraints?
Comment 14 Glyn Normington CLA 2007-07-20 04:42:05 EDT
Isn't the loaded class cache to blame rather than loading constraints?
Comment 15 BJ Hargrave CLA 2007-07-20 09:26:38 EDT
(In reply to comment #14)
> Isn't the loaded class cache to blame rather than loading constraints?
> 

I would assume the class would appear in the loaded class cache of the defining loader (i.e. bundle class loader) while also appearing in the loading constraints of the initiating class loader (i.e. ContextFinder). The former case is not an issue since there are already hard references between a class and it's defining loader. It is the hard reference between the loading constraints in the initiating loader and the class that cause the class and it's defining loader to be pinned as long as the initiating loader to be reachable.
Comment 16 Glyn Normington CLA 2007-07-20 11:01:29 EDT
Loading constraints do not prevent garbage collection of the classes and class loaders they refer to. Note that loading constraints are internal to the VM and will not be encountered while tracing live objects starting from statics and thread stacks and following all object references.
Comment 17 Maciej Datka CLA 2007-08-23 10:32:12 EDT
In bug 197819 there is a problem with axis being included in 2 separate plugins and somehow axis classes from one plugin are able to 'see' classes from another plugin (using context classloader).

I am referring to comment #7, point 2:

"
2) One bundle X uses ContextFinder to load a class with Class.forName and finds
the class in its private class space.  Another bundle Y uses ContextFinder to
load the same class but it is not found in bundle Y's private class space. 
ContextFinder then does a super.loadClass which then will find the cached
version of the class from bundle X even though bundle Y should not have access
to the class from bundle X.
"

It seems like exactly this is happening to axis in bug 197819. 

There is a patch for this issue mentioned in comment #9 but i think it has never been committed. 
Can anyone correct me if I am wrong?
Is there a reason why the patch can't be committed?


Comment 18 Thomas Watson CLA 2007-08-23 16:59:33 EDT
We will look to release this for 3.4 M2.  With this fix we still have concerns about the native cache pinning ClassLoaders, but we have gotten confirmation back from the VM team that the native cached should not pin ClassLoaders.  I think this is a good patch to release.
Comment 19 Thomas Watson CLA 2007-08-23 17:35:35 EDT
Created attachment 76827 [details]
patch
Comment 20 Thomas Watson CLA 2007-08-23 17:43:28 EDT
Ignore the last patch.  I thought we needed to use a doPrivileged to isolate the creating of a ClassLoader in the constructor of ContextFinder.  But this is not needed because ContextFinder *is* a ClassLoader so if the caller can create ContextFinder then they can create a ClassLoader already.

I released the previous patch to HEAD.
Comment 21 Maciej Datka CLA 2007-10-15 05:18:26 EDT
Is the fix present in 3.4 M2?
I have downloaded 3.4 M2 with newest mylyn - the bug 197819 still reproduces.
Before I start debugging to confirm and before I produce some example if how to reproduce I'd like to be sure that this fix is in 3.4 M2.
Comment 22 Thomas Watson CLA 2007-10-15 09:48:31 EDT
Yes the fix is in 3.4 M2.  You will still have issues if multiple bundles use Class.forName(..., tccl) instead of tccl.loadClass(...).  In bug 197819 do you know if the two bundles using axis are using the TCCL (Thread Context ClassLoader) in Class.forName()?

If so then the VM performs the caching and there is nothing we can do in to prevent it.  It should not effect other clients that use tccl.loadClass, only clients that use Class.forName(...,tccl).
Comment 23 BJ Hargrave CLA 2007-10-15 11:43:59 EDT
See http://blog.bjhargrave.com/2007/09/classforname-caches-defined-class-in.html for some research I did on this subject.
Comment 24 Chris Aniszczyk CLA 2007-10-15 12:35:35 EDT
more importantly, omg bj has a blog.

bj, see bug 206349 if you approve of the addition to PlanetEclipse.org BJ