FinalizableReferenceQueue still leaks

[gissuebot]

From gili.tzabari on December 25, 2008 12:51:41

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

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

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

[gissuebot]

From limpbizkit on December 30, 2008 14:03:08

I've started working on a fix, but it might not be worthwhile... http://groups.google.com/group/google-guice-dev/browse_thread/thread/28caab16fc75ad25

[gissuebot]

From limpbizkit on December 30, 2008 15:39:59

(No comment was entered for this change.)

Labels: Priority-High Milestone-Release2.0

[gissuebot]

From gili.tzabari on December 30, 2008 16:10:12

1. Is there a way to use SoftReferences without a dedicated thread (perhaps similar
   to how WeakHashMap works)?
2. Another possibility (one that has been brought up before) is exposing an explicit
   method for shutting down the Thread. Developers would invoke this method from
   ServletContextListener.contextDestroyed()

[gissuebot]

From limpbizkit on December 30, 2008 21:03:52

1. yes
2. not necessary. The thread improved speed and memory usage by vacuuming out the collected elements. But
   it's not strictly necessary.

[gissuebot]

From mcculls on December 31, 2008 14:19:55

Any thoughts on the experimental concurrent reference map from JSR166? http://viewvc.jboss.org/cgi- bin/viewvc.cgi/jbosscache/experimental/jsr166/src/jsr166y/ConcurrentReferenceHashMap.
java?view=markup

it doesn't use a thread, but still supports the concurrent map API.

[gissuebot]

From gili.tzabari on December 31, 2008 14:32:44

Very cool! Doug Lea wrote the thing so I am willing to bet that it's pretty solid ;)
There is also the fact that it's scheduled for inclusion in Java7.

[gissuebot]

From limpbizkit on December 31, 2008 15:42:52

Unfortunately, although it's extremely handy, Doug Lea's ConcurrentReferenceHashMap doesn't have the one
method we need, whose signature would resemble either this
  public void putIfAbsent(K key, Function<K, V> keyToValue)
or perhaps this (which is what ReferenceCache uses):
  public abstract V create(K)

[gissuebot]

From gili.tzabari on December 31, 2008 16:13:22

Jesse,

I suppose you could always file a RFE with Doug and see what he says. Alternatively,
would it be possible to store Function in place of the actual value?

[gissuebot]

From dhanji on January 03, 2009 22:45:32

I wanted to add that awesome method to CHM itself, but there didn't seem to be much enthusiasm when I tried.

Afaik it is not solely Doug Lea's RefMap, it was developed by Red Hat et al., in parallel to a somewhat better
implementation that Google open sourced (viz. ReferenceCache). Unfortunately for us, the wrong one made it
into the JDK =(

[gissuebot]

From gili.tzabari on January 03, 2009 22:50:35

I didn't get the impression that it was too late to make changes to their
implementation. Did you file a formal RFE with them regarding this issue (do they
even have a public bug tracking system)? If so please link to it so I can run it by
my contacts at Sun. There is always room to apply more pressure :)

[gissuebot]

From crazyboblee on January 04, 2009 10:19:58

Nothing has made it into the JDK. Doug hasn't even actually looked at the RedHat impl (which doesn't actually
work). I already sent Doug a much better impl that shares code w/ CHM.

[gissuebot]

From dmdabbs on March 16, 2009 09:57:43

