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

InMemoryWebSessionStore could leak memory if sessions created but never used [SPR-17020] #21558

Closed
spring-projects-issues opened this issue Jul 9, 2018 · 7 comments
Assignees
Labels
in: web type: enhancement
Milestone

Comments

@spring-projects-issues
Copy link
Collaborator

@spring-projects-issues spring-projects-issues commented Jul 9, 2018

adrienleroy opened SPR-17020 and commented

In InMemoryWebSessionStore, expired sessions are checked and deleted when retrieveSession is called. In our case sessions are created but users never reuse the session id. So we never retrieve sessions and never clean them. We then have a memory leak.

I think that InMemoryWebSessionStore should not rely on session's creation/retrieval etc... to clean up its own sessions.


Affects: 5.0.6

Reference URL: https://jira.spring.io/browse/SPR-15963?jql=text%20~%20%22InMemoryWebSessionStore%22

Issue Links:

  • #20515 InMemoryWebSessionStore Leaks Memory
  • #21254 InMemoryWebSessionStore method to access map of sessions

Referenced from: commits 43fbd63, 6218db9, 32b7522

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jul 9, 2018

Rossen Stoyanchev commented

The checkExpiredSessions is made on every retrieve, but not more frequently than 60 seconds. In other words retrieve is used as the trigger but it doesn't matter whose session the retrieve is for (all sessions are checked). Does no one every call retrieve again?

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jul 9, 2018

adrienleroy commented

Unfortunately, yes, the load balancer is not using sticky session for the moment, and the android rest client too. We don't want to not rely on them to clear expired session.

What i did for the moment is overriding InMemoryWebSessionStore to check expired session during the creation of a new session.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jul 9, 2018

Rossen Stoyanchev commented

I just want to double check. I'm sure this is clear but just in case – a retrieve for +any+ session will trigger checkExpiredSessions. Eventually the load balancer should route someone with a session to each server, or else the number of sessions on that server is not growing.

In the end this is trying to find a convenient trigger to perform expiration checks vs managing scheduler resources. The main way I can see this becoming an issue is if there is a big spike of sessions on one server, and then no more session users to that server again. Seems like a less common scenario but possible. Even then the memory will not grow unbounded, so it's a question of how much memory can be accumulated in the mean time.

Any further specifics you want to share would help, but it seems like we need to provide more control for specific scenarios. Would a property for the frequency of checks help? If you have spikes, and then nothing at all then setting it to a low number would help.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jul 9, 2018

Rossen Stoyanchev commented

Scheduling since we'll make some improvement, just need to decide on the specifics. I lowered the priority because there is fairly straight-forward workaround.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jul 10, 2018

adrienleroy commented

You have resumed the issue well, it is a less commons scenario but possible, I see also this behaviour as a way of an malicious person to make our application crashing.

I don't think the frequency of checks would help but rather checks should be trigger by something else than creating/retrieve session.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jul 10, 2018

Rossen Stoyanchev commented

The frequency of checks would most certainly help but I think we can improve differently.

We can add an extra check on the total number of sessions created since the last expiration check. If done from the create method, that will happen on every request and should effectively put an upper limit (e.g. N = 100) on the total number of sessions that can be created before the next round of expiration checks. That will complement the time window limit currently in place.

This can also help optimize performance by reducing the frequency of current time checks from every session request, to once every Nth request, as well as help to ensure a round of expiration checks happens regardless of whether retrieve is called.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jul 12, 2018

Rossen Stoyanchev commented

I ended with hooks in create and retrieve to ensure periodic checks for any getSession() call. In addition, there is now a maxSessions to put an upper limit on the total count (10K by default but configurable). That should cover most cases I think and puts upper limits but I also added a public API removeExpiredSessions for extra flexibility if you need to force a check at a specific time. Last, there is a getSessions() method based on #21254.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web type: enhancement
Projects
None yet
Development

No branches or pull requests

2 participants