diff --git a/spring-web/src/main/java/org/springframework/web/server/session/DefaultWebSessionManager.java b/spring-web/src/main/java/org/springframework/web/server/session/DefaultWebSessionManager.java index 50555d1dc4..bbeec287bb 100644 --- a/spring-web/src/main/java/org/springframework/web/server/session/DefaultWebSessionManager.java +++ b/spring-web/src/main/java/org/springframework/web/server/session/DefaultWebSessionManager.java @@ -78,11 +78,9 @@ public class DefaultWebSessionManager implements WebSessionManager { @Override public Mono getSession(ServerWebExchange exchange) { - return Mono.defer(() -> - retrieveSession(exchange) - .flatMap(this.getSessionStore()::updateLastAccessTime) - .switchIfEmpty(this.sessionStore.createWebSession()) - .doOnNext(session -> exchange.getResponse().beforeCommit(() -> save(exchange, session)))); + return Mono.defer(() -> retrieveSession(exchange) + .switchIfEmpty(this.sessionStore.createWebSession()) + .doOnNext(session -> exchange.getResponse().beforeCommit(() -> save(exchange, session)))); } private Mono retrieveSession(ServerWebExchange exchange) { @@ -93,11 +91,7 @@ public class DefaultWebSessionManager implements WebSessionManager { private Mono save(ServerWebExchange exchange, WebSession session) { if (session.isExpired()) { - return Mono.error(new IllegalStateException( - "Sessions are checked for expiration and have their " + - "lastAccessTime updated when first accessed during request processing. " + - "However this session is expired meaning that maxIdleTime elapsed " + - "before the call to session.save().")); + return Mono.error(new IllegalStateException("Session='" + session.getId() + "' expired.")); } if (!session.isStarted()) { 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 b8db7090b5..c6eb8e6c6d 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 @@ -45,7 +45,7 @@ public class InMemoryWebSessionStore implements WebSessionStore { private Clock clock = Clock.system(ZoneId.of("GMT")); - private final Map sessions = new ConcurrentHashMap<>(); + private final Map sessions = new ConcurrentHashMap<>(); /** @@ -77,7 +77,7 @@ public class InMemoryWebSessionStore implements WebSessionStore { @Override public Mono retrieveSession(String id) { - WebSession session = this.sessions.get(id); + InMemoryWebSession session = this.sessions.get(id); if (session == null) { return Mono.empty(); } @@ -86,6 +86,7 @@ public class InMemoryWebSessionStore implements WebSessionStore { return Mono.empty(); } else { + session.updateLastAccessTime(); return Mono.just(session); } } @@ -98,27 +99,14 @@ public class InMemoryWebSessionStore implements WebSessionStore { public Mono updateLastAccessTime(WebSession webSession) { return Mono.fromSupplier(() -> { + Assert.isInstanceOf(InMemoryWebSession.class, webSession); InMemoryWebSession session = (InMemoryWebSession) webSession; - Instant lastAccessTime = Instant.now(getClock()); - return new InMemoryWebSession(session, lastAccessTime); + session.updateLastAccessTime(); + return session; }); } - // Private methods for InMemoryWebSession - - private Mono changeSessionId(String oldId, WebSession session) { - this.sessions.remove(oldId); - this.sessions.put(session.getId(), session); - return Mono.empty(); - } - - private Mono storeSession(WebSession session) { - this.sessions.put(session.getId(), session); - return Mono.empty(); - } - - private class InMemoryWebSession implements WebSession { private final AtomicReference id; @@ -127,12 +115,13 @@ public class InMemoryWebSessionStore implements WebSessionStore { private final Instant creationTime; - private final Instant lastAccessTime; + private volatile Instant lastAccessTime; private volatile Duration maxIdleTime; private volatile boolean started; + InMemoryWebSession() { this.id = new AtomicReference<>(String.valueOf(idGenerator.generateId())); this.attributes = new ConcurrentHashMap<>(); @@ -141,14 +130,6 @@ public class InMemoryWebSessionStore implements WebSessionStore { this.maxIdleTime = Duration.ofMinutes(30); } - InMemoryWebSession(InMemoryWebSession existingSession, Instant lastAccessTime) { - this.id = existingSession.id; - this.attributes = existingSession.attributes; - this.creationTime = existingSession.creationTime; - this.lastAccessTime = lastAccessTime; - this.maxIdleTime = existingSession.maxIdleTime; - this.started = existingSession.isStarted(); // Use method (explicit or implicit start) - } @Override public String getId() { @@ -192,15 +173,22 @@ public class InMemoryWebSessionStore implements WebSessionStore { @Override public Mono changeSessionId() { - String oldId = this.id.get(); + String currentId = this.id.get(); + if (InMemoryWebSessionStore.this.sessions.remove(currentId) == null) { + return Mono.error(new IllegalStateException( + "Failed to change session id: " + currentId + + " because the Session is no longer present in the store.")); + } String newId = String.valueOf(idGenerator.generateId()); this.id.set(newId); - return InMemoryWebSessionStore.this.changeSessionId(oldId, this).doOnError(ex -> this.id.set(oldId)); + InMemoryWebSessionStore.this.sessions.put(this.getId(), this); + return Mono.empty(); } @Override public Mono save() { - return InMemoryWebSessionStore.this.storeSession(this); + InMemoryWebSessionStore.this.sessions.put(this.getId(), this); + return Mono.empty(); } @Override @@ -208,6 +196,10 @@ public class InMemoryWebSessionStore implements WebSessionStore { return (isStarted() && !this.maxIdleTime.isNegative() && Instant.now(getClock()).minus(this.maxIdleTime).isAfter(this.lastAccessTime)); } + + private void updateLastAccessTime() { + this.lastAccessTime = Instant.now(getClock()); + } } } 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 14505425ad..6de2902834 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 @@ -41,7 +41,8 @@ public interface WebSessionStore { /** * Return the WebSession for the given id. *

Note: This method should perform an expiration check, - * remove the session if it has expired and return empty. + * and if it has expired remove the session and return empty. This method + * should also update the 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 7c3cf21ce4..ac49f905c5 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 @@ -17,6 +17,7 @@ package org.springframework.web.server.session; import java.time.Clock; import java.time.Duration; +import java.time.Instant; import org.junit.Test; @@ -28,7 +29,7 @@ import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; /** - * Unit tests. + * Unit tests for {@link InMemoryWebSessionStore}. * @author Rob Winch */ public class InMemoryWebSessionStoreTests { @@ -36,28 +37,6 @@ public class InMemoryWebSessionStoreTests { private InMemoryWebSessionStore store = new InMemoryWebSessionStore(); - @Test - public void constructorWhenImplicitStartCopiedThenCopyIsStarted() { - WebSession original = this.store.createWebSession().block(); - assertNotNull(original); - original.getAttributes().put("foo", "bar"); - - WebSession copy = this.store.updateLastAccessTime(original).block(); - assertNotNull(copy); - assertTrue(copy.isStarted()); - } - - @Test - public void constructorWhenExplicitStartCopiedThenCopyIsStarted() { - WebSession original = this.store.createWebSession().block(); - assertNotNull(original); - original.start(); - - WebSession copy = this.store.updateLastAccessTime(original).block(); - assertNotNull(copy); - assertTrue(copy.isStarted()); - } - @Test public void startsSessionExplicitly() { WebSession session = this.store.createWebSession().block(); @@ -87,8 +66,27 @@ public class InMemoryWebSessionStoreTests { assertNotNull(retrieved); assertSame(session, retrieved); + // Fast-forward 31 minutes this.store.setClock(Clock.offset(this.store.getClock(), Duration.ofMinutes(31))); WebSession retrievedAgain = this.store.retrieveSession(id).block(); assertNull(retrievedAgain); } + + @Test + public void lastAccessTimeIsUpdatedOnRetrieve() throws Exception { + WebSession session1 = this.store.createWebSession().block(); + assertNotNull(session1); + String id = session1.getId(); + Instant time1 = session1.getLastAccessTime(); + session1.save(); + + // Fast-forward a few seconds + this.store.setClock(Clock.offset(this.store.getClock(), Duration.ofSeconds(5))); + + WebSession session2 = this.store.retrieveSession(id).block(); + assertNotNull(session2); + assertSame(session1, session2); + Instant time2 = session2.getLastAccessTime(); + assertTrue(time1.isBefore(time2)); + } } \ No newline at end of file