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: https://github.com/sonatype/sisu-guice/commit/d5dc781d31a625a9596cd0fcbe3c54fa15ee28ce

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 cowwoc%bbs.darktech.org@gtempaccount.com on January 17, 2011 06:20:48

So if I understand you correctly: the benefit of allowing users to configure a custom Executor is that they can then invoke ExecutorService.shutdown(). Is that correct?

[gissuebot]

From mcculls on January 17, 2011 06:32:37

There are many benefits:

1) the container can decide not to accept the task, in which case the cleanup work is only done in the foreground

2) the container can run the task on a 'pure' thread with no references to the app - part of the problem at the moment is that Guice/Guava creates a child thread which can have internal references back to the parent thread (and from the to the app domain) that then lead to it not being GC'd properly - this should mean the thread will shutdown itself cleaning when the app is undeployed

3) the container can decide to attempt to stop the work by interrupting the runnable with an exception/error or if that fails by forcibly stopping the thread as a last resort

So while the Executor approach will let you forcibly shut down the thread, it should be seen as a last resort.

[gissuebot]

From maxb@j.maxb.eu on January 17, 2011 07:16:26

Is controlling this by means of a system property a good idea? Surely the sorts of embedded environments where you would want to avoid this behaviour are ones where you may not conveniently be able to add system properties?

[gissuebot]

From mcculls on January 17, 2011 07:30:07

Using a system property allows people to experiment with this feature before committing to a particular API. Remember this is only a proposal at the moment (although available for testing over at sisu-guice on github).

[gissuebot]

From cowwoc%bbs.darktech.org@gtempaccount.com on January 17, 2011 07:54:45

Moved comment from https://code.google.com/p/guava-libraries/issues/detail?id=92#c38 ----- Let's remember: the main goal of this RFE is to solve a pain-point for web applications, not desktop applications.

There is no ambiguity for webapps as to who should invoke the shutdown() method because there are well-defined shutdown hooks and expectations for what should happen on subsequent requests.

System properties are a bad fit for webapps as they cannot be controlled on a per-webapp basis.

Don't get me wrong. I think your use of Executors is quite clever. I just don't think it really solves this RFE.

Here's an idea: how about allowing us to configure the Executor in the Guice Module? For example:

public class MyModule extends AbstractModule {    private final ExecutorService sharedExecutor = ...;

   protected void configure() {      bindConstant().annotatedWith(GuiceExecutorAnnotation).to(sharedExecutor);    }  }

Nothing prevents you from sharing the same executor across all injectors, and if you really want separate Executors you could do that as well.

[gissuebot]

From mcculls on January 17, 2011 08:11:06

Unfortunately that's not possible - all this happens before the injector is created, in code that has no knowledge of individual injectors or their bindings. The only alternative I can think of would be a static method somewhere - but that would have to be accepted by other member of the Guice team.

For now the patch uses a system property because it's still only a proposal - what would help is if people tried out the snapshot that contains this experimental patch to see if it resolved the unload/shutdown issue for them: https://repository.sonatype.org/content/groups/forge/org/sonatype/sisu/sisu-guice/3.0-SNAPSHOT/ The system property shouldn't be a problem when it comes to testing the feature on a development machine.

Positive feedback would help towards getting this patch applied - then hopefully we could move on to discuss how best to configure this setting (be it a static method or another approach). Alternative patches would also welcome.

[gissuebot]

From tj.rothwell on January 17, 2011 16:42:36

In Tomcat 6.0.29, it doesn't find my defined class.

Finalizer.class.getClassLoader() is a java.net.URLClassLoader which would probably only be looking in jar.

I tried Thread.currentThread().getContextClassLoader() which is a org.apache.catalina.loader.WebappClassLoader and the class is loaded.

I didn't try other class loaders (e.g. Class.forName()).

[gissuebot]

From mcculls on January 19, 2011 13:32:54

yes Thread.currentThread().getContextClassLoader() probably makes more sense

[gissuebot]

From mcculls on January 19, 2011 14:14:04

Prototype patch now tries to load the custom Executor from the TCCL before falling back to Class.forName

Attachment: gist    _GUICE_ISSUE_288_20110119.patch_

[gissuebot]

From tj.rothwell on January 25, 2011 07:59:39

I must be missing something--does it make sense to have a custom executor? How does this solve the sticky references that prevents gc of the frq?

Using "NONE" solves it, but is there a way to have the Finalizer thread and still have the container not leak?

[gissuebot]

From mcculls on January 25, 2011 08:30:43

The problem is that when you create the child Finalizer thread inside a secure app container there are internal JVM references back to the parent thread (access context, etc.) that means it cannot be GC'd, and it is these refs that keep the thread around. These internal refs can be nulled out using reflection, but then you're making deep assumptions about JVM internals which may change.

Using an executor means you can control how this background work is scheduled - ie. you can choose the thread and forcibly stop that thread if the work fails to completely properly. For libraries like guava (which has the same issue) you could multiplex work onto one thread - rather than have multiple threads managing Finalizers.

Using executors is also better than a single big red shutdown button because it works better with containers that might want to share the Guice library between multiple applications. Or to put it another way: guice wants to do some background work, and the executor API is how it can communicate that wish with the container.

[gissuebot]

From Andrei.Pozolotin on January 27, 2011 15:07:04

Stuart: thanks for the patch; I vote for this for I need it to terminate this thread from external container; I hope in final form in guice 3.0 you do not have to use System.property(); Andrei

[gissuebot]

From tj.rothwell on January 27, 2011 15:24:25

I've been meaning to test again, but haven't had time the last couple of days. From #41, it looks to do exactly what needs to be done. Thanks Stuart.

[gissuebot]

From cowwoc%bbs.darktech.org@gtempaccount.com on January 28, 2011 06:18:15

Assuming everyone is in favor of this patch, how do you propose on making this work without System properties (since they are a non-starter for webapps)?

[gissuebot]

From jose.illescas on January 28, 2011 06:23:21

One static method Injector.destroy() ?

[gissuebot]

From mcculls on January 28, 2011 06:28:59

Unfortunately static shutdown methods don't work well when you're sharing the JAR between applications...

[gissuebot]

From Andrei.Pozolotin on January 28, 2011 06:47:45

ServiceLoader? http://download.oracle.com/javase/6/docs/api/java/util/ServiceLoader.html

[gissuebot]

From jose.illescas on January 28, 2011 06:48:53

Guice.createInjector(myModule, myExecutor) ?

[gissuebot]

From simone.tripodi on January 28, 2011 06:50:59

ServiceLoader is only available from Java6, Guice target is Java5 and the animal-sniffer-plugin in the mvn build takes care of Java5 APIs compliance

[gissuebot]

From cowwoc%bbs.darktech.org@gtempaccount.com on January 28, 2011 07:19:15

Correct me if I'm wrong, but I don't think we can use Guice.createInjector(myModule, myExecutor) for the same reason mentioned in comment #36.

As far as I can tell, we have four options:

1. Do something at Guice initialization time: Guice.init(Executor), or
2. Do something at Guice shutdown time: Guice.shutdown(), or
3. Refactor Guice allow Injector-level finalization: comment #35
4. Guice ships with a ServletContextListener that uses implementation details to shut down Guice (using reflection, unsafe casting or anything else you can come up with). Users would then be responsible for registering this ServletContextListener in their configuration.

Did I miss anything?