Probable EventBus memory leak when shared class loader used

[GoogleCodeExporter]

Hi,

I am having Guava loaded by shared classloader inside Tomcat and every webapp 
can register into static singleton EventBus waiting to incoming events. Problem 
is that it looks like that EventBus from shared classloader is unable to free 
registered methods when webapp is undeployed.

I have attached test case with workaround and probable fix when LoadingCache 
uses weakKeys with weakValues.

Could you look at it - code will tell you more. You can build test case with 
Guava 14 and 17 (Maven profile) when symptoms are the same. 

Thx
Ivos

*Test case 1*
{{{Using Guava version 17.0
class cz.bedla.guava.MemoryLeak loaded by 
sun.misc.Launcher$AppClassLoader@1d6535bf
class com.google.common.eventbus.EventBus loaded by 
sun.misc.Launcher$AppClassLoader@1d6535bf
--- start ---
Test using ordinary Guava
class guava.Callback loaded by java.net.URLClassLoader@25b0eadd
class guava.Callback$Event loaded by java.net.URLClassLoader@25b0eadd
onEvent(guava.Callback$Event@edea70b)
    Callback    java.net.URLClassLoader@25b0eadd
    Event       java.net.URLClassLoader@25b0eadd
--- end ---
Loading cache result {class guava.Callback=[public void 
guava.Callback.onEvent(guava.Callback$Event)]}
=== Memory leak detected? ===}}}

*Test case 2*
_Note: take a look at inconsistency message_
{{{Using Guava version 17.0
class cz.bedla.guava.MemoryLeak loaded by 
sun.misc.Launcher$AppClassLoader@5fab9dac
class com.google.common.eventbus.EventBus loaded by 
sun.misc.Launcher$AppClassLoader@5fab9dac
--- start ---
Test with weak values loading cache
class com.google.common.cache.LocalCache$LocalLoadingCache loaded by 
sun.misc.Launcher$AppClassLoader@5fab9dac
class guava.Callback loaded by java.net.URLClassLoader@1f4cc34b
class guava.Callback$Event loaded by java.net.URLClassLoader@1f4cc34b
onEvent(guava.Callback$Event@c063ed4)
    Callback    java.net.URLClassLoader@1f4cc34b
    Event       java.net.URLClassLoader@1f4cc34b
--- end ---
Loading cache result {}
=== Memory leak detected? ===
Inconsistent .toString({}) and .size(1) when LoadingCache.asMap() called ???}}}

*Test case 3*
{{{Using Guava version 17.0
class cz.bedla.guava.MemoryLeak loaded by 
sun.misc.Launcher$AppClassLoader@5fab9dac
class com.google.common.eventbus.EventBus loaded by 
sun.misc.Launcher$AppClassLoader@5fab9dac
--- start ---
Test with loading cache invalidation workaround
class guava.Callback loaded by java.net.URLClassLoader@f785762
class guava.Callback$Event loaded by java.net.URLClassLoader@f785762
onEvent(guava.Callback$Event@2aae2481)
    Callback    java.net.URLClassLoader@f785762
    Event       java.net.URLClassLoader@f785762
--- end ---
Loading cache result {} }}}

Original issue reported on code.google.com by bedla.cz...@gmail.com on 23 Aug 2014 at 11:18

Attachments:

• guava-memory-leak.zip
• guava-leak.png

[GoogleCodeExporter]

I guess this is caused by the static members of `SubscriberRegistry`: 
`flattenHierarchyCache` and `subscriberMethodsCache`. They use `weakKeys`, but 
this seems to be of no use at all, because of the strong value references. In 
your example, an entry linking `Callback.class` to 
`ImmutableList.of(Callback#onEvent)` gets created, and the method strongly 
references its class, so nothing can be GC'ed.

Original comment by Maaarti...@gmail.com on 27 Aug 2014 at 3:18

[GoogleCodeExporter]

I know why it is caused :-) . Anyways thanks for summarization...

Original comment by bedla.cz...@gmail.com on 27 Aug 2014 at 7:13

[GoogleCodeExporter]

