Skip to content

Commit f0d4012

Browse files
yeomin4242yeomin4242
committed
Clean Redis principal index during expiration cleanup
When Redis expiration events are unavailable, the expires key may be gone while the session hash remains available in the grace period. Clean the principal index during the existing expiration cleanup flow when that session is already expired. Addresses gh-1715. Signed-off-by: YeongJae Min <whereismysejong@naver.com>
1 parent 2d78858 commit f0d4012

3 files changed

Lines changed: 228 additions & 3 deletions

File tree

‎spring-session-data-redis/src/integration-test/java/org/springframework/session/data/redis/RedisIndexedSessionRepositoryStaticMasterReplicaITests.java‎

Lines changed: 157 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,157 @@
1+
/*
2+
* Copyright 2014-present the original author or authors.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* https://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
package org.springframework.session.data.redis;
18+
19+
import java.time.Duration;
20+
import java.util.UUID;
21+
22+
import com.redis.testcontainers.RedisContainer;
23+
import io.lettuce.core.ReadFrom;
24+
import org.junit.jupiter.api.Test;
25+
import org.testcontainers.containers.Container;
26+
import org.testcontainers.containers.Network;
27+
28+
import org.springframework.data.redis.connection.RedisStaticMasterReplicaConfiguration;
29+
import org.springframework.data.redis.connection.lettuce.LettuceClientConfiguration;
30+
import org.springframework.data.redis.connection.lettuce.LettuceConnectionFactory;
31+
import org.springframework.data.redis.core.RedisTemplate;
32+
import org.springframework.data.redis.serializer.RedisSerializer;
33+
import org.springframework.session.FindByIndexNameSessionRepository;
34+
import org.springframework.session.data.redis.RedisIndexedSessionRepository.RedisSession;
35+
36+
import static org.assertj.core.api.Assertions.assertThat;
37+
import static org.awaitility.Awaitility.await;
38+
39+
/**
40+
* Integration tests for {@link RedisIndexedSessionRepository} using
41+
* {@link RedisStaticMasterReplicaConfiguration}.
42+
*/
43+
class RedisIndexedSessionRepositoryStaticMasterReplicaITests {
44+
45+
private static final String INDEX_NAME = FindByIndexNameSessionRepository.PRINCIPAL_NAME_INDEX_NAME;
46+
47+
@Test // gh-1715
48+
void cleanUpExpiredSessionsWhenStaticMasterReplicaExpirationEventUnavailableThenRemovesPrincipalIndex()
49+
throws Exception {
50+
Network network = Network.newNetwork();
51+
RedisContainer master = redisContainer();
52+
master.withNetwork(network);
53+
master.withNetworkAliases("redis-master");
54+
RedisContainer replica = redisContainer();
55+
replica.withNetwork(network);
56+
replica.withCommand("redis-server", "--replicaof", "redis-master", "6379");
57+
try {
58+
master.start();
59+
replica.start();
60+
awaitReplica(replica);
61+
LettuceConnectionFactory connectionFactory = createStaticMasterReplicaConnectionFactory(master, replica);
62+
try {
63+
connectionFactory.afterPropertiesSet();
64+
RedisTemplate<String, Object> redis = createRedisTemplate(connectionFactory);
65+
RedisIndexedSessionRepository repository = new RedisIndexedSessionRepository(redis);
66+
String namespace = "RedisIndexedSessionRepositoryStaticMasterReplicaITests" + UUID.randomUUID();
67+
repository.setRedisKeyNamespace(namespace);
68+
repository.setDefaultMaxInactiveInterval(Duration.ofSeconds(1));
69+
70+
String principalName = "findByPrincipalNameStaticMasterReplica" + UUID.randomUUID();
71+
RedisSession session = repository.createSession();
72+
session.setAttribute(INDEX_NAME, principalName);
73+
74+
repository.save(session);
75+
76+
String sessionKey = namespace + ":sessions:" + session.getId();
77+
String expiresKey = namespace + ":sessions:expires:" + session.getId();
78+
String principalIndexKey = namespace + ":index:" + INDEX_NAME + ":" + principalName;
79+
awaitReplicaContainsSessionAndIndex(replica, sessionKey, principalIndexKey);
80+
assertThat(repository.findByIndexNameAndIndexValue(INDEX_NAME, principalName)).hasSize(1);
81+
82+
await().atMost(Duration.ofSeconds(10)).untilAsserted(() -> {
83+
assertThat(redis.hasKey(expiresKey)).isFalse();
84+
assertThat(repository.findById(session.getId())).isNull();
85+
});
86+
87+
assertThat(redis.boundSetOps(principalIndexKey).members()).contains(session.getId());
88+
assertThat(repository.findByIndexNameAndIndexValue(INDEX_NAME, principalName)).isEmpty();
89+
90+
await().atMost(Duration.ofSeconds(10)).untilAsserted(() -> {
91+
String expirationKey = namespace + ":expirations:" + roundDownMinute(System.currentTimeMillis());
92+
redis.boundSetOps(expirationKey).add("expires:" + session.getId());
93+
repository.cleanUpExpiredSessions();
94+
assertThat(redis.boundSetOps(principalIndexKey).members()).doesNotContain(session.getId());
95+
});
96+
}
97+
finally {
98+
connectionFactory.destroy();
99+
}
100+
}
101+
finally {
102+
replica.stop();
103+
master.stop();
104+
network.close();
105+
}
106+
}
107+
108+
private static RedisContainer redisContainer() {
109+
return new RedisContainer(RedisContainer.DEFAULT_IMAGE_NAME.withTag(RedisContainer.DEFAULT_TAG));
110+
}
111+
112+
private static long roundDownMinute(long timeInMillis) {
113+
long minuteInMillis = Duration.ofMinutes(1).toMillis();
114+
return timeInMillis - (timeInMillis % minuteInMillis);
115+
}
116+
117+
private static LettuceConnectionFactory createStaticMasterReplicaConnectionFactory(RedisContainer master,
118+
RedisContainer replica) {
119+
RedisStaticMasterReplicaConfiguration configuration = new RedisStaticMasterReplicaConfiguration(
120+
master.getHost(), master.getFirstMappedPort());
121+
configuration.node(replica.getHost(), replica.getFirstMappedPort());
122+
LettuceClientConfiguration clientConfiguration = LettuceClientConfiguration.builder()
123+
.readFrom(ReadFrom.REPLICA_PREFERRED)
124+
.commandTimeout(Duration.ofSeconds(5))
125+
.build();
126+
return new LettuceConnectionFactory(configuration, clientConfiguration);
127+
}
128+
129+
private static RedisTemplate<String, Object> createRedisTemplate(LettuceConnectionFactory connectionFactory) {
130+
RedisTemplate<String, Object> redisTemplate = new RedisTemplate<>();
131+
redisTemplate.setKeySerializer(RedisSerializer.string());
132+
redisTemplate.setHashKeySerializer(RedisSerializer.string());
133+
redisTemplate.setConnectionFactory(connectionFactory);
134+
redisTemplate.afterPropertiesSet();
135+
return redisTemplate;
136+
}
137+
138+
private static void awaitReplica(RedisContainer replica) {
139+
await().atMost(Duration.ofSeconds(30)).untilAsserted(() -> {
140+
Container.ExecResult result = replica.execInContainer("redis-cli", "INFO", "replication");
141+
String output = result.getStdout();
142+
assertThat(output).contains("master_link_status:up");
143+
assertThat(output.contains("role:slave") || output.contains("role:replica")).isTrue();
144+
});
145+
}
146+
147+
private static void awaitReplicaContainsSessionAndIndex(RedisContainer replica, String sessionKey,
148+
String principalIndexKey) {
149+
await().atMost(Duration.ofSeconds(30)).untilAsserted(() -> {
150+
Container.ExecResult sessionResult = replica.execInContainer("redis-cli", "EXISTS", sessionKey);
151+
Container.ExecResult indexResult = replica.execInContainer("redis-cli", "SCARD", principalIndexKey);
152+
assertThat(sessionResult.getStdout().trim()).isEqualTo("1");
153+
assertThat(indexResult.getStdout().trim()).isEqualTo("1");
154+
});
155+
}
156+
157+
}

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

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -630,6 +630,15 @@ private void cleanupPrincipalIndex(RedisSession session) {
630630
}
631631
}
632632