// Comment 11 by crazyboblee, Jan 04, 2009
// Nothing has made it into the JDK. Doug hasn't even actually looked at the RedHat
impl (which doesn't actually
//work). I already sent Doug a much better impl that shares code w/ CHM.

Is this better impl available anywhere?

[gissuebot]

From crazyboblee on March 16, 2009 10:03:53

Yes. Check out MapMaker in Google Collections
( https://code.google.com/p/google-collections/ ).

[gissuebot]

From limpbizkit on May 03, 2009 15:49:41

I believe this should be fixed. At a minimum, we now respect a security manager that refuses to let us spawn
threads.

Status: Fixed

[gissuebot]

From syvalta@yahoo.com on October 06, 2009 07:32:52

It seems to me that this issue is not fixed. From
com.google.inject.internal.Finalizer there is still the context class loader
reference, which has been discussed in the related issues.

[gissuebot]

From crazyboblee on October 06, 2009 07:42:41

Yeah, it still needs some work if you don't want to use a SM. I'm on it.

Status: Accepted

[gissuebot]

From alen_vrecko@yahoo.com on May 02, 2010 10:14:28

How about a refacture? MapMaker, that causes the thread and subsequently memory leak,
is used only at 2 places.

o) StackTraceElements. This can be refactured so it doesn't use weak-soft map. I just
removed the map entirely. Alternatively the caller could supply a plain HashMap and
clear it at the end when using StactTraceElements/Errors.

o) BytecodeGen. This is a bit trickier. I just disabled the caching of
BridgeClassloader. Imho, a reasonable compromise is if you disable bridge classloader
then the weak-weak map is not started as no bridge class loaders will be cached.

Alternatively instead of static cache with MapMaker, how about per Injector instance
cache with just plain HashMap?

Attachment: gist
   redeploySafe.diff

[gissuebot]

From mcculls on May 02, 2010 18:11:52

Well there's a reason for both of these weak caches - namely that you don't want to
keep creating the same object again and again (because it's expensive) but you also
want to have them automatically cleaned up when they're not needed.

By removing these caches you'd ironically be increasing the memory (and CPU) used by
Guice. In addition by turning off the bridge classloader cache you would create way
more classloaders than necessary which could exhaust the PermGen space.

(BTW - the classloader bridging is potentially needed whenever you have custom
classloaders loading your classes... be it application servers, tomcat, or OSGi)

[gissuebot]

From alen_vrecko@yahoo.com on May 03, 2010 02:21:27

From my understanding (or lack thereof) your comment is spot on if you put Guice in
bootclasspath. There the weak caches and bridge CL really shine. I can see it.

Now I must be missing something obvious but I don't think the behaviour is the same
when you put the guice.jar in the WAR. All the Guice classes get loaded in WebAppCl
and all the generated classes get loaded in the WACL or be it BCL.

When you redeploy/uninstall everything is eligible for GC. How much more can you
bargain for? Here I am at a loss, I don't see how weak caches will help here. What am
I missing?

[gissuebot]

From mcculls on May 03, 2010 02:40:29

By removing the weak caches you're forcing it to create new objects every time - even
though it might have created the same object in the past. For each of these use-cases
creating the object is an expensive operation, hence the cache. The situation is even
worse with classloaders because by creating one-per-bridged-type you would be making
the situation worse by isolating each bridged-type in its own tiny classloader (which
break certain visibility rules and would be a big overhead).

Even with WAR files you can have custom classloaders (typically when you want to
share classes or resources). Removing this cache (and changing to default to disable
bridging) would break a lot of applications that rely on this feature.

IMHO bridging should still be enabled by default - of course the creation of the
cache could be put inside a static initializer and made optional depending on the
flag. Then you could simply set the property to avoid the bridge cache.

[gissuebot]

From jose.illescas on May 17, 2010 09:28:17

This feature must be in 2.1 roadmap?

Now tomcat (from 6.0.26 version) logout as a possible memory leak:

<SEVERE> <WebappClassLoader.clearReferencesThreads> 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.

<SEVERE> <WebappClassLoader.clearThreadLocalMap> A web application created a
ThreadLocal with key of type [null] (value [com.google.inject.InjectorImpl$1@f2f585])
and a value of type [java.lang.Object[]] (value [[Ljava.lang.Object;@15fdf31]) but
failed to remove it when the web application was stopped. To prevent a memory leak,
the ThreadLocal has been forcibly removed.

[gissuebot]

From sameb@google.com on May 17, 2010 14:30:22

Guava's is now using a new implementation of MapMaker that doesn't use the extra
thread..  can we use that?

[gissuebot]

From terciofilho on September 09, 2010 13:29:09

Any news? Any workaround?

[gissuebot]

From alen_vrecko@yahoo.com on September 10, 2010 07:05:57

FWIW I am using a slightly changed version of Guice that doesn't leak on redeploy. See the svn patch (against tagged 2.0 release). Basically I just get rid of non-strong maps that cause the finalizer thread to start.

Changes made:

o) StackTraceElements originally uses weak.soft map(Thread). I've replaced this with a plain strong map which is cleared when Guice ends reporting errors.

o) BytecodeGen uses a weak.weak cache(Thread). I've put the cache in IODH (lazy singleton). I see that the latest code also follows a similar approach. Therefore if bridging is disabled the thread will not be started.

o) I've made bridging disabled by default as I don't see any added value for a plain servlet setup. It works also on WebSphere and don't see the point of making sure my co-workers diligently disable the bridging. Besides putting Guice in boot class path still produces a memory leak...

o) Replaced ThreadLocal with a plain concurrent map so Tomcat no longer spams with SEVERE ThreadLocal not cleared warning. It is less than 2x slower but this is acceptable price for me. I don't expect this to be popular choice.

I wouldn't be surprised if at Google they have a custom JDK that has this functionality built in.

Attachment: gist
   RedeplyForGuice.patch

[gissuebot]

From tj.rothwell on October 19, 2010 07:44:17

@crazyboblee Has this issue dropped to the floor? Is the SM method the only way to address this issue without custom patches? Looking forward to a status update. :)

[gissuebot]

From miazzo.valentino on December 10, 2010 05:35:59

Wow, Guice 3.0 is RC1 and this bug is still open.
It will be fixed in Guice 3.0?

[gissuebot]

From jose.illescas on December 10, 2010 07:07:02

You can add your comment (or opinion) on wiki page of 3.0 version ( https://code.google.com/p/google-guice/wiki/Guice30 )

[gissuebot]

From mcculls on January 11, 2011 15:57:16

Here's a potential solution, which adds a new property:

   -Dguice.executor.class

* This property can be used to pass a custom java.util.concurrent.Executor implementation class to Guice.

• Setting it to the empty string (-Dguice.executor.class=) will disable the Guice Finalizer thread completely.
• If this property is unset then Guice will use a simple Executor that creates a new thread (as at the moment).

This should avoid the main coupling issue wrt. who creates the actual thread (the Finalizer runnable is already decoupled) and also provide some means for shutting down the task from the outside without having to add a public shutdown API.

Note: I'm currently testing this patch in an experimental sisu-guice build: sonatype/sisu-guice@d5dc781

Attachment: gist
   GUICE_ISSUE_288_20110111.txt

[gissuebot]

From mcculls on January 11, 2011 16:42:31

Updated patch which avoids keeping a reference to the custom executor class.

Attachment: gist
   GUICE_ISSUE_288_20110112.txt

[gissuebot]

From mcculls on January 17, 2011 05:44:23

Latest patch: use "-Dguice.executor.class=NONE" to disable the background thread, as this makes the intention clear. Decided against using "false" because that would suggest that "true" was a valid value - similarly using the word "null" could be confused with the value null. "NONE" seems a reasonable compromise.

Attachment: gist
   GUICE_ISSUE_288_20110117.patch

[gissuebot]

From henrychankinwo on July 12, 2011 18:09:04

Will there be a new Guice release containing this fix?

[gissuebot]

From jose.illescas on September 29, 2011 04:14:39

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

[gissuebot]

From colin.taylor on October 02, 2011 18:12:38

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

[gissuebot]

From mcculls on October 02, 2011 18:36:18

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.

[gissuebot]

From mcculls on October 07, 2011 03:49:10

Minor patch to update Guava dependency to 10.0

Attachment: gist
   update_to_guava_10.patch
Binary attachments: guava-10.0.jar

[gissuebot]

From jose.illescas on October 26, 2011 04:03:12

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...

[gissuebot]

From mcculls on November 10, 2011 11:25:10

Issue 630 has been merged into this issue.

[gissuebot]

From jose.illescas on November 11, 2011 09:20:48

@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?

[gissuebot]

From stefan.simroth on November 11, 2011 09:27:37

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/2bc6e1554d29faf95df3cb959d258456ceaf060a Thx @mcculls for providing that suggestion.

[gissuebot]

From mcculls on November 11, 2011 09:34:20

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).

