ClassLoader leaks in IndicesCallbackFilter

[gissuebot]

From adrianosf on July 22, 2011 09:51:48

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.

Attachment: gist
   guice-classloader-leak.diff

Original issue: http://code.google.com/p/google-guice/issues/detail?id=643

[gissuebot]

From mcculls on July 22, 2011 07:31:11

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.

[gissuebot]

From adrianosf on July 22, 2011 10:46:51

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.

Binary attachments: guice-classloader-leak-test.zip

[gissuebot]

From mcculls on July 22, 2011 10:49:35

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

[gissuebot]

From mcculls on July 22, 2011 11:30:24

(No comment was entered for this change.)

Owner: mcculls

[gissuebot]

From adrianosf on October 02, 2011 19:00:22

Is there any update on this?

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

[gissuebot]

From sberlin on October 02, 2011 19:54:05

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

[gissuebot]

From adrianosf on October 03, 2011 03:54:25

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

[gissuebot]

From mcculls on October 03, 2011 05:07:27

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.

[gissuebot]

From adrianosf on October 04, 2011 04:06:23

Patch updated accordingly to #8 comment.

Attachment: gist
   guice-classloader-leak-2.diff

[gissuebot]

From sberlin on October 07, 2011 06:29:56

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 ( https://code.google.com/p/google-guice/source/browse/trunk/core/test/com/googlecode/guice/BytecodeGenTest.java?r=1568#242 ).

[gissuebot]

From mcculls on October 27, 2011 16:08:55

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.

[gissuebot]

From mcculls on October 27, 2011 17:41:24

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.

Attachment: gist
   GUICE_ISSUE_643_20111028.patch

[gissuebot]

From mcculls on October 27, 2011 18:00:19

FYI, I've applied the alternative patch to sisu-guice: sonatype/sisu-guice@f2aae88 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/

[gissuebot]

From sberlin on January 21, 2012 09:57:03

fixed in r1c9b92a5d869 .  thanks for the patches everyone!

Status: Fixed