Memory leak by using child injector and multibinder

[gissuebot]

From gbloisi on July 15, 2013 12:15:57

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.

Attachment: gist
   ChildInjectorMemoryLeak.java

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

[gissuebot]

From johannes.wachter on September 21, 2013 14:02:32

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.

[gissuebot]

From sberlin on December 05, 2013 14:51:06

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

[gissuebot]

From jim.hurne on March 31, 2014 10:52:54

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

Attachment: gist
   0001-Fixes-WeakKeySet-to-use-real-weak-references.patch

[gissuebot]

From jim.hurne on March 31, 2014 10:55:24

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.

[gissuebot]

From sberlin on March 31, 2014 11:06:39

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.

[gissuebot]

From sberlin on April 01, 2014 17:34:15

Fixed by r= bab9b60

Status: Fixed

[gissuebot]

From jim.hurne on April 24, 2014 12:12:31

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.

[gissuebot]

From sberlin on April 24, 2014 12:18:10

Great news, thanks for letting us know!