FinalizableReferenceQueue still leaks #288
Comments
|
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 |
|
From limpbizkit on December 30, 2008 15:39:59 (No comment was entered for this change.) Labels: Priority-High Milestone-Release2.0 |
|
From gili.tzabari on December 30, 2008 16:10:12
|
|
From limpbizkit on December 30, 2008 21:03:52
|
|
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. it doesn't use a thread, but still supports the concurrent map API. |
|
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 ;) |
|
From limpbizkit on December 31, 2008 15:42:52 Unfortunately, although it's extremely handy, Doug Lea's ConcurrentReferenceHashMap doesn't have the one |
|
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, |
|
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 |
|
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 |
|
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 |
|
From dmdabbs on March 16, 2009 09:57:43 // Comment 11 by crazyboblee, Jan 04, 2009 Is this better impl available anywhere? |
|
From crazyboblee on March 16, 2009 10:03:53 Yes. Check out MapMaker in Google Collections |
|
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 Status: Fixed |
|
From [email protected] on October 06, 2009 07:32:52 It seems to me that this issue is not fixed. From |
|
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 |
|
From [email protected] on May 02, 2010 10:14:28 How about a refacture? MapMaker, that causes the thread and subsequently memory leak, o) StackTraceElements. This can be refactured so it doesn't use weak-soft map. I just o) BytecodeGen. This is a bit trickier. I just disabled the caching of Alternatively instead of static cache with MapMaker, how about per Injector instance Attachment: gist |
|
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 By removing these caches you'd ironically be increasing the memory (and CPU) used by (BTW - the classloader bridging is potentially needed whenever you have custom |
|
From [email protected] on May 03, 2010 02:21:27 From my understanding (or lack thereof) your comment is spot on if you put Guice in Now I must be missing something obvious but I don't think the behaviour is the same When you redeploy/uninstall everything is eligible for GC. How much more can you |
|
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 Even with WAR files you can have custom classloaders (typically when you want to IMHO bridging should still be enabled by default - of course the creation of the |
|
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 <SEVERE> <WebappClassLoader.clearThreadLocalMap> A web application created a |
|
From [email protected] on May 17, 2010 14:30:22 Guava's is now using a new implementation of MapMaker that doesn't use the extra |
|
From terciofilho on September 09, 2010 13:29:09 Any news? Any workaround? |
|
From [email protected] 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 |
|
From tj.rothwell on October 19, 2010 07:44:17
|
|
From miazzo.valentino on December 10, 2010 05:35:59 Wow, Guice 3.0 is RC1 and this bug is still open. |
|
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 ) |
|
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.
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 |
|
From mcculls on January 11, 2011 16:42:31 Updated patch which avoids keeping a reference to the custom executor class. Attachment: gist |
|
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 |
|
From henrychankinwo on July 12, 2011 18:09:04 Will there be a new Guice release containing this fix? |
|
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? |
|
From colin.taylor on October 02, 2011 18:12:38 Any word on a fix? Nothing mentioned so far works for me.. |
|
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. |
|
From mcculls on October 07, 2011 03:49:10 Minor patch to update Guava dependency to 10.0 Attachment: gist |
|
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... |
|
From mcculls on November 10, 2011 11:25:10 Issue 630 has been merged into this issue. |
|
From jose.illescas on November 11, 2011 09:20:48
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 Any fix/idea? |
|
From stefan.simroth on November 11, 2011 09:27:37 Hi, |
|
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). |
|
From [email protected] 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 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! |
|
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. |
|
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! |
|
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. |
|
From s.a.grigoriev on July 28, 2012 07:51:18 This definitely needs to be fixed. |
|
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 |
|
From mcculls on August 07, 2012 12:00:50 Issue 698 has been merged into this issue. |
|
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. Thanks |
|
From mcculls on September 02, 2012 03:32:55 Issue 698 has been merged into this issue. |
|
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 |
|
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. |
|
From [email protected] 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. |
|
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. |
|
From [email protected] 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. |
|
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. |
|
From mcculls on December 01, 2013 17:23:50 Issue 786 has been merged into this issue. |
|
From sberlin on December 05, 2013 15:51:07 Issue 707 has been merged into this issue. |
|
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 |
|
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 |
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
The text was updated successfully, but these errors were encountered: