From 6da3518a6662fd0e6725597009acaf0b26262a16 Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Tue, 26 Sep 2017 23:11:25 -0400 Subject: [PATCH] WebSessionStore updates lastAccessTime on retrieve Now that WebSessionStore is in charge of expiration checks on retrieve it makes sense to also update the lastAccessTime on retrieve at the same time, saving the need to call it after a retrieve. Issue: SPR-15963 --- .../session/DefaultWebSessionManager.java | 14 ++--- .../session/InMemoryWebSessionStore.java | 52 ++++++++----------- .../web/server/session/WebSessionStore.java | 3 +- .../session/InMemoryWebSessionStoreTests.java | 44 ++++++++-------- 4 files changed, 49 insertions(+), 64 deletions(-) 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