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]

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

Original comment by limpbizkit on 30 Dec 2008 at 10:03

[GoogleCodeExporter]

Original comment by limpbizkit on 30 Dec 2008 at 11:39

• Added labels: Priority-High, Milestone-Release2.0

[GoogleCodeExporter]

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

Original comment by gili.tza...@gmail.com on 31 Dec 2008 at 12:10

[GoogleCodeExporter]

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

Original comment by limpbizkit on 31 Dec 2008 at 5:03

[GoogleCodeExporter]

Any thoughts on the experimental concurrent reference map from JSR166?

  http://viewvc.jboss.org/cgi-
bin/viewvc.cgi/jbosscache/experimental/jsr166/src/jsr166y/ConcurrentReferenceHas
hMap.
java?view=markup

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

Original comment by mccu...@gmail.com on 31 Dec 2008 at 10:19

[GoogleCodeExporter]

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.

Original comment by gili.tza...@gmail.com on 31 Dec 2008 at 10:32

[GoogleCodeExporter]

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)

Original comment by limpbizkit on 31 Dec 2008 at 11:42

[GoogleCodeExporter]

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?

Original comment by gili.tza...@gmail.com on 1 Jan 2009 at 12:13

[GoogleCodeExporter]

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 =(

Original comment by dha...@gmail.com on 4 Jan 2009 at 6:45

[GoogleCodeExporter]

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

Original comment by gili.tza...@gmail.com on 4 Jan 2009 at 6:50

[GoogleCodeExporter]

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.

Original comment by crazybob...@gmail.com on 4 Jan 2009 at 6:19

[GoogleCodeExporter]

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

Original comment by dmda...@gmail.com on 16 Mar 2009 at 4:57

[GoogleCodeExporter]

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

Original comment by crazybob...@gmail.com on 16 Mar 2009 at 5:03

[GoogleCodeExporter]

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

Original comment by limpbizkit on 3 May 2009 at 10:49

• Changed state: Fixed

[GoogleCodeExporter]

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.

Original comment by syva...@yahoo.com on 6 Oct 2009 at 2:32

[GoogleCodeExporter]

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

Original comment by crazybob...@gmail.com on 6 Oct 2009 at 2:42

• Changed state: Accepted

[GoogleCodeExporter]

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?

Original comment by alen_vre...@yahoo.com on 2 May 2010 at 5:14

Attachments:

• redeploySafe.diff

[GoogleCodeExporter]

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)

Original comment by mccu...@gmail.com on 3 May 2010 at 1:11

[GoogleCodeExporter]

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?

Original comment by alen_vre...@yahoo.com on 3 May 2010 at 9:21

[GoogleCodeExporter]

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.

Original comment by mccu...@gmail.com on 3 May 2010 at 9:40

[GoogleCodeExporter]

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.

Original comment by jose.ill...@gmail.com on 17 May 2010 at 4:28

[GoogleCodeExporter]

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

Original comment by sa...@google.com on 17 May 2010 at 9:30

[GoogleCodeExporter]

Any news? Any workaround?

Original comment by terciofi...@gmail.com on 9 Sep 2010 at 8:29

[GoogleCodeExporter]

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.

Original comment by alen_vre...@yahoo.com on 10 Sep 2010 at 2:05

Attachments:

• RedeplyForGuice.patch

[GoogleCodeExporter]

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

Original comment by tj.rothw...@gmail.com on 19 Oct 2010 at 2:44

[GoogleCodeExporter]

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

Original comment by miazzo.v...@googlemail.com on 10 Dec 2010 at 1:35

[GoogleCodeExporter]

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

Original comment by jose.ill...@gmail.com on 10 Dec 2010 at 3:07

[GoogleCodeExporter]

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

Original comment by mccu...@gmail.com on 11 Jan 2011 at 11:57

Attachments:

• GUICE_ISSUE_288_20110111.txt

[GoogleCodeExporter]

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

Original comment by mccu...@gmail.com on 12 Jan 2011 at 12:42

Attachments:

• GUICE_ISSUE_288_20110112.txt

[GoogleCodeExporter]

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.

Original comment by mccu...@gmail.com on 17 Jan 2011 at 1:44

Attachments:

• GUICE_ISSUE_288_20110117.patch

[GoogleCodeExporter]

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?

Original comment by cowwoc...@gmail.com on 17 Jan 2011 at 2:20

[GoogleCodeExporter]

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.

Original comment by mccu...@gmail.com on 17 Jan 2011 at 2:32

[GoogleCodeExporter]

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?

Original comment by m...@j.maxb.eu on 17 Jan 2011 at 3:16

[GoogleCodeExporter]

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

Original comment by mccu...@gmail.com on 17 Jan 2011 at 3:30

[GoogleCodeExporter]

Moved comment from 
http://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.
-----

Original comment by cowwoc...@gmail.com on 17 Jan 2011 at 3:54

[GoogleCodeExporter]

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.

Original comment by mccu...@gmail.com on 17 Jan 2011 at 4:11

[GoogleCodeExporter]

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

Original comment by tj.rothw...@gmail.com on 18 Jan 2011 at 12:42

[GoogleCodeExporter]

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

Original comment by mccu...@gmail.com on 19 Jan 2011 at 9:32

[GoogleCodeExporter]

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

Original comment by mccu...@gmail.com on 19 Jan 2011 at 10:14

Attachments:

• GUICE_ISSUE_288_20110119.patch

[GoogleCodeExporter]

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?

Original comment by tj.rothw...@gmail.com on 25 Jan 2011 at 3:59

[GoogleCodeExporter]

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.

Original comment by mccu...@gmail.com on 25 Jan 2011 at 4:30

[GoogleCodeExporter]

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

Original comment by Andrei.Pozolotin on 27 Jan 2011 at 11:07

[GoogleCodeExporter]

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.

Original comment by tj.rothw...@gmail.com on 27 Jan 2011 at 11:24

[GoogleCodeExporter]

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

Original comment by cowwoc...@gmail.com on 28 Jan 2011 at 2:18

[GoogleCodeExporter]

One static method Injector.destroy() ?

Original comment by jose.ill...@gmail.com on 28 Jan 2011 at 2:23

[GoogleCodeExporter]

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

Original comment by mccu...@gmail.com on 28 Jan 2011 at 2:28

[GoogleCodeExporter]

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

Original comment by Andrei.Pozolotin on 28 Jan 2011 at 2:47

[GoogleCodeExporter]

Guice.createInjector(myModule, myExecutor) ?

Original comment by jose.ill...@gmail.com on 28 Jan 2011 at 2:48

[GoogleCodeExporter]

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

Original comment by simone.t...@gmail.com on 28 Jan 2011 at 2:50

[GoogleCodeExporter]

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?

Original comment by cowwoc...@gmail.com on 28 Jan 2011 at 3:19