Memory leak by using child injector and multibinder

[GoogleCodeExporter]

The attached minimal example shows how guice child injectors are leaking memory 
when used with MapBinder.

From the analysis I performed so far it looks like a WeakKeySet instance 
associated with the parent keeps growing. From what I understand from the code 
"mapped" instances in child injector are marked in a blacklist of the parent 
injector and are never released.

Original issue reported on code.google.com by gblo...@gmail.com on 15 Jul 2013 at 4:15

Attachments:

• ChildInjectorMemoryLeak.java

[GoogleCodeExporter]

Basically what is the problem, is that by using the toString method to put the 
binding in the blacklist, this will lead to a new blacklist entry for every 
multibound class in each child injector since RealElement of 
guice-multibindings generates a uniqueid that is included in every toString.

Is including the uniqueid in the toString of RealElement really needed? As 
without that it seems that blacklisting still works fine, except no ridiculous 
amounts of blacklist entries are created.

Original comment by johannes...@gmail.com on 21 Sep 2013 at 9:02

[GoogleCodeExporter]

Without the unique ID, wouldn't we blacklist more than necessary?

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

[GoogleCodeExporter]

The attached patch file fixes the memory leak by updating WeakKeySet to use a 
java.util.WeakHashMap as its internal data structure.

The patch was created against the master branch using 'git format-patch'.

Multibinders create "unique" keys for each binding that is a member of the 
collection (set or map).  If multibinders are used in child injectors, then the 
unique keys are added to the parent's blacklist. If child injectors are 
routinely created, then this leads to a memory leak where more and more 
Multibinding keys are added to the blacklist.

The patch changes the WeakKeySet class (which manages the blacklist) to use 
true weak references. When the child injector is eligible for garbage 
collection, so will the keys it contributed to the blacklist (thus eliminating 
the memory leak).

It is possible that something else will hold strong references to the keys, but 
that is almost certainly NOT the case for Multibinding keys because only the 
Multibinder and the child injector are aware of the keys. As such, any 
unintended affects that may exist by others holding strong references to the 
keys are highly unlikely to occur in practice (because the user code would have 
to be doing something ridiculous).

This is a more general fix than fixing the Multibinder RealElement.toString 
method (which would only fix the Multibinder extension and not third-party or 
future extensions).

Original comment by jim.hu...@gmail.com on 31 Mar 2014 at 5:52

Attachments:

• 0001-Fixes-WeakKeySet-to-use-real-weak-references.patch

[GoogleCodeExporter]

What are the chances of getting the memory leak fix in to Guice 4.0?

We've heavily tested the patch submitted in Comment #3 in our internal 
application and it works perfectly and with no unintended consequences.  We 
would rather not maintain an internal fork of Guice, so it would be fantastic 
if we could get the fix into Guice 4.0.

Original comment by jim.hu...@gmail.com on 31 Mar 2014 at 5:55

[GoogleCodeExporter]

Using a WeakHashMap w/ Key as the key type doesn't work because the map is 
shared among many different child injectors that may each be independently 
blacklisting the key.

We actually have a different change ready internally that should be submitted 
any day now to fix this particular bug, and we'll get it included in 4.0.

Original comment by sberlin on 31 Mar 2014 at 6:06

[GoogleCodeExporter]

Fixed by r=bab9b6082ff7c3aefac2dc8c7de0468fe60fe8f6

Original comment by sberlin on 2 Apr 2014 at 12:34

• Changed state: Fixed

[GoogleCodeExporter]

FWIW, we tested the fix mentioned in comment #6 (using a local build of guice 
from HEAD) and we can confirm that it does eliminate the memory leak.

Original comment by jim.hu...@gmail.com on 24 Apr 2014 at 7:12

[GoogleCodeExporter]

Great news, thanks for letting us know!

Original comment by sberlin on 24 Apr 2014 at 7:18