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

Sessions don't expire! #39

Closed
bergander opened this issue Mar 8, 2018 · 2 comments
Closed

Sessions don't expire! #39

bergander opened this issue Mar 8, 2018 · 2 comments
Milestone

Comments

@bergander
Copy link
Contributor

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

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

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

@alparslanavci alparslanavci added this to the 1.1.4 milestone Mar 21, 2019
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

No branches or pull requests

3 participants