Original comment posted by kevinb9n on 2008-07-28 at 07:00 PM

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.

Owner: crazyboblee

Original comment posted by nairb774 on 2008-07-28 at 07:43 PM

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 posted by mcculls on 2008-11-26 at 05:24 PM

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 posted by gili.tzabari on 2008-11-27 at 02:31 AM

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 posted by mcculls on 2008-11-27 at 03:01 AM

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 posted by mcculls on 2008-11-28 at 03:14 PM

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

1. 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 :(

1. 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 :(

1. 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 posted by crazyboblee on 2008-11-30 at 02:38 AM

Nice detective work, mcculls!

Original comment posted by mcculls on 2008-12-02 at 09:55 AM

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 posted by crazyboblee on 2009-03-17 at 09:48 PM

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 posted by kevinb9n on 2009-03-18 at 02:21 AM

(No comment entered for this change.)

Labels: 1.0

Original comment posted by kevinb9n on 2009-09-17 at 05:45 PM

(No comment entered for this change.)

Labels: Milestone-1.0

Original comment posted by kevinb9n on 2009-09-17 at 05:46 PM

(No comment entered for this change.)

Labels: -1.0

Original comment posted by kevinb9n on 2009-10-16 at 09:01 PM

Seems this won't be in 1.0 after all.

Labels: -Milestone-1.0, Milestone-Post1.0

Original comment posted by syva...@yahoo.com on 2009-10-26 at 02:06 PM

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 posted by alen_vre...@yahoo.com on 2009-11-06 at 06:13 PM

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 posted by pseudonymorama on 2009-11-12 at 12:26 AM

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 posted by terciofilho on 2010-04-11 at 09:49 PM

Any news on this issue?

Original comment posted by kevinb@google.com on 2010-04-12 at 11:43 PM

No. Bob was going to do it but... then he didn't. Bob?

Original comment posted by pawjanssen on 2010-05-29 at 09:48 PM

Today I received a PermGen error from my beloved Tomcat, for wich the
JreMemoryLeakPreventionListener logged the error message "SEVERE: A web application
appears to have started a thread named [com.google.inject.internal.Finalizer] but has
failed to stop it. This is very likely to create a memory leak.".

After some research, I figured this has to do with the issue addressed here, so I'm
wondering, is it about to be fixed, or is there some API to close the thread started by
Guice?

Original comment posted by kevinb@google.com on 2010-07-30 at 03:53 AM

(No comment entered for this change.)

Labels: -Milestone-Post1.0

Original comment posted by kevinb@google.com on 2010-07-30 at 03:56 AM

(No comment entered for this change.)

Labels: -Priority-Medium

Original comment posted by kevinb@google.com on 2010-09-08 at 07:00 PM

Issue #382 has been merged into this issue.

Original comment posted by terciofilho on 2010-09-09 at 08:14 PM

No news about this issue? Does anybody have a workaround?

Original comment posted by kevinb@google.com on 2010-09-10 at 06:36 AM

I talked to Bob about it today. We're considering reading a system property -- shocking, I know -- that would tell MapMaker not to start any background thread (but perform cleanup in the user thread). Using that would make this problem go away for you.

Status: Accepted
Owner: kev...@google.com

Original comment posted by syva...@yahoo.com on 2010-09-10 at 06:57 AM

I might be missing something, but couldn't you just add a shutdown method?

The problem is most severe in app servers and often you can't add system properties there, so the workaround wouldn't be very widely applicable.

You could just document the shutdown method as unsupported and not part of public API. Perhaps even advice that it's existence should be checked by reflection (and thus also called by reflection).

Original comment posted by holger.hoffstaette on 2010-09-10 at 09:08 AM

The suggested solution of using a System property is unfortunately useless for a vast number of deployment scenarios and does nothing to fix the problem. +1 for just not creating a Finalizer thread in a library and instead piggybacking expiry on user-level operations every <hopefully customizable n> operations instead.

Here's another idea that builds on this. How about requiring a user-supplied Executor in which to run instead, as additional argument to the expiration() builder method or the public FinalizerQueue class? This decouples enabled expiry from having to create a physical thread at all, and allows proper lifecycle control by the caller, instead of forcing the library to fly blind and make guesses about what to do. The caller then is not only responsible, but also now actually in a position (!) to do environment-specific lifecycle control: quiesced waiting, forced shutdown, Bundle.stop(), ServletContextListener, whatever.

While we're at it, the same Executor could also be used to invoke the listener, if added by the builder.

One new caveat would be that a user can now share an Executor between multiple CCHMs. If the Executor had really only one or any other fixed number of threads, then too many continuously running-and-blocking-without-timeout Finalizer.run() in their current form might start clogging the Executor. If reference cleaning were batched per CCHM, the run() loop could terminate after doing whatever is necessary.

As far as I can tell this solves the Finalizer thread lifecycle problem (caused really by the nondeterministic behaviour of its ref queue polling and therefore non-termination) and also the thread-per-CCHM overhead (n CCHMs use n Finalizer threads for no really good reason).

Original comment posted by terciofilho on 2010-09-10 at 01:00 PM

+1 for a shutdown method, this is much better as in my deployment scenario I already have a shutdown for my stuffs and I think that the ContextDestroyed could do this job.

Original comment posted by kevinb@google.com on 2010-09-10 at 01:48 PM

I'm sorry, if it's true that our intended solution is "useless" and "does nothing to fix the problem" would it be too much trouble to explain why that is?

Original comment posted by terciofilho on 2010-09-10 at 02:13 PM

Well, I don't think that this solution is useless nor does nothing to fix the problem, I just think that isn't the best way to solve it, just that.

Maybe people upon can explain their reasons about that...

Original comment posted by archie.cobbs on 2012-05-17 at 08:05 PM

Sorry this report is not before the 12.0 release, but I am still seeing this problem in 12.0:

  May 17, 2012 3:02:54 PM org.apache.catalina.loader.WebappClassLoader clearReferencesThreads
  SEVERE: The web application [/demo3] appears to have started a thread named [com.google.common.base.internal.Finalizer] but has failed to stop it. This is very likely to create a memory leak.

I'm using tomcat 7.0.21.

Original comment posted by jens.nehlmeier on 2012-05-23 at 04:14 PM

We are using Jetty 8.1.3 along with BoneCP which requires Guava. Even with Guava 12.0 the Finalizer thread does not stop running and prevents the garbage collection of the WebAppClassloader.

Original comment posted by fry@google.com on 2012-05-23 at 06:41 PM

(No comment entered for this change.)

Status: Accepted
Owner: emcmanus@google.com

Original comment posted by techsy730 on 2012-07-09 at 06:42 PM

I can confirm this in Guava 12.0 on Sun's JDK 1.7.0_04 on a Windows 7 box.

I think I have traced this down to the thread that runs the com.google.common.base.internal.Finalizer.

Despite great pains to make sure that the Finalizer and its spawned thread use independant classloaders from the app's classloaders , it's contextClassLoader is still holding a reference to the module's classloader.

Original comment posted by emcmanus@google.com on 2012-07-10 at 08:04 PM

I've spent some more time investigating what happens with Tomcat specifically. If the FinalizableReferenceQueue is in a static field of some class in a webapp, then indeed the Finalizer thread never dies and the ClassLoader of the webapp is never garbage-collected. The reason is that the separate ClassLoader that is used to load the Finalizer class contains a reference to the original webapp ClassLoader in its saved AccessControlContext. That ClassLoader then keeps all the static fields of all of its classes alive, and in particular the FRQ. The Finalizer thread is set up to die as soon as its FRQ is garbage-collected but that never happens.

This can be fixed as previously suggested, by bashing the private URLClassLoader.acc field to null, but I think the cure is worse than the disease, and in particular could open security holes in environments with untrusted code.

In my tests, it is enough for the webapp just to set its static FRQ field to null and then the Finalizer thread will die when the no-longer-referenced FRQ is collected, and the webapp ClassLoader becomes collectible at that point. (In Tomcat, you can still get the alarming log message about the Finalizer thread still being present, but that is only because the FRQ hasn't been collected yet.)

I would be interested to know whether people who have run into this problem are able to work around it in a similar way.

I think we should probably add, as suggested earlier, a FinalizableReferenceQueue.close() instance method that explicitly stops the Finalizer thread, along with a test that this is enough to make the ClassLoader collectible.

Original comment posted by archie.cobbs on 2012-07-10 at 08:14 PM

Hmm... in my case, I don't think I have any such static fields, at least none that directly reference a FRQ. There may be some indirect reference being created somehow.

FYI, here are all the guava classes that I'm explicitly importing:

import com.google.common.base.Function;
import com.google.common.base.Predicate;
import com.google.common.base.Predicates;
import com.google.common.cache.Cache;
import com.google.common.cache.CacheBuilder;
import com.google.common.cache.CacheLoader;
import com.google.common.cache.LoadingCache;
import com.google.common.collect.BiMap;
import com.google.common.collect.Collections2;
import com.google.common.collect.DiscreteDomains;
import com.google.common.collect.ImmutableBiMap;
import com.google.common.collect.Iterators;
import com.google.common.collect.MapMaker;
import com.google.common.collect.Ranges;
import com.google.common.util.concurrent.UncheckedExecutionException;

Also, I'm using BoneCP as well.. perhaps that is where the reference originates.

Original comment posted by cow...@bbs.darktech.org on 2012-07-10 at 08:15 PM

Consider how much time has been wasted on this issue (by developers and end-users) when we could have just added a close() method to begin with. Please end the pain :)

Original comment posted by techsy730 on 2012-07-10 at 08:22 PM

I worked around it by making my static reference to the FinalizibleReferenceQueue a weak reference. Then when I create each of my FinalizibleReferences, I have them keep a strong reference to the FinalizibleReferenceQueue that they are registerered with. When finalizeReferent() is called, that reference is nulled out. Thus, once all finalizible references are finalized, the FinalizibleReferenceQueue only has a weak reference, thus can be GCed, and thus the thread stopped.

I also have a method to fetch the FinalizibleReferenceQueue, where if the previous reference one collected (and thus, the reference cleared), it will create a new FinalizibleReferenceQueue, give that to the newly created FinalizibleReference, and then create a new WeakReference to that FinalizibleReferenceQueue and update my static reference to that. Moderately elegant. ;)

Something like that could probably be moved to inside the FinalizibleReferenceQueue (which the Finalizer thread will "listen" for its collection, as opposed to the FinalizibleReferenceQueue itself like it is now), and have each of the three FinalizibleReference reference implementations keep a hard reference, that is cleared upon the Finalizer "finalizing" it.

Original comment posted by emcmanus@google.com on 2012-07-10 at 09:05 PM

The suggestion from techsy730 is very interesting. The idea would be that the Finalizer thread would die, not when the FRQ is no longer referenced as today, but when there are no longer any FinalizableWeakReferences (etc) that reference the FRQ. Perhaps with some amount of hysteresis to avoid continually creating and destroying Finalizer threads (we would get that more or less for free using ThreadPoolExecutor). Making this work in a race-proof way could be challenging but I think the end result would be substantially better than what we have today.

Original comment posted by techsy730 on 2012-07-10 at 09:29 PM

Using a SoftReference instead of a WeakReference for the "handle" in the FinalizibleReferenceQueue may help with excessive amounts of stopping the thread just to have to create a new one again, but it carries its own risks. According to the API, a perfectly valid implementation of SoftReference would be to not clear it at all until the absolute latest the API mandates it be considered, in the final "Last ditch" GC before a OutOfMemoryError is thrown. Of course no sane JVM implementation will always wait that long (most use some sort of "timeout" after the last reference was used), but we have to consider that possibility.

Maybe a better idea would be an "expiring" reference. An object that holds a strong and a weak reference until time X passes, after which is nulls out the strong reference and only holds the weak reference. In fact, that would be a pretty nifty wrapper for existing references. (Contemplates making a new feature suggestion)

As for the thread safety, I did it by just having a private static lock in the class holding the static WeakReference, and all fetches and initilizations of that WeakReference were synchronized on that. For each of my FinalizibleReferences, I made the field holding the strong reference volatile. I'm pretty reasonably sure this covers all angles.

Now, since in my workaround, I am creating whole new FinalizibleReferenceQueue's, I don't have to care about the thread safety of spawning the new thread using the fancy ClassLoader and reflection magic that is done, as it's a whole new instance. If this sort of reinitializable weak handle approach was brought into FinalizibleReferenceQueue, then the fancy Java magic would have to be made thread safe. But I agree that it would be worth it.

(Actually, a reinitializable weak reference and a reinitializable soft reference are also cool idea, maybe another for the feature suggestions)

Original comment posted by emcmanus@google.com on 2012-08-01 at 03:15 PM

I've added a close() method for now (and made FRQ implement Closeable). Even if we later add a mechanism that closes automatically when there are no FinalizableReferences left, the close() method will still be useful for when it is not practical to dispose of all the referenced objects explicitly.

Original comment posted by jens.nehlmeier on 2012-08-01 at 03:52 PM

So does this mean that every library that uses Guava's FinalizableReferenceQueue has to expose a close() method itself because the library may not know that it is used inside a web application and have no notion about redeployments?
For example would the close() method help to fix Google Guice's issue #288 (http://code.google.com/p/google-guice/issues/detail?id=288) which is pretty much the same as this one?

Original comment posted by archie.cobbs on 2012-08-01 at 03:57 PM

To address a WAR memory leak caused by this bug, just add a ServletContextListener that uses reflection to invoke the close() method (if found by reflection).

See Spring's IntrospectorCleanupListener for example of this idea applied to the Introspector class: http://static.springsource.org/spring/docs/3.1.x/javadoc-api/org/springframework/web/util/IntrospectorCleanupListener.html

Original comment posted by mcculls on 2012-08-01 at 04:18 PM

Wrt. Guice issue 288 the only fix required is to update it to use Guava 10.0 or later, which is when the weak/soft maps created by MapMaker stopped using the FinalizableReferenceQueue. The change to use a more recent build of Guava has already been committed to Guice trunk (it's currently using 11.0.1) and will be in the next release.

Original comment posted by emcmanus@google.com on 2012-08-01 at 04:41 PM

You don't need to use reflection to invoke the close() method even if you are concerned about compatibility with earlier Guava versions. Just write
  if (frq instanceof Closeable) {
    ((Closeable) frq).close();
  }

Original comment posted by jerith666 on 2013-08-16 at 05:57 PM

issue 1505 may or may not be relevant here

Original comment posted by emcmanus@google.com on 2013-11-20 at 11:23 PM

Marking this as Fixed since it is possible to avoid the problem by calling close() at the appropriate time. We do not currently plan to implement a mechanism that closes automatically when there are no FinalizableReferences left.

Status: Fixed

Original comment posted by Karsten.Ohme on 2014-03-28 at 02:33 PM

For Guava 13.0.1 (which is a requirement for BoneCP) I had to modify the fix from comment #15:

http://pastie.org/8976099
Insteas of thread.getClass().getName(); I had to use thread.getName();

Also important is, that this must not be proxied by aspectj. The method was not invoke here if this was the case.

Original comment posted by cow...@bbs.darktech.org on 2014-03-28 at 02:37 PM

I suggest opening a new bug report that references this one because most committers will ignore comments on closed issues.

Original comment posted by emcmanus@google.com on 2014-03-28 at 04:43 PM

I believe comment #110 is a note about a workaround that is needed if you have to use an old version of Guava, correct? Obviously we can't modify old Guava versions retroactively. If there is something that needs to be done to the current version then by all means open a new issue.