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

@@ -333,7 +333,7 @@ This allows a background task to access the potentially expired sessions to ensu
 For example:
 
 SADD spring:session:expirations:<expire-rounded-up-to-nearest-minute> <session-id>
-EXPIRE spring:session:expirations:<expire-rounded-up-to-nearest-minute> 1800
+EXPIRE spring:session:expirations:<expire-rounded-up-to-nearest-minute> 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.

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<String, String> 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<String> 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();

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);
+	}
+}