TimurMahammadov / google-collections

Automatically exported from code.google.com/p/google-collections
Apache License 2.0
0 stars 0 forks source link

FinalizableReferenceQueue keeps ClassLoaders around. #92

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
If the class loader which loaded the google-collections jar is let go
(consider redeploying a war to Apache Geronimo) the class loader will not
be able to be garbage collected.  This is caused by the thread started by
FinalizableReferenceQueue never ending.  In other words the garbage
collection (for the class loader) can only happen when the thread stops. 
The solution is to stop the thread when there are no more objects on the
queue.  This will happen eventually if the rest of the app behaves
correctly by not placing state outside of its class loader's environment. 
Attached is a version of the class which I have used to this effect.

This is also a problem in the OSGi space where every jar gets its own class
loader and they can be refreshed easily at runtime to do rolling
upgrades/bug fixes.

Original issue reported on code.google.com by nairb...@gmail.com on 28 Jul 2008 at 5:39

Attachments:

GoogleCodeExporter commented 9 years ago
Thanks very much!  Bob has agreed to fix this issue.  He's decided not to use 
your
patch, and will fix it a slightly different way, but thanks for going to the 
extra
effort anyway.

Original comment by kevin...@gmail.com on 28 Jul 2008 at 7:00

GoogleCodeExporter commented 9 years ago
A fix is a fix is a fix... :)  There is always more then one way - I just took 
the
fastest and least complicated way I could think of.

Thanks.

Original comment by nairb...@gmail.com on 28 Jul 2008 at 7:43

GoogleCodeExporter commented 9 years ago
FYI: I just tested the new version of FinalizableReferenceQueue with Guice on 
an OSGi
container, and I can confirm it now allows the classloader to be garbage 
collected :)

Original comment by mccu...@gmail.com on 26 Nov 2008 at 5:24

GoogleCodeExporter commented 9 years ago
This isn't solved for me (under Glassfish) but I'm not 100% sure I am 
interpreting
the results correctly. See
http://groups.google.com/group/google-guice/msg/ff53f95f422edabf

Original comment by gili.tza...@gmail.com on 27 Nov 2008 at 2:31

GoogleCodeExporter commented 9 years ago
I'd need to know more about the way you're loading Guice into the classloader
hierarchy to comment, and also if you're seeing any warnings or exceptions 
relating
to the Finalizer thread. What I can say is that it does unload OK under OSGi, 
which
suggests that something else is keeping the classloader alive in your case.

Note that the Finalizer thread won't shutdown until the Guice classloader is 
eligible
for GC, so it will appear as a GC root until that happens - you may think it 
looks
like a leak, but actually it will clear itself once other references to the 
Guice
classloader are cleared. So I'd dig deeper into the heapdump for other causes.

You may also need to force finalization and GC a few times using the System API.

Original comment by mccu...@gmail.com on 27 Nov 2008 at 3:01

GoogleCodeExporter commented 9 years ago
I did some digging and found the problem - when running under JEE it seems 
there are
several additional references from the Finalizer thread to the container 
classloader
via access control contexts (presumably because of additional protection 
domains).

Here's the complete list of references I found and some possible solutions:

1. Finalizer -> Thread.inheritedAccessControlContext -> ... -> Web CL

   Set in Thread constructor to 'AccessController.getContext()' ie. the
   context of the calling thread, this may refer to the Web classloader.

   Solution: wrap creation of Finalizer thread in doPrivileged block