I'm not sure what can be done about this. In the flattenHierarchyCache case 
(used for event types), I think we could safely store the values in 
WeakReferences. In the subscriberMethodsCache, I'm pretty sure we can't, 
because Class always returns *copies* of the Method objects. In other words, if 
we store WeakReferences to the Methods, nothing will be strongly referencing 
those Method objects and they'll just get GC'ed, meaning we aren't really 
caching anything. And just not caching the Methods seems like it could have a 
significant performance impact.

As an aside, I don't feel like using a shared EventBus to communicate between 
separate web applications is a good idea. In particular, I'd be concerned about 
things like different applications loading the "same" class with different 
classloaders. There are other tools that are better suited for what is 
effectively interprocess communication.

Original comment by cgdecker@google.com on 30 Aug 2014 at 5:44

• Added labels: Package-EventBus, Type-Enhancement

[GoogleCodeExporter]

> nothing will be strongly referencing those Method objects

I'd say, as long as they're used in an `EventBus`, their `Subscriber`s will. 
But if they're not, then they can be GC'd even when their classes are loaded 
and this is AFAIK unsolvable without ephemerons.

And you'd have to use `ImmutableList<WeakReference<Method>>` which means that 
you could get an incomplete method list from the cache. Surely solvable, but 
ugly.

Anyway, using `weakKeys` in the current design doesn't help at all, does it?

A minor optimization: I guess `getAnnotatedMethodsNotCached` could use 
`flattenHierarchyCache` instead of `TypeToken.of(clazz).getTypes().rawTypes()`.

Original comment by Maaarti...@gmail.com on 30 Aug 2014 at 7:44

[GoogleCodeExporter]

> I'd say, as long as they're used in an `EventBus`, their `Subscriber`s will. 
But if they're not, then they can be GC'd even when their classes are loaded 
and this is AFAIK unsolvable without ephemerons.

True, though if you've got a class that is subscribed and unsubscribed 
repeatedly it's not really helping.

> And you'd have to use `ImmutableList<WeakReference<Method>>` which means that 
you could get an incomplete method list from the cache. Surely solvable, but 
ugly.

Actually, if the Methods are being thrown into WeakReferences in the 
CacheLoader, they wouldn't have any strong references left even before the call 
to LoadingCache.get that loads them completes. Which makes it sound like it's 
possible that you might *never* get a complete list of Methods back. 

> Anyway, using `weakKeys` in the current design doesn't help at all, does it?

No, not really.

> A minor optimization: I guess `getAnnotatedMethodsNotCached` could use 
`flattenHierarchyCache` instead of `TypeToken.of(clazz).getTypes().rawTypes()`.

It could, but I don't feel like it's really an optimization at all given that 
the result of getAnnotatedMethodsNotCached is cached itself. Right now, that 
TypeToken.of(clazz).getTypes().rawTypes() should only be called once per class 
anyway, so we'd be caching the result for no reason. (And I think it's very 
unlikely that there would be any overlap between the current usage of 
flattenHiearchyCache and this, since currently it's only used for event 
classes, while this would be using it for subscriber classes.) If we weren't 
caching the result, that would be a different story.

Original comment by cgdecker@google.com on 30 Aug 2014 at 8:48

[GoogleCodeExporter]

I think that good enought solution would be having flush cache methods on 
EventBus (or on AnnotatedSubscriberFinder) like JavaBeans Introspector have. 

java.beans.Introspector#flushCaches()
java.beans.Introspector#flushFromCaches(Class<?>)

Btw. In my design I have MyEvent classes loaded by same shared classloader 
which loads Guava. And shared event classes refer to classes from same shared 
classloader or from parent class loaders.

Original comment by bedla.cz...@gmail.com on 31 Aug 2014 at 7:08

[GoogleCodeExporter]

This issue has been migrated to GitHub.

It can be found at https://github.com/google/guava/issues/<issue id>

Original comment by cgdecker@google.com on 1 Nov 2014 at 4:08

• Added labels: MigratedToGitHub

[GoogleCodeExporter]

Original comment by cgdecker@google.com on 1 Nov 2014 at 4:17

• Changed state: Migrated

[GoogleCodeExporter]

Original comment by cgdecker@google.com on 3 Nov 2014 at 9:07

• Added labels: Restrict-AddIssueComment-Commit