Probable EventBus memory leak when shared class loader used

[gissuebot]

Original issue created by bedla.czech on 2014-08-23 at 11:18 AM

---

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 {}

[gissuebot]

Original comment posted by Maaartinus on 2014-08-27 at 03:18 PM

---

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.

[gissuebot]

Original comment posted by bedla.czech on 2014-08-27 at 07:13 PM

---

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

[gissuebot]

Original comment posted by [email protected] on 2014-08-30 at 05:44 PM

---

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.

---

Labels: Type-Enhancement, Package-EventBus

[gissuebot]

Original comment posted by Maaartinus on 2014-08-30 at 07:44 PM

---

nothing will be strongly referencing those Method objects

I'd say, as long as they're used in an EventBus, their Subscribers 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().

[gissuebot]

Original comment posted by [email protected] on 2014-08-30 at 08:48 PM

---

I'd say, as long as they're used in an EventBus, their Subscribers 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.

[gissuebot]

Original comment posted by bedla.czech on 2014-08-31 at 07:08 AM

---

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.

[cgdecker]

I'm closing this issue because we are no longer going to be making changes to EventBus other than important bug fixes. The reasons we're discouraging it and some alternatives are listed in the EventBus javadoc.