Ensure Redis session with immediate flush respects defaultMaxInactiveInterval

Resolves: #1409

Author: vpavic

spring-session-data-redis/src/main/java/org/springframework/session/data/redis/RedisOperationsSessionRepository.java

@@ -418,7 +418,7 @@ public RedisOperations<Object, Object> getSessionRedisOperations() {
 
 	@Override
 	public void save(RedisSession session) {
-		session.saveDelta();
+		session.save();
 		if (session.isNew()) {
 			String sessionCreatedKey = getSessionCreatedChannel(session.getId());
 			this.sessionRedisOperations.convertAndSend(sessionCreatedKey, session.delta);
@@ -516,12 +516,13 @@ public void deleteById(String sessionId) {
 
 	@Override
 	public RedisSession createSession() {
-		RedisSession redisSession = new RedisSession();
-		if (this.defaultMaxInactiveInterval != null) {
-			redisSession.setMaxInactiveInterval(
-					Duration.ofSeconds(this.defaultMaxInactiveInterval));
-		}
-		return redisSession;
+		Duration maxInactiveInterval = Duration
+				.ofSeconds((this.defaultMaxInactiveInterval != null)
+						? this.defaultMaxInactiveInterval
+						: MapSession.DEFAULT_MAX_INACTIVE_INTERVAL_SECONDS);
+		RedisSession session = new RedisSession(maxInactiveInterval);
+		session.flushImmediateIfNecessary();
+		return session;
 	}
 
 	@Override
@@ -711,14 +712,17 @@ final class RedisSession implements Session {
 		/**
 		 * Creates a new instance ensuring to mark all of the new attributes to be
 		 * persisted in the next save operation.
+		 *
+		 * @param maxInactiveInterval the amount of time that the {@link Session} should
+		 * be kept alive between client requests.
 		 */
-		RedisSession() {
+		RedisSession(Duration maxInactiveInterval) {
 			this(new MapSession());
+			this.cached.setMaxInactiveInterval(maxInactiveInterval);
 			this.delta.put(CREATION_TIME_ATTR, getCreationTime().toEpochMilli());
 			this.delta.put(MAX_INACTIVE_ATTR, (int) getMaxInactiveInterval().getSeconds());
 			this.delta.put(LAST_ACCESSED_ATTR, getLastAccessedTime().toEpochMilli());
 			this.isNew = true;
-			this.flushImmediateIfNecessary();
 		}
 
 		/**
@@ -808,7 +812,7 @@ public void removeAttribute(String attributeName) {
 
 		private void flushImmediateIfNecessary() {
 			if (RedisOperationsSessionRepository.this.redisFlushMode == RedisFlushMode.IMMEDIATE) {
-				saveDelta();
+				save();
 			}
 		}
 
@@ -817,16 +821,20 @@ private void putAndFlush(String a, Object v) {
 			this.flushImmediateIfNecessary();
 		}
 
+		private void save() {
+			saveChangeSessionId();
+			saveDelta();
+		}
+
 		/**
 		 * Saves any attributes that have been changed and updates the expiration of this
 		 * session.
 		 */
 		private void saveDelta() {
-			String sessionId = getId();
-			saveChangeSessionId(sessionId);
 			if (this.delta.isEmpty()) {
 				return;
 			}
+			String sessionId = getId();
 			getSessionBoundHashOperations(sessionId).putAll(this.delta);
 			String principalSessionKey = getSessionAttrNameKey(
 					FindByIndexNameSessionRepository.PRINCIPAL_NAME_INDEX_NAME);
@@ -859,30 +867,32 @@ private void saveDelta() {
 					.onExpirationUpdated(originalExpiration, this);
 		}
 
-		private void saveChangeSessionId(String sessionId) {
-			if (!sessionId.equals(this.originalSessionId)) {
-				if (!isNew()) {
-					String originalSessionIdKey = getSessionKey(this.originalSessionId);
-					String sessionIdKey = getSessionKey(sessionId);
-					try {
-						RedisOperationsSessionRepository.this.sessionRedisOperations
-								.rename(originalSessionIdKey, sessionIdKey);
-					}
-					catch (NonTransientDataAccessException ex) {
-						handleErrNoSuchKeyError(ex);
-					}
-					String originalExpiredKey = getExpiredKey(this.originalSessionId);
-					String expiredKey = getExpiredKey(sessionId);
-					try {
-						RedisOperationsSessionRepository.this.sessionRedisOperations
-								.rename(originalExpiredKey, expiredKey);
-					}
-					catch (NonTransientDataAccessException ex) {
-						handleErrNoSuchKeyError(ex);
-					}
+		private void saveChangeSessionId() {
+			String sessionId = getId();
+			if (sessionId.equals(this.originalSessionId)) {
+				return;
+			}
+			if (!isNew()) {
+				String originalSessionIdKey = getSessionKey(this.originalSessionId);
+				String sessionIdKey = getSessionKey(sessionId);
+				try {
+					RedisOperationsSessionRepository.this.sessionRedisOperations
+							.rename(originalSessionIdKey, sessionIdKey);
+				}
+				catch (NonTransientDataAccessException ex) {
+					handleErrNoSuchKeyError(ex);
+				}
+				String originalExpiredKey = getExpiredKey(this.originalSessionId);
+				String expiredKey = getExpiredKey(sessionId);
+				try {
+					RedisOperationsSessionRepository.this.sessionRedisOperations
+							.rename(originalExpiredKey, expiredKey);
+				}
+				catch (NonTransientDataAccessException ex) {
+					handleErrNoSuchKeyError(ex);
 				}
-				this.originalSessionId = sessionId;
 			}
+			this.originalSessionId = sessionId;
 		}
 
 		private void handleErrNoSuchKeyError(NonTransientDataAccessException ex) {

spring-session-data-redis/src/test/java/org/springframework/session/data/redis/RedisOperationsSessionRepositoryTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2014-2018 the original author or authors.
+ * Copyright 2014-2019 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.
@@ -343,7 +343,8 @@ public void saveExpired() {
 	@Test
 	public void redisSessionGetAttributes() {
 		String attrName = "attrName";
-		RedisSession session = this.redisRepository.new RedisSession();
+		RedisSession session = this.redisRepository.new RedisSession(
+				Duration.ofSeconds(MapSession.DEFAULT_MAX_INACTIVE_INTERVAL_SECONDS));
 		assertThat(session.getAttributeNames()).isEmpty();
 		session.setAttribute(attrName, "attrValue");
 		assertThat(session.getAttributeNames()).containsOnly(attrName);
@@ -757,6 +758,23 @@ public void flushModeImmediateCreate() {
 				.isEqualTo(session.getCreationTime().toEpochMilli());
 	}
 
+	@Test // gh-1409
+	public void flushModeImmediateCreateWithCustomMaxInactiveInterval() {
+		given(this.redisOperations.boundHashOps(anyString()))
+				.willReturn(this.boundHashOperations);
+		given(this.redisOperations.boundSetOps(anyString()))
+				.willReturn(this.boundSetOperations);
+		given(this.redisOperations.boundValueOps(anyString()))
+				.willReturn(this.boundValueOperations);
+		this.redisRepository.setDefaultMaxInactiveInterval(60);
+		this.redisRepository.setRedisFlushMode(RedisFlushMode.IMMEDIATE);
+		this.redisRepository.createSession();
+		Map<String, Object> delta = getDelta();
+		assertThat(delta.size()).isEqualTo(3);
+		assertThat(delta.get(RedisOperationsSessionRepository.MAX_INACTIVE_ATTR))
+				.isEqualTo(60);
+	}
+
 	@Test
 	public void flushModeImmediateSetAttribute() {
 		given(this.redisOperations.boundHashOps(anyString()))