[gissuebot]

From balint@krivan.info on July 12, 2012 00:10:51

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!

[gissuebot]

From mcculls on July 20, 2012 11:11:56

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.

[gissuebot]

From pas256 on July 20, 2012 11:17:52

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

[gissuebot]

From mcculls on July 20, 2012 11:23:14

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.

[gissuebot]

From s.a.grigoriev on July 28, 2012 07:51:18

This definitely needs to be fixed.

[gissuebot]

From mcculls on August 06, 2012 18:25:21

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...

Attachment: gist
   GUICE_288_decouple_thread_local.patch

[gissuebot]

From mcculls on August 07, 2012 12:00:50

Issue 698 has been merged into this issue.

[gissuebot]

From nicolas.antoniazzi on August 24, 2012 02:01:51

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

[gissuebot]

From mcculls on September 02, 2012 03:32:55

Issue 698 has been merged into this issue.

[gissuebot]

From lorenzom on January 21, 2013 07:57:37

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.

Attachment: gist
   GUICE_288_static_concrete_class.patch

[gissuebot]

From mcculls on February 27, 2013 15:12:40

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.

[gissuebot]

From ryan@asleson.net on October 16, 2013 05:35:18

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.

[gissuebot]

From mcculls on October 16, 2013 06:00:36

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.

[gissuebot]

From cowwoc@bbs.darktech.org on October 16, 2013 08:10:20

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.

[gissuebot]

From mcculls on October 16, 2013 08:21:22

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: https://code.google.com/p/guava-libraries/source/detail?r=a13e02167e90125e6a78bf9bbd061996d05a143a . The FRQ fix is available in Guice 4.0-beta for testing.

[gissuebot]

From mcculls on December 01, 2013 17:23:50

Issue 786 has been merged into this issue.

[gissuebot]

From sberlin on December 05, 2013 15:51:07

Issue 707 has been merged into this issue.

[gissuebot]

From mvdgraaf on March 01, 2014 06:12:14

I see that there have been multiple attempts to fix the localContext variable in the InjectorImpl. But sadly they all fail to solve the issue completely and specially in a Servlet container.

In tomcat there is still a warning (also in 4.0-beta) because the ThreadLocal is not cleared (the object[] is still leaked into the Threads).

With the current JVM's it requires a new ThreadLocal to do something to finally garbage collect  ThreadLocal's values from the Thread class and each thread needs to get hit with that new ThreadLocal to completely cleanup the mess left behind by other ThreadLocal's this can take some time for an application server like tomcat because it can run 100+ Thread's and there is no guarantee that the Thread will ever hit a new ThreadLocal.

To solve these issues we need to clear the ThreadLocal and must not use a library/application extends class to initialize the ThreadLocal.

The attached patch file removes the LocalContextThreadLocal, removes the unnecessary Object[] and clears the ThreadLocal.

Attachment: gist
   ThreadLocalFix-guice.patch

[gissuebot]

From mcculls on March 01, 2014 06:18:53

IIRC the use of the holder array was for performance reasons, but there is a way to keep the holder array and still avoid the implicit classloader reference and associated Tomcat warning: https://github.com/sonatype/sisu-guice/blob/master/PATCHES/GUICE_288_decouple_thread_local.patch