Revise implementation of RedisMessageListenerContainer.

RedisMessageListenerContainer is now reimplemented using non-blocking synchronization guards and a state management to simplify its maintenances. Additionally, listener registration and subscription setup through the start() method awaits until the listener subscription is confirmed by the Redis server. The synchronization removes potential race conditions that could happen by concurrent access to blocking Redis connectors in which the registration state was guessed and not awaited.

Resolves: #964
Original Pull Request: #2256

src/test/java/org/springframework/data/redis/core/RedisKeyValueAdapterUnitTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2016-2021 the original author or authors.
+ * Copyright 2016-2022 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.
@@ -13,7 +13,6 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
-
 package org.springframework.data.redis.core;
 
 import static org.assertj.core.api.Assertions.*;
@@ -38,6 +37,7 @@ import org.mockito.quality.Strictness;
 
 import org.springframework.data.annotation.Id;
 import org.springframework.data.redis.connection.RedisConnection;
+import org.springframework.data.redis.connection.SubscriptionListener;
 import org.springframework.data.redis.connection.jedis.JedisConnectionFactory;
 import org.springframework.data.redis.core.RedisKeyValueAdapter.EnableKeyspaceEvents;
 import org.springframework.data.redis.core.convert.Bucket;
@@ -72,6 +72,22 @@ class RedisKeyValueAdapterUnitTests {
 		template.setConnectionFactory(jedisConnectionFactoryMock);
 		template.afterPropertiesSet();
 
+		doAnswer(it -> {
+
+			SubscriptionListener listener = it.getArgument(0);
+			listener.onChannelSubscribed(it.getArgument(1), 0);
+
+			return null;
+		}).when(redisConnectionMock).subscribe(any(), any());
+
+		doAnswer(it -> {
+
+			SubscriptionListener listener = it.getArgument(0);
+			listener.onPatternSubscribed(it.getArgument(1), 0);
+
+			return null;
+		}).when(redisConnectionMock).pSubscribe(any(), any());
+
 		when(jedisConnectionFactoryMock.getConnection()).thenReturn(redisConnectionMock);
 
 		Properties keyspaceEventsConfig = new Properties();

src/test/java/org/springframework/data/redis/listener/PubSubResubscribeTests.java

@@ -33,7 +33,6 @@ import org.junit.jupiter.api.BeforeEach;
 
 import org.springframework.core.task.SimpleAsyncTaskExecutor;
 import org.springframework.core.task.SyncTaskExecutor;
-import org.springframework.data.redis.SettingsUtils;
 import org.springframework.data.redis.connection.RedisConnectionFactory;
 import org.springframework.data.redis.connection.jedis.JedisConnectionFactory;
 import org.springframework.data.redis.connection.jedis.extension.JedisConnectionFactoryExtension;
@@ -77,9 +76,6 @@ public class PubSubResubscribeTests {
 
 	public static Collection<Object[]> testParams() {
 
-		int port = SettingsUtils.getPort();
-		String host = SettingsUtils.getHost();
-
 		List<RedisConnectionFactory> factories = new ArrayList<>(3);
 
 		// Jedis

src/test/java/org/springframework/data/redis/listener/PubSubTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2011-2021 the original author or authors.
+ * Copyright 2011-2022 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.
@@ -34,7 +34,6 @@ import org.junit.jupiter.api.BeforeEach;
 import org.springframework.core.task.SimpleAsyncTaskExecutor;
 import org.springframework.core.task.SyncTaskExecutor;
 import org.springframework.data.redis.ObjectFactory;
-import org.springframework.data.redis.connection.ConnectionUtils;
 import org.springframework.data.redis.connection.RedisConnectionFactory;
 import org.springframework.data.redis.connection.jedis.JedisConnectionFactory;
 import org.springframework.data.redis.connection.lettuce.LettuceConnectionFactory;
@@ -171,18 +170,14 @@ public class PubSubTests<T> {
 	}
 
 	@SuppressWarnings("unchecked")
-	@ParameterizedRedisTest // DATAREDIS-251
+	@ParameterizedRedisTest // DATAREDIS-251, GH-964
 	void testStartListenersToNoSpecificChannelTest() throws InterruptedException {
 
 		assumeThat(isClusterAware(template.getConnectionFactory())).isFalse();
-		assumeThat(ConnectionUtils.isJedis(template.getConnectionFactory())).isTrue();
 
-		PubSubAwaitUtil.runAndAwaitPatternSubscription(template.getRequiredConnectionFactory(), () -> {
-
-			container.removeMessageListener(adapter, new ChannelTopic(CHANNEL));
-			container.addMessageListener(adapter, Collections.singletonList(new PatternTopic(CHANNEL + "*")));
-			container.start();
-		});
+		container.removeMessageListener(adapter, new ChannelTopic(CHANNEL));
+		container.addMessageListener(adapter, Collections.singletonList(new PatternTopic(CHANNEL + "*")));
+		container.start();
 
 		T payload = getT();
 

src/test/java/org/springframework/data/redis/listener/RedisMessageListenerContainerFailureIntegrationTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2016-2021 the original author or authors.
+ * Copyright 2016-2022 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.
@@ -18,6 +18,7 @@ package org.springframework.data.redis.listener;
 import static org.assertj.core.api.Assertions.*;
 import static org.mockito.Mockito.*;
 
+import java.util.concurrent.CompletionException;
 import java.util.concurrent.Executor;
 
 import org.junit.jupiter.api.AfterEach;
@@ -25,11 +26,12 @@ import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 
 import org.springframework.core.task.SyncTaskExecutor;
+import org.springframework.data.redis.RedisConnectionFailureException;
 import org.springframework.data.redis.SettingsUtils;
 import org.springframework.data.redis.connection.RedisStandaloneConfiguration;
 import org.springframework.data.redis.connection.jedis.JedisConnectionFactory;
 import org.springframework.data.redis.listener.adapter.MessageListenerAdapter;
-import org.springframework.data.redis.test.extension.parametrized.MethodSource;
+import org.springframework.util.backoff.FixedBackOff;
 
 /**
  * Integration tests for {@link RedisMessageListenerContainer}.
@@ -37,7 +39,7 @@ import org.springframework.data.redis.test.extension.parametrized.MethodSource;
  * @author Mark Paluch
  * @author Christoph Strobl
  */
-class RedisMessageListenerContainerInterruptIntegrationTests {
+class RedisMessageListenerContainerFailureIntegrationTests {
 
 	private final Object handler = new Object() {
 
@@ -80,24 +82,44 @@ class RedisMessageListenerContainerInterruptIntegrationTests {
 		connectionFactory.destroy();
 	}
 
-	@Test // DATAREDIS-415
+	@Test // DATAREDIS-415, GH-964
 	void interruptAtStart() {
 
-		final Thread main = Thread.currentThread();
+		Thread main = Thread.currentThread();
 
 		// interrupt thread once Executor.execute is called
 		doAnswer(invocationOnMock -> {
 
 			main.interrupt();
-			return null;
+			throw new InterruptedException();
 		}).when(executorMock).execute(any(Runnable.class));
 
 		container.addMessageListener(adapter, new ChannelTopic("a"));
-		container.start();
+		assertThatThrownBy(() -> container.start()).isInstanceOf(CompletionException.class)
+				.hasRootCauseInstanceOf(InterruptedException.class);
 
 		// reset the interrupted flag to not destroy the teardown
-		assertThat(Thread.interrupted()).isTrue();
+		Thread.interrupted();
 
-		assertThat(container.isRunning()).isFalse();
+		assertThat(container.isRunning()).isTrue();
+		assertThat(container.isListening()).isFalse();
+	}
+
+	@Test // GH-964
+	void connectionFailureAndRetry() {
+
+		// interrupt thread once Executor.execute is called
+		doAnswer(invocationOnMock -> {
+
+			throw new RedisConnectionFailureException("I want to break free!");
+		}).when(executorMock).execute(any(Runnable.class));
+
+		container.setRecoveryBackoff(new FixedBackOff(1, 5));
+		container.addMessageListener(adapter, new ChannelTopic("a"));
+		assertThatThrownBy(() -> container.start()).isInstanceOf(CompletionException.class)
+				.hasRootCauseInstanceOf(RedisConnectionFailureException.class);
+
+		assertThat(container.isRunning()).isTrue();
+		assertThat(container.isListening()).isFalse();
 	}
 }

src/test/java/org/springframework/data/redis/listener/RedisMessageListenerContainerIntegrationTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2016-2021 the original author or authors.
+ * Copyright 2016-2022 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.
@@ -26,6 +26,7 @@ import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicInteger;
 import java.util.concurrent.atomic.AtomicReference;
 
+import org.hamcrest.Matchers;
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
 
@@ -213,6 +214,143 @@ class RedisMessageListenerContainerIntegrationTests {
 		assertThat(subscriptions1.get() + subscriptions2.get()).isGreaterThan(0);
 	}
 
+	@ParameterizedRedisTest // GH-964
+	void subscribeAfterStart() throws Exception {
+
+		AtomicInteger subscriptions1 = new AtomicInteger();
+		AtomicInteger subscriptions2 = new AtomicInteger();
+		CountDownLatch received = new CountDownLatch(2);
+
+		CompositeListener listener1 = new CompositeListener() {
+			@Override
+			public void onMessage(Message message, @Nullable byte[] pattern) {
+				received.countDown();
+			}
+
+			@Override
+			public void onPatternSubscribed(byte[] pattern, long count) {
+				subscriptions1.incrementAndGet();
+			}
+		};
+
+		CompositeListener listener2 = new CompositeListener() {
+			@Override
+			public void onMessage(Message message, @Nullable byte[] pattern) {
+				received.countDown();
+			}
+
+			@Override
+			public void onPatternSubscribed(byte[] pattern, long count) {
+				subscriptions2.incrementAndGet();
+			}
+		};
+
+		container.start();
+
+		container.addMessageListener(listener1, new PatternTopic("a"));
+		container.addMessageListener(listener2, new PatternTopic("a"));
+
+		try (RedisConnection connection = connectionFactory.getConnection()) {
+			connection.publish("a".getBytes(), "hello".getBytes());
+		}
+
+		assertThat(received.await(2, TimeUnit.SECONDS)).isTrue();
+		container.destroy();
+
+		await().until(() -> subscriptions1.get() > 0 || subscriptions2.get() > 0);
+
+		assertThat(subscriptions1.get() + subscriptions2.get()).isGreaterThan(0);
+	}
+
+	@ParameterizedRedisTest // GH-964
+	void multipleStarts() throws Exception {
+
+		AtomicInteger subscriptions = new AtomicInteger();
+		CountDownLatch received = new CountDownLatch(1);
+
+		CompositeListener listener1 = new CompositeListener() {
+			@Override
+			public void onMessage(Message message, @Nullable byte[] pattern) {
+				received.countDown();
+			}
+
+			@Override
+			public void onPatternSubscribed(byte[] pattern, long count) {
+				subscriptions.incrementAndGet();
+			}
+		};
+
+		container.start();
+		container.addMessageListener(listener1, new PatternTopic("a"));
+		container.stop();
+		container.start();
+
+		// Listeners run on a listener executor and they can be notified later
+		await().untilAtomic(subscriptions, Matchers.is(2));
+		assertThat(subscriptions.get()).isEqualTo(2);
+
+		try (RedisConnection connection = connectionFactory.getConnection()) {
+			connection.publish("a".getBytes(), "hello".getBytes());
+		}
+
+		assertThat(received.await(2, TimeUnit.SECONDS)).isTrue();
+		container.destroy();
+	}
+
+	@ParameterizedRedisTest // GH-964
+	void shouldRegisterChannelsAndTopics() throws Exception {
+
+		AtomicInteger subscriptions = new AtomicInteger();
+		CountDownLatch received = new CountDownLatch(2);
+
+		CompositeListener patternListener = new CompositeListener() {
+			@Override
+			public void onMessage(Message message, @Nullable byte[] pattern) {
+				if (message.toString().contains("pattern")) {
+					received.countDown();
+				}
+			}
+
+			@Override
+			public void onPatternSubscribed(byte[] pattern, long count) {
+				subscriptions.incrementAndGet();
+			}
+		};
+
+		CompositeListener channelListener = new CompositeListener() {
+			@Override
+			public void onMessage(Message message, @Nullable byte[] pattern) {
+				if (message.toString().contains("channel")) {
+					received.countDown();
+				}
+			}
+
+			@Override
+			public void onChannelSubscribed(byte[] channel, long count) {
+				subscriptions.incrementAndGet();
+			}
+		};
+
+		container.start();
+
+		container.addMessageListener(patternListener, new PatternTopic("a-pattern-0"));
+		container.addMessageListener(patternListener, new PatternTopic("a-pattern-1"));
+		container.addMessageListener(channelListener, new ChannelTopic("a-channel-0"));
+		container.addMessageListener(channelListener, new ChannelTopic("a-channel-1"));
+
+		// Listeners run on a listener executor and they can be notified later
+		await().untilAtomic(subscriptions, Matchers.is(4));
+		assertThat(subscriptions.get()).isEqualTo(4);
+
+		try (RedisConnection connection = connectionFactory.getConnection()) {
+			connection.publish("a-pattern-1".getBytes(), "pattern".getBytes());
+			connection.publish("a-channel-0".getBytes(), "channel".getBytes());
+		}
+
+		assertThat(received.await(2, TimeUnit.SECONDS)).isTrue();
+		container.destroy();
+	}
+
 	interface CompositeListener extends MessageListener, SubscriptionListener {
 
 	}

src/test/java/org/springframework/data/redis/listener/RedisMessageListenerContainerUnitTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2018-2021 the original author or authors.
+ * Copyright 2018-2022 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.
@@ -18,6 +18,7 @@ package org.springframework.data.redis.listener;
 import static org.assertj.core.api.Assertions.*;
 import static org.mockito.Mockito.*;
 
+import java.nio.charset.StandardCharsets;
 import java.util.concurrent.Executor;
 
 import org.junit.jupiter.api.BeforeEach;
@@ -27,7 +28,8 @@ import org.springframework.core.task.SyncTaskExecutor;
 import org.springframework.data.redis.connection.RedisConnection;
 import org.springframework.data.redis.connection.RedisConnectionFactory;
 import org.springframework.data.redis.connection.Subscription;
-import org.springframework.data.redis.connection.lettuce.LettuceConnectionFactory;
+import org.springframework.data.redis.connection.SubscriptionListener;
+import org.springframework.data.redis.connection.jedis.JedisConnectionFactory;
 import org.springframework.data.redis.listener.adapter.MessageListenerAdapter;
 
 /**
@@ -57,7 +59,7 @@ class RedisMessageListenerContainerUnitTests {
 	void setUp() {
 
 		executorMock = mock(Executor.class);
-		connectionFactoryMock = mock(LettuceConnectionFactory.class);
+		connectionFactoryMock = mock(JedisConnectionFactory.class);
 		connectionMock = mock(RedisConnection.class);
 		subscriptionMock = mock(Subscription.class);
 
@@ -85,7 +87,11 @@ class RedisMessageListenerContainerUnitTests {
 
 		doAnswer(it -> {
 
+			SubscriptionListener listener = it.getArgument(0);
 			when(connectionMock.isSubscribed()).thenReturn(true);
+
+			listener.onChannelSubscribed("a".getBytes(StandardCharsets.UTF_8), 0);
+
 			return null;
 		}).when(connectionMock).subscribe(any(), any());
 

src/test/java/org/springframework/data/redis/listener/SubscriptionConnectionTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2011-2021 the original author or authors.
+ * Copyright 2011-2022 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.
@@ -27,7 +27,6 @@ import org.junit.jupiter.api.AfterEach;
 
 import org.springframework.core.task.SimpleAsyncTaskExecutor;
 import org.springframework.core.task.SyncTaskExecutor;
-import org.springframework.data.redis.SettingsUtils;
 import org.springframework.data.redis.connection.MessageListener;
 import org.springframework.data.redis.connection.RedisConnection;
 import org.springframework.data.redis.connection.RedisConnectionFactory;
@@ -70,8 +69,6 @@ public class SubscriptionConnectionTests {
 	}
 
 	public static Collection<Object[]> testParams() {
-		int port = SettingsUtils.getPort();
-		String host = SettingsUtils.getHost();
 
 		// Jedis
 		JedisConnectionFactory jedisConnFactory = JedisConnectionFactoryExtension
@@ -93,8 +90,9 @@ public class SubscriptionConnectionTests {
 		}
 	}
 
-	@ParameterizedRedisTest
+	@ParameterizedRedisTest // GH-964
 	void testStopMessageListenerContainers() throws Exception {
+
 		// Grab all 8 connections from the pool. They should be released on
 		// container stop
 		for (int i = 0; i < 8; i++) {
@@ -108,12 +106,6 @@ public class SubscriptionConnectionTests {
 			container.afterPropertiesSet();
 			container.start();
 
-			if (connectionFactory instanceof JedisConnectionFactory) {
-				// Need to sleep shortly as jedis cannot deal propery with multiple repsonses within one connection
-				// see https://github.com/xetorthio/jedis/issues/186
-				Thread.sleep(100);
-			}
-
 			container.stop();
 			containers.add(container);
 		}
@@ -125,6 +117,7 @@ public class SubscriptionConnectionTests {
 
 	@ParameterizedRedisTest
 	void testRemoveLastListener() throws Exception {
+
 		// Grab all 8 connections from the pool
 		MessageListener listener = new MessageListenerAdapter(handler);
 		for (int i = 0; i < 8; i++) {
@@ -150,6 +143,7 @@ public class SubscriptionConnectionTests {
 
 	@ParameterizedRedisTest
 	void testStopListening() throws InterruptedException {
+
 		// Grab all 8 connections from the pool.
 		MessageListener listener = new MessageListenerAdapter(handler);
 		for (int i = 0; i < 8; i++) {
@@ -165,7 +159,7 @@ public class SubscriptionConnectionTests {
 		}
 
 		// Unsubscribe all listeners from all topics, freeing up a connection
-		containers.get(0).removeMessageListener(null, Arrays.asList(new Topic[] {}));
+		containers.get(0).stop();
 
 		// verify we can now get a connection from the pool
 		RedisConnection connection = connectionFactory.getConnection();