Skip to content

Memory leak in Index for principal null #1988

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

Merged
merged 1 commit into from
Jan 19, 2022
Merged

Memory leak in Index for principal null #1988

merged 1 commit into from
Jan 19, 2022

Conversation

ruslanys
Copy link
Contributor

I believe this should be a solution for the reported issue.

The explanation is the following. There are only two places where we add sessionId into an index: first, second.

And the first one is under the validation: if (principal != null).
So, we do not create an index if a principal is null.

When a session changes its ID, we try to change it in the index as well.
But we do not check here that the principal is null. So, we do not check if the index exists.

In the end, instead of replacing sessionId in Index, we create a new one. And this leads to a memory leak.

Close #1987

@ruslanys
Copy link
Contributor Author

ruslanys commented Dec 28, 2021

The bug goes from #1941 (eb9f62a)

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jan 4, 2022
@quaff
Copy link
Contributor

quaff commented Jan 10, 2022

Verified this PR will fix unexpected key spring:session:index:org.springframework.session.FindByIndexNameSessionRepository.PRINCIPAL_NAME_INDEX_NAME:null creation.

@eleftherias eleftherias added in: redis status: duplicate A duplicate of another issue type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels Jan 19, 2022
@eleftherias eleftherias added this to the 2.7.0-RC1 milestone Jan 19, 2022
@eleftherias eleftherias self-assigned this Jan 19, 2022
Copy link
Contributor

@eleftherias eleftherias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @ruslanys!
The changes look good to me.
Could you please squash the commits into one, and add a descriptive commit message, for example

Fix memory leak with null principal in Redis

Closes gh-1987

@ruslanys
Copy link
Contributor Author

@eleftherias Thank you for the feedback! Sure. Will do right now.

@ruslanys ruslanys requested a review from eleftherias January 19, 2022 16:09
@eleftherias eleftherias merged commit 81bd6bd into spring-projects:main Jan 19, 2022
@eleftherias
Copy link
Contributor

Thanks @ruslanys!
I have merged this into the main branch and I will also be backporting the fix to the 2.6.x branch, making it available in 2.6.2.

@ruslanys
Copy link
Contributor Author

Thank you @eleftherias !

@jebeaudet
Copy link

jebeaudet commented Jan 19, 2022

@eleftherias Since this regression also reached the 2.5.x branch via #1941, could you backport it there too please? I can open a PR if you prefer.

@eleftherias
Copy link
Contributor

Thanks for pointing that out @jebeaudet. I will backport it to 2.5.x as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: redis status: duplicate A duplicate of another issue type: bug A general bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Memory leak in Index for principal null in Redis
5 participants