From cc41ea527109e6af3bc080f0de3430f97eb94066 Mon Sep 17 00:00:00 2001 From: Vedran Pavic Date: Mon, 13 May 2019 19:06:35 +0200 Subject: [PATCH] Ensure Redis session with immediate flush respects defaultMaxInactiveInterval Resolves: #1409 --- .../RedisOperationsSessionRepository.java | 80 +++++++++++-------- ...RedisOperationsSessionRepositoryTests.java | 22 ++++- 2 files changed, 65 insertions(+), 37 deletions(-) diff --git a/spring-session-data-redis/src/main/java/org/springframework/session/data/redis/RedisOperationsSessionRepository.java b/spring-session-data-redis/src/main/java/org/springframework/session/data/redis/RedisOperationsSessionRepository.java index f2e5a048..2be2796d 100644 --- a/spring-session-data-redis/src/main/java/org/springframework/session/data/redis/RedisOperationsSessionRepository.java +++ b/spring-session-data-redis/src/main/java/org/springframework/session/data/redis/RedisOperationsSessionRepository.java @@ -418,7 +418,7 @@ public class RedisOperationsSessionRepository implements @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 class RedisOperationsSessionRepository implements @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 @@ public class RedisOperationsSessionRepository implements /** * 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 class RedisOperationsSessionRepository implements private void flushImmediateIfNecessary() { if (RedisOperationsSessionRepository.this.redisFlushMode == RedisFlushMode.IMMEDIATE) { - saveDelta(); + save(); } } @@ -817,16 +821,20 @@ public class RedisOperationsSessionRepository implements 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 @@ public class RedisOperationsSessionRepository implements .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); - } - } - this.originalSessionId = sessionId; + 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; } private void handleErrNoSuchKeyError(NonTransientDataAccessException ex) { diff --git a/spring-session-data-redis/src/test/java/org/springframework/session/data/redis/RedisOperationsSessionRepositoryTests.java b/spring-session-data-redis/src/test/java/org/springframework/session/data/redis/RedisOperationsSessionRepositoryTests.java index cb144c4f..17d66190 100644 --- a/spring-session-data-redis/src/test/java/org/springframework/session/data/redis/RedisOperationsSessionRepositoryTests.java +++ b/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 class RedisOperationsSessionRepositoryTests { @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 class RedisOperationsSessionRepositoryTests { .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 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()))