From fc4964f8d5fcc2b2baadddf7c1c3be3cf10252d9 Mon Sep 17 00:00:00 2001 From: junhyeong9812 Date: Sun, 14 Jun 2026 19:15:25 +0900 Subject: [PATCH] Make InMemoryReactiveSessionRegistry updates atomic 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 gh-19338 Signed-off-by: junhyeong9812 --- .../InMemoryReactiveSessionRegistry.java | 27 ++++++--- .../InMemoryReactiveSessionRegistryTests.java | 55 +++++++++++++++++++ 2 files changed, 73 insertions(+), 9 deletions(-) diff --git a/core/src/main/java/org/springframework/security/core/session/InMemoryReactiveSessionRegistry.java b/core/src/main/java/org/springframework/security/core/session/InMemoryReactiveSessionRegistry.java index 3827e0fcf42..fbe3d63349c 100644 --- a/core/src/main/java/org/springframework/security/core/session/InMemoryReactiveSessionRegistry.java +++ b/core/src/main/java/org/springframework/security/core/session/InMemoryReactiveSessionRegistry.java @@ -59,8 +59,16 @@ public Flux getAllSessions(Object principal) { @Override public Mono saveSessionInformation(ReactiveSessionInformation information) { this.sessionById.put(information.getSessionId(), information); - this.sessionIdsByPrincipal.computeIfAbsent(information.getPrincipal(), (key) -> new CopyOnWriteArraySet<>()) - .add(information.getSessionId()); + // Add the session id inside the compute so that it cannot race with the key + // removal performed by removeSessionInformation (which could otherwise drop a + // concurrently added session). This mirrors the blocking SessionRegistryImpl. + this.sessionIdsByPrincipal.compute(information.getPrincipal(), (key, sessionsUsedByPrincipal) -> { + if (sessionsUsedByPrincipal == null) { + sessionsUsedByPrincipal = new CopyOnWriteArraySet<>(); + } + sessionsUsedByPrincipal.add(information.getSessionId()); + return sessionsUsedByPrincipal; + }); return Mono.empty(); } @@ -73,13 +81,14 @@ public Mono getSessionInformation(String sessionId) public Mono removeSessionInformation(String sessionId) { return getSessionInformation(sessionId).doOnNext((sessionInformation) -> { this.sessionById.remove(sessionId); - Set sessionsUsedByPrincipal = this.sessionIdsByPrincipal.get(sessionInformation.getPrincipal()); - if (sessionsUsedByPrincipal != null) { - sessionsUsedByPrincipal.remove(sessionId); - if (sessionsUsedByPrincipal.isEmpty()) { - this.sessionIdsByPrincipal.remove(sessionInformation.getPrincipal()); - } - } + // Remove and prune atomically so the principal key is dropped only while its + // set is empty; otherwise a session added concurrently could be lost. Mirrors + // the blocking SessionRegistryImpl. + this.sessionIdsByPrincipal.computeIfPresent(sessionInformation.getPrincipal(), + (key, sessionsUsedByPrincipal) -> { + sessionsUsedByPrincipal.remove(sessionId); + return sessionsUsedByPrincipal.isEmpty() ? null : sessionsUsedByPrincipal; + }); }); } diff --git a/web/src/test/java/org/springframework/security/web/server/authentication/session/InMemoryReactiveSessionRegistryTests.java b/web/src/test/java/org/springframework/security/web/server/authentication/session/InMemoryReactiveSessionRegistryTests.java index 286ff672c83..bda41a63009 100644 --- a/web/src/test/java/org/springframework/security/web/server/authentication/session/InMemoryReactiveSessionRegistryTests.java +++ b/web/src/test/java/org/springframework/security/web/server/authentication/session/InMemoryReactiveSessionRegistryTests.java @@ -20,6 +20,10 @@ import java.time.LocalDate; import java.time.ZoneOffset; import java.util.List; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; import org.junit.jupiter.api.Test; @@ -103,4 +107,55 @@ void updateLastAccessTimeThenUpdated() { assertThat(saved.getLastAccessTime()).isAfter(lastAccessTimeBefore); } + // Removing the last session of a principal must not race with a concurrent save for + // the same principal. Previously remove pruned the principal key with a non-atomic + // isEmpty()-then-remove, which could drop a session added concurrently. + @Test + void saveAndRemoveConcurrentlyThenAddedSessionNotLost() throws Exception { + Authentication authentication = TestAuthentication.authenticatedUser(); + Object principal = authentication.getPrincipal(); + ExecutorService executor = Executors.newFixedThreadPool(2); + try { + for (int i = 0; i < 1000; i++) { + String existing = "existing-" + i; + String added = "added-" + i; + this.sessionRegistry + .saveSessionInformation(new ReactiveSessionInformation(principal, existing, this.now)) + .block(); + CountDownLatch start = new CountDownLatch(1); + Future remove = executor.submit(() -> { + awaitUninterruptibly(start); + this.sessionRegistry.removeSessionInformation(existing).block(); + }); + Future save = executor.submit(() -> { + awaitUninterruptibly(start); + this.sessionRegistry + .saveSessionInformation(new ReactiveSessionInformation(principal, added, this.now)) + .block(); + }); + start.countDown(); + remove.get(); + save.get(); + List sessions = this.sessionRegistry.getAllSessions(principal) + .collectList() + .block(); + assertThat(sessions).extracting(ReactiveSessionInformation::getSessionId).contains(added); + this.sessionRegistry.removeSessionInformation(added).block(); + } + } + finally { + executor.shutdownNow(); + } + } + + private static void awaitUninterruptibly(CountDownLatch latch) { + try { + latch.await(); + } + catch (InterruptedException ex) { + Thread.currentThread().interrupt(); + throw new RuntimeException(ex); + } + } + }