Details
-
Type:
Bug
-
Status:
Closed
-
Priority:
Major
-
Resolution: Fixed
-
Affects Version/s: 1.0-JSR-4
-
Fix Version/s: 1.0-RC-1
-
Component/s: None
-
Labels:None
-
Environment:Sun JDK 1.4.2_08, Windows
-
Testcase included:yes
-
Number of attachments :
Description
Hi,
After having some time spent hunting a memory leak in our application, I've managed to narrow the problem to Groovy. Basically in our case we use a throw-away classloader that is then used to load a Groovy-generated class (to be more specific, we are using JasperReports with Groovy expressions).
The issue as I see it is linked to GROOVY-375 which is where the use of WeakHashMap was introduced. The intention was correct as it was meant to allow garbage collector to release cached info that cannot longer be used (the info for throw-away classes in our case). However there is a restriction, described in the javadoc of WeakHashMap, that must be respected for this to work: the values stored in the map must not have direct or indirect strong references to their keys. This is not the case for the metaClasses map that maps Class instances to MetaClass instances because the MetaClass holds a direct reference (theClass) back to Class.
Basically this is the same issue as with java.beans.Introspector class which also stores values with strong references to their keys (ie: Class->Method[] in declaredMethodCache field where Method has a strong reference to its class). Introspector class provides flushCaches methods as a fix for this problem.
To demonstrate the problem I've created a junit test that can help to reproduce the problem and to test possible solutions.
One possible fix for this problem is to use the same approach as the Introspector class and to provide a flush method that could be called explicitly by the user. Personally I don't like this solution as it delegates the responsibility to the user of the library which is bug-prone. Another solution would be to replace the strong references with WeakReferences where required. The attached test can help to test this solution.
Peter.
I've created a patch for the HEAD version of the MetaClassRegistry.java file. I've used a WeakReference to wrap the values that are stored in the WeakHashMap. There was a problem with default MetaClass instances that are initialized in the constructor:
// lets register the default methods
lookup(DefaultGroovyMethods.class);
registerMethods(DefaultGroovyMethods.class, true);
lookup(DefaultGroovyStaticMethods.class);
registerMethods(DefaultGroovyStaticMethods.class, false);
Apparently those should be always present in the HashMap overwise the unit tests do not pass (there is an error that says the String.plus() method cannot be found). To fix this I've mixed strong references with WeakReferences so that the default MetaClass instances are never garbage collected. There is a flag called "initializing" that is set to true during the MetaClassRegistry construction and that determines the references to be stored directly without wrapping them with WeakReference. Please see the patch for more details. With this fix all unit tests pass.