From 5847801a32bfff2b55ca89a70ac6d757b463b61c Mon Sep 17 00:00:00 2001 From: Rob Winch Date: Wed, 7 Jan 2015 14:39:10 -0600 Subject: [PATCH] RedisSessionExpirationPolicy.cleanExpiredSessions does not delete performs get The Problem: The background task that cleans up sessions can incorrectly remove a session due to a race condition. Assume an existing session with the id "1" exists and will expire at 1420656360000. This means our redis store has the following: spring:session:expirations:1420656360000 -> [1] spring:session:session:1 -> Consider the following sequence: * Thread 1 requests Session 1 and determines it should be forcibly deleted up at 1420656420000 * Thread 2 requests Session 2 and determines it should be forcibly deleted one minute later at 1420656480000 * Thread 2 removes Session 1 from 1420656360000, so it will no longer be forcibly deleted at that time spring:session:expirations:1420656360000 -> [] spring:session:session:1 -> * Thread 2 adds Session 1 to 1420656480000 spring:session:expirations:1420656360000 -> [] spring:session:session:1 -> spring:session:expirations:1420656480000 -> [1] * Thread 1 removes Session 1 (which was already removed) from 1420656360000 (the original expiration) spring:session:expirations:1420656360000 -> [] spring:session:session:1 -> spring:session:expirations:1420656480000 -> [1] * Thread 1 adds Session 1 to 1420656420000 spring:session:expirations:1420656360000 -> [] spring:session:session:1 -> spring:session:expirations:1420656480000 -> [1] spring:session:expirations:1420656420000 -> [1] Now the session is mapped to be forcibly deleted in two locations. However, at most it will be cleaned up in one location. This means that the session will be forcibly deleted even if the session is continued to be used. Fixing the Issue: Instead of deleting the session, we should have the background task access the key which will only forcibly delete the key if it is expired. This mean s that a session could at earliest be deleted when the value in the datastore indicates. This still means that a session can be deleted too soon since the incorrect TTL may be set on a key. However, at worst this is the the longest HTTP request length. Short of using distributed locking there isn't a good answer to get exact consistency. Fixes gh-93 --- docs/src/docs/asciidoc/index.adoc | 3 +++ .../RedisOperationsSessionRepository.java | 6 ++++++ .../redis/RedisSessionExpirationPolicy.java | 21 +++++++++++-------- ...RedisOperationsSessionRepositoryTests.java | 10 +++------ 4 files changed, 24 insertions(+), 16 deletions(-) diff --git a/docs/src/docs/asciidoc/index.adoc b/docs/src/docs/asciidoc/index.adoc index 233e211..e335da4 100644 --- a/docs/src/docs/asciidoc/index.adoc +++ b/docs/src/docs/asciidoc/index.adoc @@ -301,6 +301,9 @@ For example: SADD spring:session:expirations: EXPIRE spring:session:expirations: 1800 +The background task will then use these mappings to explicitly request each key. +By accessing they key, rather than deleting it, we ensure that Redis deletes the key for us only if the TTL is expired. + The Redis expiration is still placed on each key to ensure that if the server is down when the session expires, it is still cleaned up. [[api-redisoperationssessionrepository-writes]] diff --git a/spring-session/src/main/java/org/springframework/session/data/redis/RedisOperationsSessionRepository.java b/spring-session/src/main/java/org/springframework/session/data/redis/RedisOperationsSessionRepository.java index d67c381..1e30452 100644 --- a/spring-session/src/main/java/org/springframework/session/data/redis/RedisOperationsSessionRepository.java +++ b/spring-session/src/main/java/org/springframework/session/data/redis/RedisOperationsSessionRepository.java @@ -104,8 +104,14 @@ import org.springframework.util.Assert; * EXPIRE spring:session:expirations: 1800 * } * + *

+ * The background task will then use these mappings to explicitly request each key. + * By accessing they key, rather than deleting it, we ensure that Redis deletes the key for us only if the TTL is expired. + *

+ *

