Fairly large memory leak when using Flash (can happen with other clients as well)[3.3.3/PATCH]

I am using the XIFF library to connect to an openfire server running our our website. I have noticed that over time we slowly run out of resources, and major collections help less and less. After examining the issue by looking at many heap dumps, I have determined one cause of the problem. Flash clients never send a terminating </stream> element, because they aren’t presented the opportunity (there is no reliable onClose, onShutdown, or other similar events in Flash.)

The server detects that the socket is closed instantly inside of NIOConnection, and notifies it’s listeners, but the SessionManager never sets the presence of the session to ‘unavailable’, which leads to routes being left in the RoutingTable, among other things. I could also get this to happen using the Smack API by killing the parent process and un-cleanly terminating connections.

I have created a patch that seems to resolve the issue, but I’m not sure if it is 100% correct, so it would be nice if someone else could take a look. This is in SessionManager.java, in the removeSession function:

// If the user is still available then send an unavailable presence
Presence presence = session.getPresence();
if (presence.isAvailable()) {
  Presence offline = new Presence();
  offline.setFrom(session.getAddress());
  offline.setTo(new JID(null, serverName, null));
  offline.setType(Presence.Type.unavailable);   session.setPresence( offline ); // <- Added this
  router.route(offline);
}

The added line is session.setPresence( offline );.

This seems to fix the issue for me, and helped a lot with the memory leaks, although I’m sure there could be more lying around from this situation. After applying this patch, I can register for and receive PresenceEventListener#unavailableSession events for my flash clients, whereas I couldn’t before.

Hey Ben,

That fix is not fully correct since you are not firing all the things that need to happen besides updating the presence of the client session. PresenceRouter should handle the offline Presence which in turn should be handled by the PresenceUpdateHandler. Do you think you can send me a flash client that I could use to debug this problem?

Thanks,

– Gato

Sure thing, I’ll shoot you an email, after looking at another heap dump it seems that a ton of MucUserImpl objects are being left around as well, probably due to the same issue.

Hey Ben,

Yep, that’s right. That’s another consequence of not processing the offline presence.

Thanks,

– Gato

Just sent you an email with a XIFF test client, I’ll keep working on the issue here as well. Thanks for your quick response (as always)

So, an update on this. The issue is that in the onConnectionClosed handler in SessionManager.java, the session’s state has already been set to STATUS_CLOSED. It gets set to STATUS_CLOSED in NIOConnection.close before the closeListeners are notified, so when the server tries to route the unavailable presence packet it gets a not-allowed error. If I change NIOConnection.close to the following code, everything seems to work, but this might be another incorrect fix. If it is, hopefully it helps you out somehow anyway

public void close() {
        boolean closedSuccessfully = false;
        synchronized (this) {
            if (!isClosed()) {
                try {
                    deliverRawText(flashClient ? "</flash:stream>" : "</stream:stream>", false);
                } catch (Exception e) {
                    // Ignore
                }
                ioSession.close();
                closed = true;
                closedSuccessfully = true;
            }
        }
        if (closedSuccessfully) {
            notifyCloseListeners();
            // Moved this block down below the notifyCloseListeners() call, so that the state is not STATUS_CLOSED when they
            // are notified.
            if (session != null) {
                session.setStatus(Session.STATUS_CLOSED);
            }
        }
    }

Edit: Spoke too soon about ‘everything’ working, the MUCUserImpl objects are still sticking around, but I’m sure you’ll be able to figure that one out!

Edit 2: With the above solution, I appear to be leaking SocketSessionImpl objects, so I tried this one out, and it seems to be working better. I modified PresenceRouter.route as shown below:

if (session == null || session.getStatus() == Session.STATUS_AUTHENTICATED ||
                ( session.getStatus() == Session.STATUS_CLOSED && Presence.Type.unavailable == packet.getType() ) ) {
                handle(packet);
            } else {
                packet.setTo(session.getAddress());
                packet.setFrom((JID)null);
                packet.setError(PacketError.Condition.not_authorized);
                session.process(packet);
            }

This is probably also incorrect, but I’m just trying to give any possible information I can. The way I am currently handling the MUCUserImpl leaks is by registering a PresenceEventListener like this:

public void unavailableSession(ClientSession session, Presence presence){
               if( mMucServer != null ){
            mMucServer.removeUser( session.getAddress() );
        }
    }

Did you get to take a look at this issue Gato? My hack seems to resolve the issue, but removing users manually from MUC is not something I really want to be doing. Let me know if you need any more information from me.

Hey Ben,

The patch has been included into 3.4.0 beta 2 but I have not filed any issue since besides executing the logic in another order I observed that things get cleaned up at the end. I will check what’s up with Multi User Chat.

Thanks,

– Gato