From 222702f750b7f98ebabdefd8eea7025849ba8207 Mon Sep 17 00:00:00 2001 From: Sam Brannen <104798+sbrannen@users.noreply.github.com> Date: Tue, 10 Jun 2025 11:11:55 +0200 Subject: [PATCH 1/4] Polish WebSession support and tests --- .../web/server/ServerWebExchange.java | 14 +++---- .../session/InMemoryWebSessionStore.java | 24 ++++++----- .../web/server/session/WebSessionManager.java | 8 ++-- .../web/server/session/WebSessionStore.java | 4 +- .../session/InMemoryWebSessionStoreTests.java | 42 ++++++++++--------- 5 files changed, 48 insertions(+), 44 deletions(-) diff --git a/spring-web/src/main/java/org/springframework/web/server/ServerWebExchange.java b/spring-web/src/main/java/org/springframework/web/server/ServerWebExchange.java index da7a3bfb52..b086e62f5b 100644 --- a/spring-web/src/main/java/org/springframework/web/server/ServerWebExchange.java +++ b/spring-web/src/main/java/org/springframework/web/server/ServerWebExchange.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2023 the original author or authors. + * Copyright 2002-2025 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -107,12 +107,12 @@ public interface ServerWebExchange { } /** - * Return the web session for the current request. Always guaranteed to - * return an instance either matching to the session id requested by the - * client, or with a new session id either because the client did not - * specify one or because the underlying session had expired. Use of this - * method does not automatically create a session. See {@link WebSession} - * for more details. + * Return the web session for the current request. + *

Always guaranteed to return either an instance matching the session id + * requested by the client, or a new session either because the client did not + * specify a session id or because the underlying session expired. + *

Use of this method does not automatically create a session. See + * {@link WebSession} for more details. */ Mono getSession(); diff --git a/spring-web/src/main/java/org/springframework/web/server/session/InMemoryWebSessionStore.java b/spring-web/src/main/java/org/springframework/web/server/session/InMemoryWebSessionStore.java index 5644b332fd..f28fb0e09a 100644 --- a/spring-web/src/main/java/org/springframework/web/server/session/InMemoryWebSessionStore.java +++ b/spring-web/src/main/java/org/springframework/web/server/session/InMemoryWebSessionStore.java @@ -79,9 +79,9 @@ public class InMemoryWebSessionStore implements WebSessionStore { } /** - * Configure the {@link Clock} to use to set lastAccessTime on every created - * session and to calculate if it is expired. - *

This may be useful to align to different timezone or to set the clock + * Configure the {@link Clock} to use to set the {@code lastAccessTime} on + * every created session and to calculate if the session has expired. + *

This may be useful to align to different time zones or to set the clock * back in a test, for example, {@code Clock.offset(clock, Duration.ofMinutes(-31))} * in order to simulate session expiration. *

By default this is {@code Clock.system(ZoneId.of("GMT"))}. @@ -94,16 +94,17 @@ public class InMemoryWebSessionStore implements WebSessionStore { } /** - * Return the configured clock for session lastAccessTime calculations. + * Return the configured clock for session {@code lastAccessTime} calculations. */ public Clock getClock() { return this.clock; } /** - * Return the map of sessions with an {@link Collections#unmodifiableMap - * unmodifiable} wrapper. This could be used for management purposes, to - * list active sessions, invalidate expired ones, etc. + * Return an {@linkplain Collections#unmodifiableMap unmodifiable} copy of the + * map of sessions. + *

This could be used for management purposes, to list active sessions, + * to invalidate expired sessions, etc. * @since 5.0.8 */ public Map getSessions() { @@ -157,10 +158,11 @@ public class InMemoryWebSessionStore implements WebSessionStore { } /** - * Check for expired sessions and remove them. Typically such checks are - * kicked off lazily during calls to {@link #createWebSession() create} or - * {@link #retrieveSession retrieve}, no less than 60 seconds apart. - * This method can be called to force a check at a specific time. + * Check for expired sessions and remove them. + *

Typically such checks are kicked off lazily during calls to + * {@link #createWebSession()} or {@link #retrieveSession}, no less than 60 + * seconds apart. + *

This method can be called to force a check at a specific time. * @since 5.0.8 */ public void removeExpiredSessions() { diff --git a/spring-web/src/main/java/org/springframework/web/server/session/WebSessionManager.java b/spring-web/src/main/java/org/springframework/web/server/session/WebSessionManager.java index 67648eb4e8..88da7d186d 100644 --- a/spring-web/src/main/java/org/springframework/web/server/session/WebSessionManager.java +++ b/spring-web/src/main/java/org/springframework/web/server/session/WebSessionManager.java @@ -32,10 +32,10 @@ import org.springframework.web.server.WebSession; public interface WebSessionManager { /** - * Return the {@link WebSession} for the given exchange. Always guaranteed - * to return an instance either matching to the session id requested by the - * client, or a new session either because the client did not specify one - * or because the underlying session expired. + * Return the {@link WebSession} for the given exchange. + *

Always guaranteed to return either an instance matching the session id + * requested by the client, or a new session either because the client did not + * specify a session id or because the underlying session expired. * @param exchange the current exchange * @return promise for the WebSession */ diff --git a/spring-web/src/main/java/org/springframework/web/server/session/WebSessionStore.java b/spring-web/src/main/java/org/springframework/web/server/session/WebSessionStore.java index 15eeb12842..9a4faa3150 100644 --- a/spring-web/src/main/java/org/springframework/web/server/session/WebSessionStore.java +++ b/spring-web/src/main/java/org/springframework/web/server/session/WebSessionStore.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2022 the original author or authors. + * Copyright 2002-2025 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -43,7 +43,7 @@ public interface WebSessionStore { * Return the WebSession for the given id. *

Note: This method should perform an expiration check, * and if it has expired remove the session and return empty. This method - * should also update the lastAccessTime of retrieved sessions. + * should also update the {@code lastAccessTime} of retrieved sessions. * @param sessionId the session to load * @return the session, or an empty {@code Mono} */ diff --git a/spring-web/src/test/java/org/springframework/web/server/session/InMemoryWebSessionStoreTests.java b/spring-web/src/test/java/org/springframework/web/server/session/InMemoryWebSessionStoreTests.java index 0bf488eda7..726b3a2e53 100644 --- a/spring-web/src/test/java/org/springframework/web/server/session/InMemoryWebSessionStoreTests.java +++ b/spring-web/src/test/java/org/springframework/web/server/session/InMemoryWebSessionStoreTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2024 the original author or authors. + * Copyright 2002-2025 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -19,7 +19,6 @@ package org.springframework.web.server.session; import java.time.Clock; import java.time.Duration; import java.time.Instant; -import java.util.Map; import java.util.stream.IntStream; import org.junit.jupiter.api.Test; @@ -35,10 +34,11 @@ import static org.assertj.core.api.Assertions.assertThatIllegalStateException; * Tests for {@link InMemoryWebSessionStore}. * * @author Rob Winch + * @author Sam Brannen */ class InMemoryWebSessionStoreTests { - private InMemoryWebSessionStore store = new InMemoryWebSessionStore(); + private final InMemoryWebSessionStore store = new InMemoryWebSessionStore(); @Test @@ -59,7 +59,7 @@ class InMemoryWebSessionStoreTests { } @Test // gh-24027, gh-26958 - public void createSessionDoesNotBlock() { + void createSessionDoesNotBlock() { this.store.createWebSession() .doOnNext(session -> assertThat(Schedulers.isInNonBlockingThread()).isTrue()) .block(); @@ -103,7 +103,7 @@ class InMemoryWebSessionStoreTests { } @Test // SPR-17051 - public void sessionInvalidatedBeforeSave() { + void sessionInvalidatedBeforeSave() { // Request 1 creates session WebSession session1 = this.store.createWebSession().block(); assertThat(session1).isNotNull(); @@ -132,33 +132,31 @@ class InMemoryWebSessionStoreTests { @Test void expirationCheckPeriod() { - - DirectFieldAccessor accessor = new DirectFieldAccessor(this.store); - Map sessions = (Map) accessor.getPropertyValue("sessions"); - assertThat(sessions).isNotNull(); - // Create 100 sessions - IntStream.range(0, 100).forEach(i -> insertSession()); - assertThat(sessions).hasSize(100); + IntStream.rangeClosed(1, 100).forEach(i -> insertSession()); + assertNumSessions(100); - // Force a new clock (31 min later), don't use setter which would clean expired sessions + // Force a new clock (31 min later). Don't use setter which would clean expired sessions. + DirectFieldAccessor accessor = new DirectFieldAccessor(this.store); accessor.setPropertyValue("clock", Clock.offset(this.store.getClock(), Duration.ofMinutes(31))); - assertThat(sessions).hasSize(100); + assertNumSessions(100); - // Create 1 more which forces a time-based check (clock moved forward) + // Create 1 more which forces a time-based check (clock moved forward). insertSession(); - assertThat(sessions).hasSize(1); + assertNumSessions(1); } @Test void maxSessions() { + this.store.setMaxSessions(10); - IntStream.range(0, 10000).forEach(i -> insertSession()); - assertThatIllegalStateException().isThrownBy( - this::insertSession) - .withMessage("Max sessions limit reached: 10000"); + IntStream.rangeClosed(1, 10).forEach(i -> insertSession()); + assertThatIllegalStateException() + .isThrownBy(this::insertSession) + .withMessage("Max sessions limit reached: 10"); } + private WebSession insertSession() { WebSession session = this.store.createWebSession().block(); assertThat(session).isNotNull(); @@ -167,4 +165,8 @@ class InMemoryWebSessionStoreTests { return session; } + private void assertNumSessions(int numSessions) { + assertThat(store.getSessions()).hasSize(numSessions); + } + } From 3c265e104476d7c2ea18a36ce326febc1f3613f7 Mon Sep 17 00:00:00 2001 From: Sam Brannen <104798+sbrannen@users.noreply.github.com> Date: Tue, 10 Jun 2025 11:13:54 +0200 Subject: [PATCH 2/4] Fix InMemoryWebSessionStoreTests.startsSessionImplicitly() test --- .../web/server/session/InMemoryWebSessionStoreTests.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/spring-web/src/test/java/org/springframework/web/server/session/InMemoryWebSessionStoreTests.java b/spring-web/src/test/java/org/springframework/web/server/session/InMemoryWebSessionStoreTests.java index 726b3a2e53..7847cc3537 100644 --- a/spring-web/src/test/java/org/springframework/web/server/session/InMemoryWebSessionStoreTests.java +++ b/spring-web/src/test/java/org/springframework/web/server/session/InMemoryWebSessionStoreTests.java @@ -53,7 +53,8 @@ class InMemoryWebSessionStoreTests { void startsSessionImplicitly() { WebSession session = this.store.createWebSession().block(); assertThat(session).isNotNull(); - session.start(); + // We intentionally do not invoke start(). + // session.start(); session.getAttributes().put("foo", "bar"); assertThat(session.isStarted()).isTrue(); } From c04902fefbe54e89d423addbf8d724870cf09213 Mon Sep 17 00:00:00 2001 From: Mohammad Saeed Nouri Date: Sun, 8 Jun 2025 15:44:06 +0330 Subject: [PATCH 3/4] Allow update of existing WebSession after max sessions limit is reached Previously, when saving a WebSession, the system did not check whether the session ID already existed. As a result, even if the session being saved was an update to an existing one, it was incorrectly treated as a new session, and a "maximum sessions exceeded" error was triggered. This fix ensures that if a WebSession with the same ID already exists, it will be updated rather than counted as a new session, thereby preventing unnecessary session limit violations. Closes gh-35013 Signed-off-by: Mohammad Saeed Nouri --- .../session/InMemoryWebSessionStore.java | 2 +- .../session/InMemoryWebSessionStoreTests.java | 20 +++++++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/spring-web/src/main/java/org/springframework/web/server/session/InMemoryWebSessionStore.java b/spring-web/src/main/java/org/springframework/web/server/session/InMemoryWebSessionStore.java index f28fb0e09a..4e87a25a1b 100644 --- a/spring-web/src/main/java/org/springframework/web/server/session/InMemoryWebSessionStore.java +++ b/spring-web/src/main/java/org/springframework/web/server/session/InMemoryWebSessionStore.java @@ -283,7 +283,7 @@ public class InMemoryWebSessionStore implements WebSessionStore { private void checkMaxSessionsLimit() { if (sessions.size() >= maxSessions) { expiredSessionChecker.removeExpiredSessions(clock.instant()); - if (sessions.size() >= maxSessions) { + if (sessions.size() >= maxSessions && !sessions.containsKey(this.getId())) { throw new IllegalStateException("Max sessions limit reached: " + sessions.size()); } } diff --git a/spring-web/src/test/java/org/springframework/web/server/session/InMemoryWebSessionStoreTests.java b/spring-web/src/test/java/org/springframework/web/server/session/InMemoryWebSessionStoreTests.java index 7847cc3537..baeac73d00 100644 --- a/spring-web/src/test/java/org/springframework/web/server/session/InMemoryWebSessionStoreTests.java +++ b/spring-web/src/test/java/org/springframework/web/server/session/InMemoryWebSessionStoreTests.java @@ -23,6 +23,7 @@ import java.util.stream.IntStream; import org.junit.jupiter.api.Test; import reactor.core.scheduler.Schedulers; +import reactor.test.StepVerifier; import org.springframework.beans.DirectFieldAccessor; import org.springframework.web.server.WebSession; @@ -157,6 +158,25 @@ class InMemoryWebSessionStoreTests { .withMessage("Max sessions limit reached: 10"); } + @Test + void updateSession() { + WebSession oneWebSession = insertSession(); + + StepVerifier.create(oneWebSession.save()) + .expectComplete() + .verify(); + } + + @Test + void updateSession_whenMaxSessionsReached() { + WebSession onceWebSession = insertSession(); + IntStream.range(1, 10000).forEach(i -> insertSession()); + + StepVerifier.create(onceWebSession.save()) + .expectComplete() + .verify(); + } + private WebSession insertSession() { WebSession session = this.store.createWebSession().block(); From 4d2cc4ae9720ae70f6a2eacad14fb31cd2edf7dd Mon Sep 17 00:00:00 2001 From: Sam Brannen <104798+sbrannen@users.noreply.github.com> Date: Tue, 10 Jun 2025 11:31:30 +0200 Subject: [PATCH 4/4] Polish contribution See gh-35013 --- .../session/InMemoryWebSessionStore.java | 2 +- .../session/InMemoryWebSessionStoreTests.java | 33 +++++++++++++++---- 2 files changed, 27 insertions(+), 8 deletions(-) diff --git a/spring-web/src/main/java/org/springframework/web/server/session/InMemoryWebSessionStore.java b/spring-web/src/main/java/org/springframework/web/server/session/InMemoryWebSessionStore.java index 4e87a25a1b..d55a05df8f 100644 --- a/spring-web/src/main/java/org/springframework/web/server/session/InMemoryWebSessionStore.java +++ b/spring-web/src/main/java/org/springframework/web/server/session/InMemoryWebSessionStore.java @@ -283,7 +283,7 @@ public class InMemoryWebSessionStore implements WebSessionStore { private void checkMaxSessionsLimit() { if (sessions.size() >= maxSessions) { expiredSessionChecker.removeExpiredSessions(clock.instant()); - if (sessions.size() >= maxSessions && !sessions.containsKey(this.getId())) { + if (sessions.size() >= maxSessions && !sessions.containsKey(this.id.get())) { throw new IllegalStateException("Max sessions limit reached: " + sessions.size()); } } diff --git a/spring-web/src/test/java/org/springframework/web/server/session/InMemoryWebSessionStoreTests.java b/spring-web/src/test/java/org/springframework/web/server/session/InMemoryWebSessionStoreTests.java index baeac73d00..a1d62c9710 100644 --- a/spring-web/src/test/java/org/springframework/web/server/session/InMemoryWebSessionStoreTests.java +++ b/spring-web/src/test/java/org/springframework/web/server/session/InMemoryWebSessionStoreTests.java @@ -160,21 +160,40 @@ class InMemoryWebSessionStoreTests { @Test void updateSession() { - WebSession oneWebSession = insertSession(); + WebSession session = insertSession(); - StepVerifier.create(oneWebSession.save()) + StepVerifier.create(session.save()) .expectComplete() .verify(); } - @Test - void updateSession_whenMaxSessionsReached() { - WebSession onceWebSession = insertSession(); - IntStream.range(1, 10000).forEach(i -> insertSession()); + @Test // gh-35013 + void updateSessionAfterMaxSessionLimitIsExceeded() { + this.store.setMaxSessions(10); - StepVerifier.create(onceWebSession.save()) + WebSession session = insertSession(); + assertNumSessions(1); + + IntStream.rangeClosed(1, 9).forEach(i -> insertSession()); + assertNumSessions(10); + + // Updating an existing session should succeed. + StepVerifier.create(session.save()) .expectComplete() .verify(); + assertNumSessions(10); + + // Saving an additional new session should fail. + assertThatIllegalStateException() + .isThrownBy(this::insertSession) + .withMessage("Max sessions limit reached: 10"); + assertNumSessions(10); + + // Updating an existing session again should still succeed. + StepVerifier.create(session.save()) + .expectComplete() + .verify(); + assertNumSessions(10); }