From f4db072f1d0dc4d7c6ca7205d3b2d0487c5b026c Mon Sep 17 00:00:00 2001 From: Mark Paluch Date: Wed, 7 Aug 2024 10:21:50 +0200 Subject: [PATCH 1/3] Prepare issue branch. --- pom.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pom.xml b/pom.xml index f39b1fe04c..21ef4b55c9 100644 --- a/pom.xml +++ b/pom.xml @@ -5,7 +5,7 @@ org.springframework.data spring-data-redis - 3.4.0-SNAPSHOT + 3.4.0-GH-2957-SNAPSHOT Spring Data Redis Spring Data module for Redis From b4555fbedf7312523889307d10158d18d48d3c27 Mon Sep 17 00:00:00 2001 From: Mark Paluch Date: Wed, 7 Aug 2024 10:59:47 +0200 Subject: [PATCH 2/3] Revise RedisKeyValueAdapter to support lifecycle. RedisKeyValueAdapter is now a lifecycle bean participating in Spring's SmartLifecycle support. Sticking to Lifecycle aligns with lifecycle support in RedisConnectionFactory where connections are stopped upon shutdown. Previously, RedisKeyValueAdapter stopped connections upon destroy() causing a delayed shutdown behavior that was out of sync with its RedisConnectionFactory. --- .../data/redis/core/RedisKeyValueAdapter.java | 78 ++++++++++++++++--- .../redis/core/RedisKeyValueAdapterTests.java | 8 +- .../core/RedisKeyValueAdapterUnitTests.java | 21 +++++ 3 files changed, 91 insertions(+), 16 deletions(-) diff --git a/src/main/java/org/springframework/data/redis/core/RedisKeyValueAdapter.java b/src/main/java/org/springframework/data/redis/core/RedisKeyValueAdapter.java index 926f9d7681..69338587dd 100644 --- a/src/main/java/org/springframework/data/redis/core/RedisKeyValueAdapter.java +++ b/src/main/java/org/springframework/data/redis/core/RedisKeyValueAdapter.java @@ -26,12 +26,16 @@ import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicReference; +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; + import org.springframework.beans.BeansException; import org.springframework.beans.factory.InitializingBean; import org.springframework.context.ApplicationContext; import org.springframework.context.ApplicationContextAware; import org.springframework.context.ApplicationEventPublisher; import org.springframework.context.ApplicationListener; +import org.springframework.context.SmartLifecycle; import org.springframework.core.convert.ConversionService; import org.springframework.data.keyvalue.core.AbstractKeyValueAdapter; import org.springframework.data.keyvalue.core.KeyValueAdapter; @@ -99,17 +103,19 @@ * @author Mark Paluch * @author Andrey Muchnik * @author John Blum - * @author Lucian Torje * @since 1.7 */ public class RedisKeyValueAdapter extends AbstractKeyValueAdapter - implements InitializingBean, ApplicationContextAware, ApplicationListener { + implements InitializingBean, SmartLifecycle, ApplicationContextAware, ApplicationListener { /** * Time To Live in seconds that phantom keys should live longer than the actual key. */ private static final int PHANTOM_KEY_TTL = 300; + private final Log logger = LogFactory.getLog(getClass()); + private final AtomicReference state = new AtomicReference<>(State.CREATED); + private RedisOperations redisOps; private RedisConverter converter; private @Nullable RedisMessageListenerContainer messageListenerContainer; @@ -121,6 +127,13 @@ public class RedisKeyValueAdapter extends AbstractKeyValueAdapter private @Nullable String keyspaceNotificationsConfigParameter = null; private ShadowCopy shadowCopy = ShadowCopy.DEFAULT; + /** + * Lifecycle state of this factory. + */ + enum State { + CREATED, STARTING, STARTED, STOPPING, STOPPED, DESTROYED; + } + /** * Creates new {@link RedisKeyValueAdapter} with default {@link RedisMappingContext} and default * {@link RedisCustomConversions}. @@ -202,7 +215,7 @@ public Object put(Object id, Object item, String keyspace) { && this.expirationListener.get() == null) { if (rdo.getTimeToLive() != null && rdo.getTimeToLive() > 0) { - initKeyExpirationListener(); + initKeyExpirationListener(this.messageListenerContainer); } } @@ -686,6 +699,11 @@ public void setShadowCopy(ShadowCopy shadowCopy) { this.shadowCopy = shadowCopy; } + @Override + public boolean isRunning() { + return State.STARTED.equals(this.state.get()); + } + /** * @see org.springframework.beans.factory.InitializingBean#afterPropertiesSet() * @since 1.8 @@ -696,22 +714,61 @@ public void afterPropertiesSet() { if (this.managedListenerContainer) { initMessageListenerContainer(); } + } + + @Override + public void start() { + + State current = this.state.getAndUpdate(state -> isCreatedOrStopped(state) ? State.STARTING : state); + + if (isCreatedOrStopped(current)) { + + messageListenerContainer.start(); + + if (ObjectUtils.nullSafeEquals(EnableKeyspaceEvents.ON_STARTUP, this.enableKeyspaceEvents)) { + initKeyExpirationListener(this.messageListenerContainer); + } + + this.state.set(State.STARTED); + } + } + + private static boolean isCreatedOrStopped(@Nullable State state) { + return State.CREATED.equals(state) || State.STOPPED.equals(state); + } + + @Override + public void stop() { - if (ObjectUtils.nullSafeEquals(EnableKeyspaceEvents.ON_STARTUP, this.enableKeyspaceEvents)) { - initKeyExpirationListener(); + if (state.compareAndSet(State.STARTED, State.STOPPING)) { + + KeyExpirationEventMessageListener listener = this.expirationListener.get(); + if (listener != null) { + + if (this.expirationListener.compareAndSet(listener, null)) { + try { + listener.destroy(); + } catch (Exception e) { + logger.warn("Could not destroy KeyExpirationEventMessageListener", e); + } + } + } + + messageListenerContainer.stop(); + state.set(State.STOPPED); } } public void destroy() throws Exception { - if (this.expirationListener.get() != null) { - this.expirationListener.get().destroy(); - } + stop(); if (this.managedListenerContainer && this.messageListenerContainer != null) { this.messageListenerContainer.destroy(); this.messageListenerContainer = null; } + + this.state.set(State.DESTROYED); } @Override @@ -729,13 +786,12 @@ private void initMessageListenerContainer() { this.messageListenerContainer = new RedisMessageListenerContainer(); this.messageListenerContainer.setConnectionFactory(((RedisTemplate) redisOps).getConnectionFactory()); this.messageListenerContainer.afterPropertiesSet(); - this.messageListenerContainer.start(); } - private void initKeyExpirationListener() { + private void initKeyExpirationListener(RedisMessageListenerContainer messageListenerContainer) { if (this.expirationListener.get() == null) { - MappingExpirationListener listener = new MappingExpirationListener(this.messageListenerContainer, this.redisOps, + MappingExpirationListener listener = new MappingExpirationListener(messageListenerContainer, this.redisOps, this.converter, this.shadowCopy); listener.setKeyspaceNotificationsConfigParameter(keyspaceNotificationsConfigParameter); diff --git a/src/test/java/org/springframework/data/redis/core/RedisKeyValueAdapterTests.java b/src/test/java/org/springframework/data/redis/core/RedisKeyValueAdapterTests.java index 1e23e01c6e..65e57819f3 100644 --- a/src/test/java/org/springframework/data/redis/core/RedisKeyValueAdapterTests.java +++ b/src/test/java/org/springframework/data/redis/core/RedisKeyValueAdapterTests.java @@ -82,19 +82,17 @@ void setUp() { adapter = new RedisKeyValueAdapter(template, mappingContext); adapter.setEnableKeyspaceEvents(EnableKeyspaceEvents.ON_STARTUP); adapter.afterPropertiesSet(); + adapter.start(); template.execute((RedisCallback) connection -> { connection.flushDb(); return null; }); - RedisConnection connection = template.getConnectionFactory().getConnection(); - - try { + try (RedisConnection connection = template.getConnectionFactory() + .getConnection()) { connection.setConfig("notify-keyspace-events", ""); connection.setConfig("notify-keyspace-events", "KEA"); - } finally { - connection.close(); } } diff --git a/src/test/java/org/springframework/data/redis/core/RedisKeyValueAdapterUnitTests.java b/src/test/java/org/springframework/data/redis/core/RedisKeyValueAdapterUnitTests.java index b992cb5fa4..dca24e7482 100644 --- a/src/test/java/org/springframework/data/redis/core/RedisKeyValueAdapterUnitTests.java +++ b/src/test/java/org/springframework/data/redis/core/RedisKeyValueAdapterUnitTests.java @@ -100,6 +100,7 @@ void setUp() throws Exception { adapter = new RedisKeyValueAdapter(template, context); adapter.afterPropertiesSet(); + adapter.start(); } @AfterEach @@ -153,12 +154,32 @@ void shouldInitKeyExpirationListenerOnStartup() throws Exception { adapter = new RedisKeyValueAdapter(template, context); adapter.setEnableKeyspaceEvents(EnableKeyspaceEvents.ON_STARTUP); adapter.afterPropertiesSet(); + adapter.start(); KeyExpirationEventMessageListener listener = ((AtomicReference) getField(adapter, "expirationListener")).get(); assertThat(listener).isNotNull(); } + @Test // GH-2957 + void adapterShouldBeRestartable() throws Exception { + + adapter.destroy(); + + adapter = new RedisKeyValueAdapter(template, context); + adapter.setEnableKeyspaceEvents(EnableKeyspaceEvents.ON_STARTUP); + adapter.afterPropertiesSet(); + adapter.start(); + adapter.stop(); + + assertThat(((AtomicReference) getField(adapter, "expirationListener")).get()) + .isNull(); + + adapter.start(); + assertThat(((AtomicReference) getField(adapter, "expirationListener")).get()) + .isNotNull(); + } + @Test // DATAREDIS-491 void shouldInitKeyExpirationListenerOnFirstPutWithTtl() throws Exception { From db1743c875203643aa51546a20bad6c08a9ca1b5 Mon Sep 17 00:00:00 2001 From: Mark Paluch Date: Wed, 7 Aug 2024 10:59:57 +0200 Subject: [PATCH 3/3] Polishing. Reorder methods, reformat code. --- .../KeyExpirationEventMessageListener.java | 13 +++-- .../KeyspaceEventMessageListener.java | 57 ++++++++++--------- 2 files changed, 36 insertions(+), 34 deletions(-) diff --git a/src/main/java/org/springframework/data/redis/listener/KeyExpirationEventMessageListener.java b/src/main/java/org/springframework/data/redis/listener/KeyExpirationEventMessageListener.java index f78981fe2c..7a43963ec8 100644 --- a/src/main/java/org/springframework/data/redis/listener/KeyExpirationEventMessageListener.java +++ b/src/main/java/org/springframework/data/redis/listener/KeyExpirationEventMessageListener.java @@ -29,8 +29,8 @@ * @author Christoph Strobl * @since 1.7 */ -public class KeyExpirationEventMessageListener extends KeyspaceEventMessageListener implements - ApplicationEventPublisherAware { +public class KeyExpirationEventMessageListener extends KeyspaceEventMessageListener + implements ApplicationEventPublisherAware { private static final Topic KEYEVENT_EXPIRED_TOPIC = new PatternTopic("__keyevent@*__:expired"); @@ -45,6 +45,11 @@ public KeyExpirationEventMessageListener(RedisMessageListenerContainer listenerC super(listenerContainer); } + @Override + public void setApplicationEventPublisher(ApplicationEventPublisher applicationEventPublisher) { + this.publisher = applicationEventPublisher; + } + @Override protected void doRegister(RedisMessageListenerContainer listenerContainer) { listenerContainer.addMessageListener(this, KEYEVENT_EXPIRED_TOPIC); @@ -67,8 +72,4 @@ protected void publishEvent(RedisKeyExpiredEvent event) { } } - @Override - public void setApplicationEventPublisher(ApplicationEventPublisher applicationEventPublisher) { - this.publisher = applicationEventPublisher; - } } diff --git a/src/main/java/org/springframework/data/redis/listener/KeyspaceEventMessageListener.java b/src/main/java/org/springframework/data/redis/listener/KeyspaceEventMessageListener.java index 11e21e49c1..eb61014037 100644 --- a/src/main/java/org/springframework/data/redis/listener/KeyspaceEventMessageListener.java +++ b/src/main/java/org/springframework/data/redis/listener/KeyspaceEventMessageListener.java @@ -22,6 +22,8 @@ import org.springframework.data.redis.connection.Message; import org.springframework.data.redis.connection.MessageListener; import org.springframework.data.redis.connection.RedisConnection; +import org.springframework.data.redis.connection.RedisConnectionFactory; +import org.springframework.data.redis.connection.RedisServerCommands; import org.springframework.lang.Nullable; import org.springframework.util.Assert; import org.springframework.util.ObjectUtils; @@ -40,7 +42,7 @@ public abstract class KeyspaceEventMessageListener implements MessageListener, I private final RedisMessageListenerContainer listenerContainer; - private String keyspaceNotificationsConfigParameter = "EA"; + private @Nullable String keyspaceNotificationsConfigParameter = "EA"; /** * Creates new {@link KeyspaceEventMessageListener}. @@ -53,6 +55,26 @@ public KeyspaceEventMessageListener(RedisMessageListenerContainer listenerContai this.listenerContainer = listenerContainer; } + /** + * Set the configuration string to use for {@literal notify-keyspace-events}. + * + * @param keyspaceNotificationsConfigParameter can be {@literal null}. + * @since 1.8 + */ + public void setKeyspaceNotificationsConfigParameter(@Nullable String keyspaceNotificationsConfigParameter) { + this.keyspaceNotificationsConfigParameter = keyspaceNotificationsConfigParameter; + } + + @Override + public void afterPropertiesSet() { + init(); + } + + @Override + public void destroy() throws Exception { + listenerContainer.removeMessageListener(this); + } + @Override public void onMessage(Message message, @Nullable byte[] pattern) { @@ -76,20 +98,18 @@ public void onMessage(Message message, @Nullable byte[] pattern) { */ public void init() { - if (StringUtils.hasText(keyspaceNotificationsConfigParameter)) { + RedisConnectionFactory connectionFactory = listenerContainer.getConnectionFactory(); - RedisConnection connection = listenerContainer.getConnectionFactory().getConnection(); + if (StringUtils.hasText(keyspaceNotificationsConfigParameter) && connectionFactory != null) { - try { + try (RedisConnection connection = connectionFactory.getConnection()) { - Properties config = connection.getConfig("notify-keyspace-events"); + RedisServerCommands commands = connection.serverCommands(); + Properties config = commands.getConfig("notify-keyspace-events"); if (!StringUtils.hasText(config.getProperty("notify-keyspace-events"))) { - connection.setConfig("notify-keyspace-events", keyspaceNotificationsConfigParameter); + commands.setConfig("notify-keyspace-events", keyspaceNotificationsConfigParameter); } - - } finally { - connection.close(); } } @@ -105,23 +125,4 @@ protected void doRegister(RedisMessageListenerContainer container) { listenerContainer.addMessageListener(this, TOPIC_ALL_KEYEVENTS); } - @Override - public void destroy() throws Exception { - listenerContainer.removeMessageListener(this); - } - - /** - * Set the configuration string to use for {@literal notify-keyspace-events}. - * - * @param keyspaceNotificationsConfigParameter can be {@literal null}. - * @since 1.8 - */ - public void setKeyspaceNotificationsConfigParameter(String keyspaceNotificationsConfigParameter) { - this.keyspaceNotificationsConfigParameter = keyspaceNotificationsConfigParameter; - } - - @Override - public void afterPropertiesSet() throws Exception { - init(); - } }