* The Redis expiration is still placed on each key to ensure that if the server * is down when the session expires, it is still cleaned up. + *

* * @since 1.0 * diff --git a/spring-session/src/main/java/org/springframework/session/data/redis/RedisSessionExpirationPolicy.java b/spring-session/src/main/java/org/springframework/session/data/redis/RedisSessionExpirationPolicy.java index 1f7b8af..c872703 100644 --- a/spring-session/src/main/java/org/springframework/session/data/redis/RedisSessionExpirationPolicy.java +++ b/spring-session/src/main/java/org/springframework/session/data/redis/RedisSessionExpirationPolicy.java @@ -35,7 +35,7 @@ import org.springframework.session.data.redis.RedisOperationsSessionRepository.R * order to ensure expired session events are processed in a timely fashion the * expiration (rounded to the nearest minute) is mapped to all the sessions that * expire at that time. Whenever {@link #cleanExpiredSessions()} is invoked, the - * sessions for the previous minute are then deleted explicitly. + * sessions for the previous minute are then accessed to ensure they are deleted if expired. * * In some instances the {@link #cleanExpiredSessions()} method may not be not * invoked for a specific time. For example, this may happen when a server is @@ -108,18 +108,21 @@ final class RedisSessionExpirationPolicy { String expirationKey = getExpirationKey(prevMin); Set sessionsToExpire = expirationRedisOperations.boundSetOps(expirationKey).members(); - Set keysToDelete = new HashSet(sessionsToExpire.size() + 1); - keysToDelete.add(expirationKey); + touch(expirationKey); for(String session : sessionsToExpire) { String sessionKey = getSessionKey(session); - keysToDelete.add(sessionKey); + touch(sessionKey); } + } - sessionRedisOperations.delete(keysToDelete); - - if(logger.isDebugEnabled()) { - logger.debug("The following expired Sessions were deleted " + keysToDelete); - } + /** + * By trying to access the session we only trigger a deletion if it the TTL is expired. This is done to handle + * https://github.com/spring-projects/spring-session/issues/93 + * + * @param key + */ + private void touch(String key) { + sessionRedisOperations.hasKey(key); } private long roundUpToNextMinute(long timeInMs, int inactiveIntervalInSec) { diff --git a/spring-session/src/test/java/org/springframework/session/data/redis/RedisOperationsSessionRepositoryTests.java b/spring-session/src/test/java/org/springframework/session/data/redis/RedisOperationsSessionRepositoryTests.java index d736f4a..da0c75d 100644 --- a/spring-session/src/test/java/org/springframework/session/data/redis/RedisOperationsSessionRepositoryTests.java +++ b/spring-session/src/test/java/org/springframework/session/data/redis/RedisOperationsSessionRepositoryTests.java @@ -47,8 +47,6 @@ public class RedisOperationsSessionRepositoryTests { BoundSetOperations boundSetOperations; @Captor ArgumentCaptor> delta; - @Captor - ArgumentCaptor> keys; private RedisOperationsSessionRepository redisRepository; @@ -237,19 +235,17 @@ public class RedisOperationsSessionRepositoryTests { String expiredId = "expired-id"; when(redisOperations.boundHashOps(getKey(expiredId))).thenReturn(boundHashOperations); when(redisOperations.boundSetOps(anyString())).thenReturn(boundSetOperations); + Set expiredIds = new HashSet(Arrays.asList("expired-key1","expired-key2")); when(boundSetOperations.members()).thenReturn(expiredIds); redisRepository.cleanupExpiredSessions(); - verify(redisOperations).delete(keys.capture()); - for(String id : expiredIds) { String expiredKey = RedisOperationsSessionRepository.BOUNDED_HASH_KEY_PREFIX + id; - assertThat(keys.getValue()).contains(expiredKey); + // https://github.com/spring-projects/spring-session/issues/93 + verify(redisOperations).hasKey(expiredKey); } - // the key that maps the expiration time to the expired ids - assertThat(keys.getValue().size()).isEqualTo(expiredIds.size() + 1); } @SuppressWarnings("rawtypes")