633+
private void cleanupPrincipalIndexForExpiredSession(String sessionId) {
634+
RedisSession session = getSession(sessionId, true);
635+
if (session == null || !session.isExpired()) {
636+
return;
637+
}
638+
cleanupPrincipalIndex(session);
639+
this.expirationStore.remove(session.getId());
640+
}
641+
633642
private void handleCreated(RedisSession session) {
634643
publishEvent(new SessionCreatedEvent(this, session));
635644
}
@@ -1066,7 +1075,12 @@ public void cleanupExpiredSessions() {
10661075
return;
10671076
}
10681077
for (Object sessionId : sessionsToExpire) {
1069-
touch(getSessionKey((String) sessionId));
1078+
String sessionIdToExpire = (String) sessionId;
1079+
boolean exists = touch(getSessionKey(sessionIdToExpire));
1080+
if (!exists && sessionIdToExpire.startsWith(SESSION_EXPIRES_PREFIX)) {
1081+
RedisIndexedSessionRepository.this.cleanupPrincipalIndexForExpiredSession(
1082+
sessionIdToExpire.substring(SESSION_EXPIRES_PREFIX.length()));
1083+
}
10701084
}
10711085
}
10721086

@@ -1076,8 +1090,8 @@ public void cleanupExpiredSessions() {
10761090
* <a href="https://github.com/spring-projects/spring-session/issues/93">gh-93</a>
10771091
* @param sessionKey the key
10781092
*/
1079-
private void touch(String sessionKey) {
1080-
RedisIndexedSessionRepository.this.sessionRedisOperations.hasKey(sessionKey);
1093+
private boolean touch(String sessionKey) {
1094+
return Boolean.TRUE.equals(RedisIndexedSessionRepository.this.sessionRedisOperations.hasKey(sessionKey));
10811095
}
10821096

10831097
String getExpirationKey(long expires) {

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

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -458,6 +458,60 @@ void cleanupExpiredSessions() {
458458
}
459459
}
460460

461+
@Test
462+
void cleanupExpiredSessionsWhenExpireKeyMissingAndSessionExpiredThenCleanupPrincipalIndex() {
463+
String expiredId = "expired-id";
464+
given(this.redisOperations.boundSetOps(anyString())).willReturn(this.boundSetOperations);
465+
given(this.boundSetOperations.members()).willReturn(Collections.singleton("expires:" + expiredId));
466+
given(this.redisOperations.hasKey(getKey("expires:" + expiredId))).willReturn(false);
467+
given(this.redisOperations.<String, Object>boundHashOps(getKey(expiredId)))
468+
.willReturn(this.boundHashOperations);
469+
Map<String, Object> map = map(RedisSessionMapper.CREATION_TIME_KEY, Instant.EPOCH.toEpochMilli(),
470+
RedisSessionMapper.MAX_INACTIVE_INTERVAL_KEY, 1, RedisSessionMapper.LAST_ACCESSED_TIME_KEY,
471+
Instant.now().minus(5, ChronoUnit.MINUTES).toEpochMilli(),
472+
RedisSessionMapper.ATTRIBUTE_PREFIX + FindByIndexNameSessionRepository.PRINCIPAL_NAME_INDEX_NAME,
473+
"principal");
474+
given(this.boundHashOperations.entries()).willReturn(map);
475+
476+
this.redisRepository.cleanUpExpiredSessions();
477+
478+
verify(this.boundSetOperations).remove(expiredId);
479+
verify(this.boundSetOperations).remove("expires:" + expiredId);
480+
}
481+
482+
@Test
483+
void cleanupExpiredSessionsWhenExpireKeyExistsThenDoesNotLookupSession() {
484+
String sessionId = "session-id";
485+
given(this.redisOperations.boundSetOps(anyString())).willReturn(this.boundSetOperations);
486+
given(this.boundSetOperations.members()).willReturn(Collections.singleton("expires:" + sessionId));
487+
given(this.redisOperations.hasKey(getKey("expires:" + sessionId))).willReturn(true);
488+
489+
this.redisRepository.cleanUpExpiredSessions();
490+
491+
verify(this.redisOperations, never()).boundHashOps(getKey(sessionId));
492+
}
493+
494+
@Test
495+
void cleanupExpiredSessionsWhenExpireKeyMissingAndSessionNotExpiredThenDoesNotCleanupPrincipalIndex() {
496+
String sessionId = "session-id";
497+
given(this.redisOperations.boundSetOps(anyString())).willReturn(this.boundSetOperations);
498+
given(this.boundSetOperations.members()).willReturn(Collections.singleton("expires:" + sessionId));
499+
given(this.redisOperations.hasKey(getKey("expires:" + sessionId))).willReturn(false);
500+
given(this.redisOperations.<String, Object>boundHashOps(getKey(sessionId)))
501+
.willReturn(this.boundHashOperations);
502+
Map<String, Object> map = map(RedisSessionMapper.CREATION_TIME_KEY, Instant.EPOCH.toEpochMilli(),
503+
RedisSessionMapper.MAX_INACTIVE_INTERVAL_KEY, 3600, RedisSessionMapper.LAST_ACCESSED_TIME_KEY,
504+
Instant.now().toEpochMilli(),
505+
RedisSessionMapper.ATTRIBUTE_PREFIX + FindByIndexNameSessionRepository.PRINCIPAL_NAME_INDEX_NAME,
506+
"principal");
507+
given(this.boundHashOperations.entries()).willReturn(map);
508+
509+
this.redisRepository.cleanUpExpiredSessions();
510+
511+
verify(this.boundSetOperations, never()).remove(sessionId);
512+
verify(this.boundSetOperations, never()).remove("expires:" + sessionId);
513+
}
514+
461515
@Test
462516
void onMessageCreated() {
463517
MapSession session = this.cached;

0 commit comments

Comments
 (0)