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

HazelcastClusterManager#getAsyncMultiMap memory leak #316

Closed
michalsida opened this issue Jan 4, 2018 · 6 comments
Closed

HazelcastClusterManager#getAsyncMultiMap memory leak #316

michalsida opened this issue Jan 4, 2018 · 6 comments
Assignees
Labels

Comments

@michalsida
Copy link

michalsida commented Jan 4, 2018

Repeating call cause probably a memory leak. In this call is created a new one HazelcastAsyncMultiMap - a wrapper for Hazelcast MultiMap, it is stored into weak set and send it to the result handler. So if the result handler throw the reference away, it should be removed from weak set too.

But.

There is a call of com.hazelcast.core.MultiMap#addEntryListener(this, true) in the HazelcastAsyncMultiMap constructor, which will store a strong reference to the object to some Hazelcast's ConcurrentHashMap of registered listeners. And it will prevent a garbage collecting of HazelcastAsyncMultiMap instance (and referenced data/objects), which is unused by result handler already.

Detected in version: 3.5.0

@vietj
Copy link

vietj commented Jan 8, 2018

can you investigate @tsegismont ?

@tsegismont
Copy link

tsegismont commented Jan 10, 2018

@michalsida nice catch!

This is an area where we would like to improve the ClusterManager SPI. The SPI grew out of the initial Hazelcast implementation, and the getAsyncMultiMap leaked out to ClusterManager so that ClusteredEventBus can get one to store handler registrations. It wasn't intended as a user-facing method, but here it is now...

The good news is that ClusteredEventBus never calls this method more than once, so you should be safe in your project. Until we review the ClusterManager SPI, I would like to add a close or dispose method to AsyncMultiMap.

Hazelcast (and Infinispan) cluster manager would use that method to remove the listener.

@michalsida
Copy link
Author

I needed this method for some (a little dirty) hack of other project and I started to use it the same way (= call it only once).

tsegismont added a commit to tsegismont/vert.x that referenced this issue Jan 10, 2018
This allows cluster managers to free resources they use to maintain the state.

Needed for vert-x3/issues#316

Signed-off-by: Thomas Segismont <tsegismont@gmail.com>
tsegismont added a commit to tsegismont/vertx-hazelcast that referenced this issue Jan 10, 2018
Calling ClusterManager.getAsyncMultiMap multiple times can leak memory has the map listener is still registered.

See vert-x3/issues#316
tsegismont added a commit to tsegismont/vertx-infinispan that referenced this issue Jan 10, 2018
Calling ClusterManager.getAsyncMultiMap multiple times can leak memory as the cache listener is still registered.

See vert-x3/issues#316
@tsegismont
Copy link

@vietj I sent a PR to core as well as HZ and ISPN cluster managers.
@michalsida please take a look as well

@tsegismont
Copy link

@michalsida is this workaround still needed after the fix for vert-x3/vertx-hazelcast#90 is merged in master?

@tsegismont
Copy link

Closing as vert-x3/vertx-hazelcast#90 should remove the need for creating many multimaps.

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

No branches or pull requests

3 participants