search

bennidi / mbassador

Powerful event-bus optimized for high throughput in multi-threaded applications. Features: Sync and Async event publication, weak/strong references, event filtering, annotation driven
MIT License
955 stars 146 forks source link

Fixing memoryleak in multiclassloader context #167

Open FlorianKirmaier opened 3 years ago

FlorianKirmaier commented 3 years ago

Keeping classes keeps the classloaders forever, introducing leaks when the classloader should be collected.

bennidi commented 2 years ago

I have been looking into your code and it is not quite clear to me what problem you are trying to solve. What is the memory leak you are talking about?

The provided modifications look problematic to me. From the commit history I can see that you also provided fixes for the fixes. So the fix itself caused new problems. It seems that the modifications make the code less clear and understandable.

FlorianKirmaier commented 2 years ago

Sadly I don't know a way, to dynamically create classes during Runtime - then it would be possible to write a unit-test for the leak.

The leak is a bit specific because it only happens in an environment where classloaders are created and removed - for example when plugins can be dynamically loaded/unloaded.

One Version didn't work, because it's not sufficient to make the HashMaps weak, because the Value also references the Key. Another version had a typo using the wrong HashMap.

This version now seems to work and didn't cause any issues. If you want, I can make a new PR, with only one commit.

What does the fix do? It basically counts the subscriptions, and if it reaches zero, they are explicitly unsubscribed.

bennidi commented 2 years ago

That is indeed a specific scenario - and very understandable you can't provide a unit test for this. Generally, I am very cautious about changing code in core methods. Especially if it only covers a very specific use case. I recently merged another PR that introduced cleaning methods for the subscriptions. I think that this is the way to deal with such a situation. If the bus is used within a plugin environment I expect there will be some kind of hook or event that could be used to plugin a handler that cleans all empty subscriptions. Trying to do it automatically every time a listener is subscribed adds code complexity and runtime overhead.

I think that cleanupEmptySubscriptions or just cleanup could do the trick. How about that?