From 23afc1b3546750c0c6bd5f8220b6a1ac8aee5d5a Mon Sep 17 00:00:00 2001 From: Rob Winch Date: Wed, 15 Apr 2015 15:25:50 -0500 Subject: [PATCH] Fix RedisSessionExpirationPolicy to properly cleanup expired sessions Previously forcibly cleaning up sessions was not working. All the cleanup was done by Redis expiration. This meant that sessions would be kept alive until Redis cleaned them up (non deterministic). This commit resolves the mapping of expiration to session ids. Fixes gh-169 --- docs/src/docs/asciidoc/index.adoc | 2 +- .../redis/RedisSessionExpirationPolicy.java | 44 ++++---- .../RedisSessionExpirationPolicyTests.java | 102 ++++++++++++++++++ 3 files changed, 128 insertions(+), 20 deletions(-) create mode 100644 spring-session/src/test/java/org/springframework/session/data/redis/RedisSessionExpirationPolicyTests.java diff --git a/docs/src/docs/asciidoc/index.adoc b/docs/src/docs/asciidoc/index.adoc index 8ea48a0..ac6fd83 100644 --- a/docs/src/docs/asciidoc/index.adoc +++ b/docs/src/docs/asciidoc/index.adoc @@ -333,7 +333,7 @@ This allows a background task to access the potentially expired sessions to ensu For example: SADD spring:session:expirations: -EXPIRE spring:session:expirations: 1800 +EXPIRE spring:session:expirations: 1860 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. 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 0bed91f..90c9094 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 @@ -22,6 +22,7 @@ import java.util.concurrent.TimeUnit; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; +import org.springframework.data.redis.core.BoundSetOperations; import org.springframework.data.redis.core.RedisOperations; import org.springframework.session.ExpiringSession; import org.springframework.session.data.redis.RedisOperationsSessionRepository.RedisSession; @@ -65,36 +66,36 @@ final class RedisSessionExpirationPolicy { } public void onDelete(ExpiringSession session) { - long lastAccessedTime = session.getLastAccessedTime(); - int maxInactiveInterval = session.getMaxInactiveIntervalInSeconds(); - - long toExpire = roundUpToNextMinute(lastAccessedTime, maxInactiveInterval); + long toExpire = roundUpToNextMinute(expiresInMillis(session)); String expireKey = getExpirationKey(toExpire); expirationRedisOperations.boundSetOps(expireKey).remove(session.getId()); } - public void onExpirationUpdated(Long originalExpirationTime, ExpiringSession session) { - if(originalExpirationTime != null) { - String expireKey = getExpirationKey(originalExpirationTime); + public void onExpirationUpdated(Long originalExpirationTimeInMilli, ExpiringSession session) { + if(originalExpirationTimeInMilli != null) { + long originalRoundedUp = roundUpToNextMinute(originalExpirationTimeInMilli); + String expireKey = getExpirationKey(originalRoundedUp); expirationRedisOperations.boundSetOps(expireKey).remove(session.getId()); } - long toExpire = roundUpToNextMinute(session.getLastAccessedTime(), session.getMaxInactiveIntervalInSeconds()); + long toExpire = roundUpToNextMinute(expiresInMillis(session)); String expireKey = getExpirationKey(toExpire); - expirationRedisOperations.boundSetOps(expireKey).add(session.getId()); + BoundSetOperations expireOperations = expirationRedisOperations.boundSetOps(expireKey); + expireOperations.add(session.getId()); - long redisExpirationInSeconds = session.getMaxInactiveIntervalInSeconds(); + long sessionExpireInSeconds = session.getMaxInactiveIntervalInSeconds(); String sessionKey = getSessionKey(session.getId()); - expirationRedisOperations.boundSetOps(expireKey).expire(redisExpirationInSeconds, TimeUnit.SECONDS); - sessionRedisOperations.boundHashOps(sessionKey).expire(redisExpirationInSeconds, TimeUnit.SECONDS); + + expireOperations.expire(sessionExpireInSeconds + 60, TimeUnit.SECONDS); + sessionRedisOperations.boundHashOps(sessionKey).expire(sessionExpireInSeconds, TimeUnit.SECONDS); } - private String getExpirationKey(long expires) { + String getExpirationKey(long expires) { return EXPIRATION_BOUNDED_HASH_KEY_PREFIX + expires; } - private String getSessionKey(String sessionId) { + String getSessionKey(String sessionId) { return RedisOperationsSessionRepository.BOUNDED_HASH_KEY_PREFIX + sessionId; } @@ -108,7 +109,7 @@ final class RedisSessionExpirationPolicy { String expirationKey = getExpirationKey(prevMin); Set sessionsToExpire = expirationRedisOperations.boundSetOps(expirationKey).members(); - touch(expirationKey); + expirationRedisOperations.delete(expirationKey); for(String session : sessionsToExpire) { String sessionKey = getSessionKey(session); touch(sessionKey); @@ -125,20 +126,25 @@ final class RedisSessionExpirationPolicy { sessionRedisOperations.hasKey(key); } - private long roundUpToNextMinute(long timeInMs, int inactiveIntervalInSec) { + static long expiresInMillis(ExpiringSession session) { + int maxInactiveInSeconds = session.getMaxInactiveIntervalInSeconds(); + long lastAccessedTimeInMillis = session.getLastAccessedTime(); + return lastAccessedTimeInMillis + TimeUnit.SECONDS.toMillis(maxInactiveInSeconds); + } + + static long roundUpToNextMinute(long timeInMs) { Calendar date = Calendar.getInstance(); - date.setTimeInMillis(timeInMs + TimeUnit.SECONDS.toMillis(inactiveIntervalInSec)); + date.setTimeInMillis(timeInMs); date.add(Calendar.MINUTE, 1); date.clear(Calendar.SECOND); date.clear(Calendar.MILLISECOND); return date.getTimeInMillis(); } - private long roundDownMinute(long timeInMs) { + static long roundDownMinute(long timeInMs) { Calendar date = Calendar.getInstance(); date.setTimeInMillis(timeInMs); - date.add(Calendar.MINUTE, -1); date.clear(Calendar.SECOND); date.clear(Calendar.MILLISECOND); return date.getTimeInMillis(); diff --git a/spring-session/src/test/java/org/springframework/session/data/redis/RedisSessionExpirationPolicyTests.java b/spring-session/src/test/java/org/springframework/session/data/redis/RedisSessionExpirationPolicyTests.java new file mode 100644 index 0000000..f7ffd73 --- /dev/null +++ b/spring-session/src/test/java/org/springframework/session/data/redis/RedisSessionExpirationPolicyTests.java @@ -0,0 +1,102 @@ +/* + * Copyright 2002-2015 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. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.springframework.session.data.redis; + +import static org.mockito.Mockito.*; + +import java.util.concurrent.TimeUnit; + +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.Mock; +import org.mockito.runners.MockitoJUnitRunner; +import org.springframework.data.redis.core.BoundHashOperations; +import org.springframework.data.redis.core.BoundSetOperations; +import org.springframework.data.redis.core.RedisOperations; +import org.springframework.session.MapSession; + +/** + * @author Rob Winch + */ +@RunWith(MockitoJUnitRunner.class) +@SuppressWarnings({"rawtypes","unchecked"}) +public class RedisSessionExpirationPolicyTests { + // Wed Apr 15 10:28:32 CDT 2015 + final static Long NOW = 1429111712346L; + + // Wed Apr 15 10:27:32 CDT 2015 + final static Long ONE_MINUTE_AGO = 1429111652346L; + + @Mock + RedisOperations sessionRedisOperations; + @Mock + BoundSetOperations setOperations; + @Mock + BoundHashOperations hashOperations; + + RedisSessionExpirationPolicy policy; + + private MapSession session; + + @Before + public void setup() { + policy = new RedisSessionExpirationPolicy(sessionRedisOperations); + session = new MapSession(); + session.setLastAccessedTime(1429116694665L); + session.setId("12345"); + + when(sessionRedisOperations.boundSetOps(anyString())).thenReturn(setOperations); + when(sessionRedisOperations.boundHashOps(anyString())).thenReturn(hashOperations); + } + + // gh-169 + @Test + public void onExpirationUpdatedRemovesOriginalExpirationTimeRoundedUp() throws Exception { + long originalExpirationTimeInMs = ONE_MINUTE_AGO; + long originalRoundedToNextMinInMs = RedisSessionExpirationPolicy.roundUpToNextMinute(originalExpirationTimeInMs); + String originalExpireKey = policy.getExpirationKey(originalRoundedToNextMinInMs); + + policy.onExpirationUpdated(originalExpirationTimeInMs, session); + + // verify the original is removed + verify(sessionRedisOperations).boundSetOps(originalExpireKey); + verify(setOperations).remove(session.getId()); + } + + @Test + public void onExpirationUpdatedAddsExpirationTimeRoundedUp() throws Exception { + long expirationTimeInMs = RedisSessionExpirationPolicy.expiresInMillis(session); + long expirationRoundedUpInMs = RedisSessionExpirationPolicy.roundUpToNextMinute(expirationTimeInMs); + String expectedExpireKey = policy.getExpirationKey(expirationRoundedUpInMs); + + policy.onExpirationUpdated(null, session); + + verify(sessionRedisOperations).boundSetOps(expectedExpireKey); + verify(setOperations).add(session.getId()); + verify(setOperations).expire(session.getMaxInactiveIntervalInSeconds() + 60, TimeUnit.SECONDS); + } + + @Test + public void onExpirationUpdatedSetExpireSession() throws Exception { + String sessionKey = policy.getSessionKey(session.getId()); + + policy.onExpirationUpdated(null, session); + + verify(sessionRedisOperations).boundHashOps(sessionKey); + verify(hashOperations).expire(session.getMaxInactiveIntervalInSeconds(), TimeUnit.SECONDS); + } +}