hazelcast / hazelcast-tomcat-sessionmanager

Tomcat Based Web Session Replication
Other
33 stars 37 forks source link

Sessions don't expire! #39

Closed bergander closed 6 years ago

bergander commented 6 years ago

If using sticky sessions and a node in the cluster fails, all sessions on that failing node will not expire unless the client makes a new request handled by another node.

The problem is that the findSessions() method is not implemented in HazelcastSessionManager which the expire thread in tomcat depends on. So it's falling back to the one implemented in ManagerBase which only returns the local sessions, but if no new request are made for a session then that session will never end up in a local session map.

So that method needs to return all sessions in the cluster for the sessions to be expired correctly. And I'm guessing other functionality in tomcat that uses that method also needs all sessions.

If sessions aren't expired they will stay in the hazelcast map forever, e.i. a memory leak. Also sessionDestroyed events are never triggered for those sessions.

I see two possible implementations. Either do something simple and just return all sessions in the distributed map:

@Override
public Session[] findSessions() {
    return sessionMap.values().toArray(new Session[0]);
}

Or try to do something clever to avoid deserializing sessions which are locally available:

@Override
public Session[] findSessions() {
    // Get all local sessions
    Set<Session> allSessions = new HashSet(sessions.values());

    // Get all non-local sessions ids
    Set<String> keys = sessionMap.keySet();
    keys.removeAll(sessions.keySet());

    // Add all non-local sessions
    allSessions.addAll(sessionMap.getAll(keys).values());

    return allSessions.toArray(new Session[allSessions.size()]);
}

Your thoughts about this?

emre-aydin commented 6 years ago

That looks like a real problem. I think your second solution is better. I'm not sure how frequently this method gets called but still it's not much more complicated than the first solution, so it should be OK.

bergander commented 6 years ago

@emre-aydin I've created a pull request for you to review. Seems to work fine in my tests.