I’ve got a scenario that causes listeners registered with org.jivesoftware.smack.chat.Chat.addMessageListener() to get leaked.
I used the eclipse MAT with several java heapdumps to detect the issue. There is already a TODO in addMessageListener that warns of this as a potential issue.
It looks like that TODO has been around for awhile, and I’m interested in cleaning it up.
When I change “listeners” from a CopyOnWriteArraySet to Collections.newSetFromMap(new WeakHashMap()), my memory leak is cleared up, and there aren’t any obvious negative side effects.
I wanted to check with the community and see if there were any objections to accepting a merge with that change. I suppose it could cause some concurrency issues, but my tests have yet to reveal any.
Indeed that TODO entry is more than 10 years old: ChatState mostly code complete. · igniterealtime/Smack@88ea6cf · GitHub
But what would be the benefit of making them weak? You still need to keep track of the ChatMessageListeners you create. It appears you to that you just shift the issue a bit.
Also no other *Listeners in Smack are weak. Your memory leak is likely caused because you don’t call Chat.removeMessageListener().
Thanks for the reply. I’m understanding you to say that the weakhashmap solution is not recommended, and I respect that.
However, that leaves me struggling a little bit to perform the cleanup. I agree that chat.removeMessageListener() could resolve the issue, if we were tracking ChatMessageListeners by Chat. It also looks like we could free the listeners using Chat.close(). Unfortunately, I’m not currently tracking either ChatMessageListeners, nor Chat objects. Obviously, I could add another cross reference, but it would be nice to leverage the fact that ChatManager already tracks the chat objects.
What do you think about the addition of either a closeAllChats() or a getChats() method to ChatManager? The closeAllChats() would close all of the tracked chat objects for the ChatManager. Alternately, the getChats() would return the tracked chat objects so that we can iterate over them and call chat.close() for each one.
I’m understanding you to say that the weakhashmap solution is not recommended, and I respect that.
I did not say that it “is not recommend”. I said that it only shifts the problem. If the listeners where weak, you would end up with the exact same memory leak, just at a different place.
What do you think about the addition of either a closeAllChats() or a getChats() method to ChatManager?
Doesn’t make sense, just like having Chat.close() makes no sense. The whole Chat* API, in its current form, is not really useful IMHO (I plan to rewrite it for Smack 4.3).
But when you use it, just with like any other API: There is just no way around from implementing a proper lifecycle management of your used callbacks.
I’ve just uploaded a new version of Smack 4.2.0-rc2-SNAPSHOT which contains a new Chat API in the org.jivesoftware.smack.chat2 package found in smack-extensions. It uses resource locking, doesn’t do threads, views chats as between bare JIDs and is upgradeable for modern carbonized-based XMPP 1:1 chat (once carbons becomes a draft XEP). In other words, it’s what you should use. Enjoy, and feel free to provide feedback. I did only some basic testing of the code.