FinalizableReferenceQueue still leaks

[GoogleCodeExporter]

Issue 227 is closed but the problem was not really fixed.

Stuart's comment #8 explains what remains to be fixed:
http://code.google.com/p/google-guice/issues/detail?id=227&can=1&start=200#c8

Original issue reported on code.google.com by gili.tza...@gmail.com on 25 Dec 2008 at 5:51

[GoogleCodeExporter]

Let me, explain...

Initialize Executor on injector instance

  Injector injector = Guice.createInjector(myModule, myExecutor);

You can initialize Executor on Injector constructor => new Injector(myExecutor)
or init method after constructor call => new Injector().init(myExecutor)

Destroying, with non static method (using "previous" instance of injector)

  injector.destroy();

Original comment by jose.ill...@gmail.com on 28 Jan 2011 at 3:31

[GoogleCodeExporter]

How early do we need to have the instance of an executor?

Thread started here:
http://code.google.com/p/google-guice/source/browse/trunk/src/com/google/inject/
internal/Finalizer.java?r=830#82
Which is only invoked by:
http://code.google.com/p/google-guice/source/browse/trunk/src/com/google/inject/
internal/FinalizableReferenceQueue.java?r=830#117
Which is only invoked by:
http://code.google.com/p/google-guice/source/browse/trunk/src/com/google/inject/
internal/MapMaker.java?r=830#773
Which is only invoked when the private static class MapMaker.QueueHolder is 
accessed at these locations:
1) 
http://code.google.com/p/google-guice/source/browse/trunk/src/com/google/inject/
internal/MapMaker.java?r=830#873
2) 
http://code.google.com/p/google-guice/source/browse/trunk/src/com/google/inject/
internal/MapMaker.java?r=830#931
3) 
http://code.google.com/p/google-guice/source/browse/trunk/src/com/google/inject/
internal/MapMaker.java?r=830#989
4) 
http://code.google.com/p/google-guice/source/browse/trunk/src/com/google/inject/
internal/MapMaker.java?r=830#1014
Which would occur when MapMaker creates  soft/weak keys or soft/weak 
references. (Does it create any of those before entries are added?)

We need the executor when we obtain the ReferenceQueue for 
FinalizerReferenceQueue (frq).

We *should* have the ability to access the executor instance before MapMaker 
*creation*.
We *must* have the ability to access the executor instance before MapMaker 
*addition*. (unless I'm mistaken)

For a Guice application, when is the first MapMaker instance _created_ as well 
as the first _addition_ to it? I'm running out of time for this research, so I 
need help here.

Original comment by tj.rothw...@gmail.com on 28 Jan 2011 at 4:50

[GoogleCodeExporter]

FYI, I'll be submitting a change to MapMaker (hopefully today) which will 
simply remove its dependence on FinalizableReferenceQueue.

Original comment by fry@google.com on 28 Jan 2011 at 4:53

[GoogleCodeExporter]

BTW, one more thing, the QueueHolder static init is what limits MapMaker to a 
single frq instance.

Original comment by tj.rothw...@gmail.com on 28 Jan 2011 at 4:56

[GoogleCodeExporter]

After we validate fry's change is stable (and after I get internet at home), 
I'll be updating the embedded guava code in guice. 

Original comment by sa...@google.com on 28 Jan 2011 at 5:00

[GoogleCodeExporter]

Well, soon enough there will be zero frq instances.

Original comment by fry@google.com on 28 Jan 2011 at 5:00

[GoogleCodeExporter]

Hi Fry,

Does your fix also address issue 488 ?

Original comment by nimbus4...@gmail.com on 8 Feb 2011 at 6:27

[GoogleCodeExporter]

yes.

Original comment by fry@google.com on 8 Feb 2011 at 6:54

[GoogleCodeExporter]

Original comment by sberlin on 22 Feb 2011 at 1:52

• Removed labels: Milestone-Release2.0

[GoogleCodeExporter]

Hi, is there any fix available? I'm using Guice 3.0 rc2 (Jan 09) and have the 
same issue.

I get the following messages:

1x
The web application [/xxx] appears to have started a thread named 
[com.google.inject.internal.util.$Finalizer] but has failed to stop it. This is 
very likely to create a memory leak.

mutiple times:
The web application [/xxx] created a ThreadLocal with key of type 
[com.google.inject.internal.InjectorImpl$1] (value 
[com.google.inject.internal.InjectorImpl$1@4604a96a]) and a value of type 
[java.lang.Object[]] (value [[Ljava.lang.Object;@40974600]) but failed to 
remove it when the web application was stopped. Threads are going to be renewed 
over time to try and avoid a probable memory leak. 

Original comment by larrrs.j...@googlemail.com on 6 Mar 2011 at 2:51

[GoogleCodeExporter]

No fix yet. This issue will be updated when there is. :-)

Original comment by fry@google.com on 6 Mar 2011 at 3:55

[GoogleCodeExporter]

okay, thx for the reply :)

Original comment by larrrs.j...@googlemail.com on 6 Mar 2011 at 3:58

[GoogleCodeExporter]

Hi Fry,
I couldn't see the fix in the recent Guice 3 RC3 release - is it still going to 
make it into the GA release ?
thanks.

Original comment by nimbus4...@gmail.com on 7 Mar 2011 at 10:07

[GoogleCodeExporter]

There is no fix yet. This issue will be updated when there is. :-/

Original comment by fry@google.com on 7 Mar 2011 at 10:10

[GoogleCodeExporter]

It is unlikely the fix will be in Guice 3.0.  We'll update the Guice source 
with the latest Guava bits when it's available.

Original comment by sberlin on 7 Mar 2011 at 10:30

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

Are there any quick and dirty workarounds for this atm? I'm really in a bind 
here :(

Original comment by jennifer...@gmail.com on 28 Mar 2011 at 1:58

[GoogleCodeExporter]

One quick and dirty workaround would be to use the sisu-guice binary:

   http://repo1.maven.org/maven2/org/sonatype/sisu/sisu-guice/3.0.0/

This is guice 3.0 plus some patches 
(https://github.com/sonatype/sisu-guice/blob/master/PATCHES).

One of these changes is to add a property "-Dguice.executor.class=Clazz" where 
Clazz implements java.util.concurrent.Executor. You can then supply your own 
executor to manage the finalizer work. You can also use 
"-Dguice.executor.class=NONE" to turn off the finalizer completely, but note 
that this means cleanup will only happen occasionally when the weak collections 
mutate.

HTH  (sorry about the use of a property, I didn't want to introduce an 
experimental API that wasn't in Guice)

Original comment by mccu...@gmail.com on 28 Mar 2011 at 4:51

[GoogleCodeExporter]

As another workaround that doesn't require to patch guice sources, I've started 
to work on a Tomcat Listener that try to shutdown the thread when the context 
is destroyed. It also tries to cancel the Expiration Timer launched by MapMaker.

It looks to work correctly (with both guice 2.0 and 3.0) but I've not tested it 
in production yet. The code can easily be ported to other applications thant 
tomcat.

The Listener sources are in attachment (note that I removed the package name 
from the sources). Maybe it can be a starting point to be integrated in guice 
or tomcat, or in a "guice-utils" jar.

Original comment by bontecy...@gmail.com on 31 Mar 2011 at 11:56

Attachments:

• GuiceLifecycleListener.java

[GoogleCodeExporter]

@bontecy, I test your GuiceLifecycleListener on my Tomcat (development 
environment) without any Permgen errors on redeploys... GREAT!!

Original comment by jose.ill...@gmail.com on 9 May 2011 at 9:37

[GoogleCodeExporter]

Ok, great. Note that I found a conflict with this workaround when a second 
context provides the jars but don't use them : the listener then initializes 
the thread for that context while it tries to stop the thread of the first 
context. Also, the loops are not optimal.
I didn't find time to fix this for now but I'll try to address this if no 
solution comes soon in guice (Then, I'll probably create a repository outside 
of this bug report).

Original comment by bontecy...@gmail.com on 9 May 2011 at 12:59

[GoogleCodeExporter]

The fix for this has finally landed in Guava, and will be deployed in release 
10.

Original comment by fry@google.com on 13 Jun 2011 at 2:11

[GoogleCodeExporter]

Will there be a new Guice release containing this fix?

Original comment by henrycha...@gmail.com on 13 Jul 2011 at 1:09

[GoogleCodeExporter]

Now guava (10) fixed this issue. Any plan/roadmap to include this fix on guice?

Original comment by jose.ill...@gmail.com on 29 Sep 2011 at 11:14

[GoogleCodeExporter]

Any word on a fix? Nothing mentioned so far works for me..

Original comment by colin.ta...@gmail.com on 3 Oct 2011 at 1:12

[GoogleCodeExporter]

You could always try sisu-guice 3.1.0 (which is guice + a few experimental 
patches, such as the guava update)

   http://search.maven.org/#artifactdetails%7Corg.sonatype.sisu%7Csisu-guice%7C3.1.0%7Cjar

Note that this depends on sisu-guava 0.9.9 - which was based on pre-10 build of 
guava with the finalizer thread fix, but you can always substitute this with 
guava 10 now that's been released. Also unlike previous releases this 
particular flavour of guice doesn't embed/jarjar guava code, so you do need 
both on the classpath.

Original comment by mccu...@gmail.com on 3 Oct 2011 at 1:36

[GoogleCodeExporter]

Minor patch to update Guava dependency to 10.0

Original comment by mccu...@gmail.com on 7 Oct 2011 at 10:49

Attachments:

• update_to_guava_10.patch
• guava-10.0.jar

[GoogleCodeExporter]

Great work mcculls,

 But i like some plan/roadmap to include this fix on "original" guice?

For example: a "official" guice 3.1 release to resolve all Accepted issues with 
Hight priority must be one ideal solution...

Original comment by jose.ill...@gmail.com on 26 Oct 2011 at 11:03

[GoogleCodeExporter]

Issue 630 has been merged into this issue.

Original comment by mccu...@gmail.com on 10 Nov 2011 at 7:25

[GoogleCodeExporter]

@mcculls,

I try your patch with guava-10.0 but tomcat persist on:

[16] SEVE 18:16:32 WebappClassLoader.checkThreadLocalMapForLeaks: The web 
application [/ulysses] created a ThreadLocal with key of type 
[com.google.inject.internal.InjectorImpl$1] (value 
[com.google.inject.internal.InjectorImpl$1@125ec471]) and a value of type 
[java.lang.Object[]] (value [[Ljava.lang.Object;@bb2e023]) but failed to remove 
it when the web application was stopped. Threads are going to be renewed over 
time to try and avoid a probable memory leak. 
[16] SEVE 18:16:32 WebappClassLoader.checkThreadLocalMapForLeaks: The web 
application [/ulysses] created a ThreadLocal with key of type 
[com.google.inject.internal.InjectorImpl$1] (value 
[com.google.inject.internal.InjectorImpl$1@125ec471]) and a value of type 
[java.lang.Object[]] (value [[Ljava.lang.Object;@c991fd5]) but failed to remove 
it when the web application was stopped. Threads are going to be renewed over 
time to try and avoid a probable memory leak.

Any fix/idea?

Original comment by jose.ill...@gmail.com on 11 Nov 2011 at 5:20

[GoogleCodeExporter]

Hi,
I resolved the issue for me by compiling sisu-guice (master) with the guava 
10.0.1 dependency (instead of the sisu-guava 0.9.9).
See here: 
https://github.com/sas101/sisu-guice/commit/2bc6e1554d29faf95df3cb959d258456ceaf
060a

Thx @mcculls for providing that suggestion.

Original comment by stefan.simroth on 11 Nov 2011 at 5:27

[GoogleCodeExporter]

As explained above the remaining ThreadLocal of type Object[1] seen by Tomcat 
is not a memory leak and would eventually get cleared. However, even if it 
wasn't cleared it only takes up a few bytes and would not hold onto any user 
level classloaders, which is the cause of most leaks. This is because the 
single element in that array is guaranteed to be null'd out in a finally clause 
and the component type of the array is Object.

Note: the reason the ThreadLocal is left between injector calls and not removed 
on every call is for performance reasons. Explicitly removing a ThreadLocal is 
not mandated as shown in the example from the official javadocs 
http://download.oracle.com/javase/6/docs/api/java/lang/ThreadLocal.html because 
it should automatically get cleared when the ThreadLocal is no longer 
accessible. Unfortunately the JDK is slow in clearing out stale entries, which 
coupled with the (overly) zealous checks in Tomcat can lead to these spurious 
error messages (which should really be warnings imho).

Original comment by mccu...@gmail.com on 11 Nov 2011 at 5:34

[GoogleCodeExporter]

Using Guice 3.0 from central maven repo I am still getting this:

2012.07.12. 9:00:18 org.apache.catalina.loader.WebappClassLoader 
clearReferencesThreads
SEVERE: The web application [/foo-bar-1.0-SNAPSHOT] appears to have started a 
thread named [com.google.inject.internal.util.$Finalizer] but has failed to 
stop it. This is very likely to create a memory leak.

Any news on this? I don't see guava dependency in the pom, but I've read it 
will be fixed once it is upgraded. Thanks!

Original comment by bal...@krivan.info on 12 Jul 2012 at 7:10

[GoogleCodeExporter]

Note that Guice 3.0 still embeds an old version of Guava (which is why the pom 
has no explicit dependency to it) so that message is expected.

Original comment by mccu...@gmail.com on 20 Jul 2012 at 6:11

[GoogleCodeExporter]

OMG, we are getting close to 4 years old for this one... way to go Google!

Original comment by pas...@gmail.com on 20 Jul 2012 at 6:17

[GoogleCodeExporter]

PS. you you can always use sisu-guice (which doesn't embed guava, but instead 
has it as a dependency)

   http://search.maven.org/#artifactdetails%7Corg.sonatype.sisu%7Csisu-guice%7C3.1.1%7Cjar

Or you could compile from source - also note in many scenarios the finalizer 
thread is not actually an issue.

Original comment by mccu...@gmail.com on 20 Jul 2012 at 6:23

[GoogleCodeExporter]

This definitely needs to be fixed.

Original comment by s.a.grigoriev on 28 Jul 2012 at 2:51

[GoogleCodeExporter]

Here's an additional patch that clears up the last of the Tomcat warnings after 
upgrading to the latest Guava (the ones referring to the injectorImpl's 
ThreadLocal). The issue here was the injector used an anonymous class to define 
a custom initialValue method for its localContext ThreadLocal. Unfortunately 
this anonymous class has an implicit reference (seen as this$0 in the bytecode) 
back to the instance where it was created, ie. the injector. We could change 
this to be a static nested class which would avoid the reference to the 
injector, but this still keeps a reference back to the containing classloader 
which upsets Tomcat's strict checks. This patch removes the custom ThreadLocal 
subclass and adds a check+set in the callInContext method - note this is 
slightly less performant, since it needs an extra call to set(). It's 
questionable whether this performance hit is worth getting rid of all the 
Tomcat warnings, switching to a static nested class may be enough since you'd 
still occasionally see a warning but the only thing that a rogue thread local 
might be holding onto would be the classloader. Anyway, here's the patch in 
case anyone is interested in trying it out with trunk...

Original comment by mccu...@gmail.com on 7 Aug 2012 at 1:25

Attachments:

• GUICE_288_decouple_thread_local.patch

[GoogleCodeExporter]

Issue 698 has been merged into this issue.

Original comment by mccu...@gmail.com on 7 Aug 2012 at 7:00

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

I tried the last version from trunk + the patch from Stuart McCulloch, and it 
works now perfectly.
This patch should be integrated to guice.

Thanks

Original comment by nicolas....@gmail.com on 24 Aug 2012 at 9:01

[GoogleCodeExporter]

Issue 698 has been merged into this issue.

Original comment by mccu...@gmail.com on 2 Sep 2012 at 10:32

[GoogleCodeExporter]

If the proposed decoupling thread local patch is the fix we need and want, then 
wouldn't it be more conservative to declare a static concrete inner class for 
the LocalContext ? We would not need to change the lifecycle of 
InjectorImpl.localContext.

Original comment by loren...@gmail.com on 21 Jan 2013 at 3:57

Attachments:

• GUICE_288_static_concrete_class.patch

[GoogleCodeExporter]

As mentioned in #c88 using a static nested class would still leave an indirect 
reference to the guice classloader which would be picked up by Tomcat's memory 
leak detector. So if you wanted to remove all warnings then the patch in #c88 
is the best I've found so far.

Original comment by mccu...@gmail.com on 27 Feb 2013 at 11:12

[GoogleCodeExporter]

So this bug has not been fixed in the Guice 4.0 beta, correct?  So is it 
correct to assume that it won't make it into 4.0 GA?  Bummer.

Original comment by r...@asleson.net on 16 Oct 2013 at 12:35

[GoogleCodeExporter]

Guice 4.0-beta embeds Guava 11.0.1, which includes the 
FinalizableReferenceQueue fix. You may still see a warning in Tomcat about a 
ThreadLocal (see comment #c88 and associated patch) but this is relatively 
minor compared to the original issue.

Original comment by mccu...@gmail.com on 16 Oct 2013 at 1:00

[GoogleCodeExporter]

All of this could have been solved 6 (!!) years ago by simply adding a close() 
method. See comment #3.

Add a close() method now and remove it in the future if you ever get an 
alternative working.

Original comment by cow...@bbs.darktech.org on 16 Oct 2013 at 3:10

[GoogleCodeExporter]

There's no need for a close() method now - the weak/soft maps were 
re-implemented in Guava 10 to avoid the need for the FRQ background thread, so 
there is literally nothing to close: 
http://code.google.com/p/guava-libraries/source/detail?r=a13e02167e90125e6a78bf9
bbd061996d05a143a. The FRQ fix is available in Guice 4.0-beta for testing.

Original comment by mccu...@gmail.com on 16 Oct 2013 at 3:21

[GoogleCodeExporter]

Issue 786 has been merged into this issue.

Original comment by mccu...@gmail.com on 2 Dec 2013 at 1:23

[GoogleCodeExporter]

Issue 707 has been merged into this issue.

Original comment by sberlin on 5 Dec 2013 at 11:51