2. URLClassLoader -> URLClassLoader.acc -> ... -> Web CL

   As above URLClassLoader constructor sets 'acc' field to current context.

   Solution: doPrivileged block doesn't help here, the only way I found to
   avoid this reference was to use reflection to null the 'acc' field which
   is yucky, anyone know a better option? Perhaps we could grab defineClass
   from the system classloader (again with reflection) and use that to load
   the Finalizer into the system classloader, but this is also yucky :(

3. Finalizer -> Thread.inheritableThreadLocals -> ... -> Web CL

   Each thread inherits an inheritableThreadLocals map from its parent, and
   this could include a reference back to the container classloader (depends
   on the app) - again no easy fix for this one, had to resort to reflection
   to null out the field :(

4. Finalizer -> Thread.contextClassLoader -> Web CL

   Yet another setting inherited from the calling thread, but at least here
   there's a public API 'setContextClassLoader()' we can use to clear it :)

With these references removed I can completely unload Guice from GlassFish v2.

I've attached a patch in case people are interested - but because of the hacky
use of reflection to clear certain private fields, I'm not too happy with it.

I'm wondering instead whether it would be better to provide a public API people
could use to shutdown the thread, which would then make a lot of this code moot.

Anyway, Happy Thanksgiving!

Original comment by mccu...@gmail.com on 28 Nov 2008 at 3:14

Attachments:

GoogleCodeExporter commented 9 years ago
Nice detective work, mcculls!

Original comment by crazybob...@gmail.com on 30 Nov 2008 at 2:38

GoogleCodeExporter commented 9 years ago
FYI here are the safe fixes that I think should be used in google-collections, 
such
as the use of doPrivileged() when creating the classloader and the Finalizer 
thread.
The TCCL should also be cleared, just in case.

The two other references (possible inherited thread-local and access control 
context)
are best fixed in the web container - certainly the thread-local issue is 
fixable in
GlassFish. However, still not sure about the remaining access control context 
issue.

Original comment by mccu...@gmail.com on 2 Dec 2008 at 9:55

Attachments:

GoogleCodeExporter commented 9 years ago
We'll address all of this in time for 1.0. I'm in favor of the reflection hacks 
over
an explicit shutdown hook.

Original comment by crazybob...@gmail.com on 17 Mar 2009 at 9:48

GoogleCodeExporter commented 9 years ago

Original comment by kevin...@gmail.com on 18 Mar 2009 at 2:21

GoogleCodeExporter commented 9 years ago

Original comment by kevin...@gmail.com on 17 Sep 2009 at 5:45

GoogleCodeExporter commented 9 years ago

Original comment by kevin...@gmail.com on 17 Sep 2009 at 5:46

GoogleCodeExporter commented 9 years ago
Seems this won't be in 1.0 after all.

Original comment by kevin...@gmail.com on 16 Oct 2009 at 9:01

GoogleCodeExporter commented 9 years ago
Could you please reconsider this? Resource leak like this leads to frequent
PermGenErrors and thus to app server restarts, causing developers to need to 
wait for
several minutes on each restart. Additionally, there are other redeployment 
issues at
least under Windows because of file-locking (jar files can't be deleted or 
moved).
Unfortunately there aren't any (usable) workarounds, so it would be really 
great if
this could be included in 1.0.

Original comment by syva...@yahoo.com on 26 Oct 2009 at 2:06

GoogleCodeExporter commented 9 years ago
As for workarounds: I've experimented with a ServletContextListener that finds 
any
Finalizer Threads and clears all the fields that Stuart mentioned.
http://pastie.org/686765 . I've tested for Guice Finalizer under Jetty and 
Tomcat and
it works.

For Servlet environments afaik ServletContextListener is the only place you're 
not
discouraged or explicitly forbidden to spawn threads. Therefore I am using a 
custom
Finalizer that creates just one Finalizer in SCL and makes sure it is disposed 
at the
end.

Original comment by alen_vre...@yahoo.com on 6 Nov 2009 at 6:13

GoogleCodeExporter commented 9 years ago
Will this cause a resource leak merely by including the jar on the classpath?  
Or 
will it only cause a problem if FinalizableReferenceQueue or classes using it 
(e.g. 
MapMaker) is used/referenced?  If the latter is the case, one could presumably 
work 
around the issue by making sure that the class isn't referenced.

Some sort of fix or workaround seems like a fairly critical thing to have, 
since I 
imagine many (most?) people who would want to use the google collections 
library are 
running under JEE.  The PermGen issue won't necessarily occur immediately as 
they 
may not redeploy right away, so users may not even know why PermGen errors 
suddenly 
started happening when they redeploy their war files.

Original comment by pseudony...@gmail.com on 12 Nov 2009 at 12:26

GoogleCodeExporter commented 9 years ago
This issue has been moved to the Guava project (keeping the same id number). 
Simply replace 'google-collections' with 'guava-libraries' in your address 
bar and it should take you there.

Original comment by kevinb@google.com on 5 Jan 2010 at 11:09