ClassLoader leaks in IndicesCallbackFilter

[GoogleCodeExporter]

Scenario:

Application loads classes using a URLClassLoader. Modules are loaded from it. 
There are non-public ImplementedBy classes, but seems to not be the reason for 
the problems.

Problem:

When I remove all references to URLClassLoader and its classes/instances, Guice 
maintains references that don't let it go away.

Patch:

Although it probably don't fixes the proxy leaks, it let the URLClassLoader to 
be released.

The patch is against the 3.0 branch.

Original issue reported on code.google.com by adrianosf on 22 Jul 2011 at 1:51

Attachments:

• guice-classloader-leak.diff

[GoogleCodeExporter]

Can you provide a testcase or example code that demonstrates the original 
problem? Guice trunk has a unit test for proxy class unloading which passes, 
but perhaps something else is going on in your particular scenario that leads 
to a leak. A testcase would be useful in identifying the root cause and help 
verify the patch.

Original comment by mccu...@gmail.com on 22 Jul 2011 at 2:31

[GoogleCodeExporter]

Here is it. Import both projects in eclipse.

When it stops waiting for a key, you may open eclipse MAT or something else and 
see there are three MyClassLoader instances alive.

Note that it shows two problems:
- The one I reported and which my patch fixes
- The Finalizer thread gets created using the user contextClassLoader. However, 
this problem doesn't show in my original (web) project.

Original comment by adrianosf on 22 Jul 2011 at 5:46

Attachments:

• guice-classloader-leak-test.zip

[GoogleCodeExporter]

Thanks, note that the finalizer thread will disappear in the next release (see 
latest news in issue 288)

Original comment by mccu...@gmail.com on 22 Jul 2011 at 5:49

[GoogleCodeExporter]

Original comment by mccu...@gmail.com on 22 Jul 2011 at 6:30

[GoogleCodeExporter]

Is there any update on this?

Is there any chance that my patch or some other fix goes into the next version?

Original comment by adrianosf on 3 Oct 2011 at 2:00

[GoogleCodeExporter]

Is it possible for you to add a patch with a test also, ensuring we don't 
regress in the future?

Original comment by sberlin on 3 Oct 2011 at 2:54

[GoogleCodeExporter]

I don't see how a test for this could be made, since it will depend on when the 
garbage collector run.

Original comment by adrianosf on 3 Oct 2011 at 10:54

[GoogleCodeExporter]

WRT the suggested patch, it should save the declaring class' hashcode (since 
this is fixed) and use that in the hashCode method - otherwise you could get a 
NPE in the CGLIB cache after the class is unloaded.

Original comment by mccu...@gmail.com on 3 Oct 2011 at 12:07

[GoogleCodeExporter]

Patch updated accordingly to #8 comment.

Original comment by adrianosf on 4 Oct 2011 at 11:06

Attachments:

• guice-classloader-leak-2.diff

[GoogleCodeExporter]

Thanks for updating!  A test would still be really really awesome.  It's 
possible to force GCs by calling System.gc(), creating large arrays & sleeping. 
 See BytecodeGenTest around line 250 
(http://code.google.com/p/google-guice/source/browse/trunk/core/test/com/googlec
ode/guice/BytecodeGenTest.java?r=1568#242).  

Original comment by sberlin on 7 Oct 2011 at 1:29

[GoogleCodeExporter]

Results of further investigation when using the original testcase from #c2 with 
Guice trunk: adding System.gc() inside the test loop allows GC to collect at 
least one of the proxy classloaders and all of the test instances and 
interceptors. This shows that it's not as simple as a stray strong reference 
causing this leak.

Instead the underlying issue seems to be that CGLIB uses a static WeakHashMap 
in its generator, using the proxy classloader as the weak key and storing 
details such as the callbacks under the value. Unfortunately WeakHashMaps are 
only cleared when they mutate - so in this example the callback table will be 
kept alive until another enhanced class is generated, at which point the map 
will mutate and the original table will be reclaimed.

Adding weak references to IndicesCallbackFilter works around this issue by 
breaking the indirect link from the WeakHashMap's reference queue back to the 
proxied class. Note that the callback table will still be held in the map's 
queue waiting to be reclaimed, but the extra weak references mean the proxy 
classloader can at least be unloaded.

PS: I attempted to extend the BytecodeGenTest to test this scenario, but 
unfortunately (depending on your perspective) the interceptor classloader is 
always GC'd. Looks like more work is needed to recreate this as a repeatable 
unit test.

Original comment by mccu...@gmail.com on 27 Oct 2011 at 11:08

[GoogleCodeExporter]

Here's an alternative patch that also avoids a strong reference to the 
declaring class or its methods. It uses the MethodWrapper utility class from 
CGLIB to create keys based on the method signatures - this same utility class 
is used to de-duplicate the original method list in the Enhancer, so it's safe 
to use it in the callback filter.

Original comment by mccu...@gmail.com on 28 Oct 2011 at 12:41

Attachments:

• GUICE_ISSUE_643_20111028.patch

[GoogleCodeExporter]

FYI, I've applied the alternative patch to sisu-guice:

   https://github.com/sonatype/sisu-guice/commit/f2aae880bb8f758d16108705ab8fe7da4769a93d

so people can try it out with the latest snapshot:

   https://repository.sonatype.org/content/groups/forge/org/sonatype/sisu/sisu-guice/3.2.0-SNAPSHOT/

Original comment by mccu...@gmail.com on 28 Oct 2011 at 1:00

[GoogleCodeExporter]

fixed in r1c9b92a5d869.  thanks for the patches everyone!

Original comment by sberlin on 21 Jan 2012 at 5:57

• Changed state: Fixed