Make InMemoryReactiveSessionRegistry updates atomic#19339
Open
junhyeong9812 wants to merge 1 commit into
Open
Conversation
saveSessionInformation added the session id outside the computeIfAbsent mapping function, and removeSessionInformation pruned the principal key with a non-atomic isEmpty()-then-remove. A concurrent save and remove for the same principal could therefore drop a newly added session. Use compute and computeIfPresent so the per-principal updates are atomic, matching the blocking SessionRegistryImpl. Closes spring-projectsgh-19338 Signed-off-by: junhyeong9812 <pickjog@gmail.com>
spring-projects-issues
added
the
status: waiting-for-triage
| (key, sessionsUsedByPrincipal) -> { | ||
| sessionsUsedByPrincipal.remove(sessionId); | ||
| return sessionsUsedByPrincipal.isEmpty() ? null : sessionsUsedByPrincipal; | ||
| }); |
Contributor
There was a problem hiding this comment.
private Set<String> addSessionToSet(String principal, String sessionId, Set<String> existing) {
if (existing == null) {
existing = new CopyOnWriteArraySet<>();
}
existing.add(sessionId);
return existing;
}
private Set<String> removeSessionFromSet(Set<String> sessions, String sessionId) {
sessions.remove(sessionId);
return sessions.isEmpty() ? null : sessions;
}
| public Mono<ReactiveSessionInformation> removeSessionInformation(String sessionId) { | ||
| return getSessionInformation(sessionId).doOnNext((sessionInformation) -> { | ||
| this.sessionById.remove(sessionId); | ||
| Set<String> sessionsUsedByPrincipal = this.sessionIdsByPrincipal.get(sessionInformation.getPrincipal()); |
Contributor
There was a problem hiding this comment.
sessionsUsedByPrincipal-> sessions
| .collectList() | ||
| .block(); | ||
| assertThat(sessions).extracting(ReactiveSessionInformation::getSessionId).contains(added); | ||
| this.sessionRegistry.removeSessionInformation(added).block(); |
Contributor
There was a problem hiding this comment.
this iterative block can be declared as a function - would be modular
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
InMemoryReactiveSessionRegistrycan lose a session whensaveSessionInformationandremoveSessionInformationrun concurrently for the same principal.Problem
The per-principal session set was updated non-atomically:
saveSessionInformationadded the session id outside thecomputeIfAbsentmapping function.removeSessionInformationpruned the principal key with a non-atomicget→isEmpty()→remove(principal).Interleaving (principal
Powns one sessionS1):S1; the set becomes empty and A is about to remove keyP.S2:computeIfAbsent(P)still sees keyP, returns the same set, and addsS2.P→ the set{S2}is orphaned;getAllSessions(P)is empty andS2is lost.The blocking
SessionRegistryImplalready avoids this by performing both updates atomically withcompute/computeIfPresent(on the sameConcurrentMapfield shape and constructor); the reactive registry did not.Fix
Mirror the blocking
SessionRegistryImpl: perform the add insidecompute, and the remove-and-prune insidecomputeIfPresent(returningnullto drop the key only while the set is empty). This makes the per-principal updates atomic.A 1000-iteration, latch-synchronized concurrent save/remove stress test is added; it fails on the previous code and passes with the fix.
Closes gh-19338