Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixing memoryleak in multiclassloader context #167

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

FlorianKirmaier
Copy link

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

@bennidi
Copy link
Owner

bennidi commented Nov 2, 2021

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
Copy link
Author

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
Copy link
Owner

bennidi commented Nov 3, 2021

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants