Polishing.

Dispose ClusterCommandExecutor before disposing connection provider. Extract RedisClient disposal to method.

Use AtomicBoolean guard instead of state and Lifecycle to dispose LettucePoolingConnectionProvider.

See #3100
Original pull request: #3164

Author: mp911de

src/main/java/org/springframework/data/redis/connection/jedis/JedisConnectionFactory.java

@@ -761,38 +761,16 @@ public void stop() {
 		if (this.state.compareAndSet(State.STARTED, State.STOPPING)) {
 
 			if (getUsePool() && !isRedisClusterAware()) {
-				if (this.pool != null) {
-					try {
-						this.pool.close();
-						this.pool = null;
-					} catch (Exception ex) {
-						log.warn("Cannot properly close Jedis pool", ex);
-					}
-				}
-			}
-
-			ClusterCommandExecutor clusterCommandExecutor = this.clusterCommandExecutor;
-
-			if (clusterCommandExecutor != null) {
-				try {
-					clusterCommandExecutor.destroy();
-					this.clusterCommandExecutor = null;
-				} catch (Exception ex) {
-					throw new RuntimeException(ex);
-				}
+				dispose(pool);
+				pool = null;
 			}
 
-			if (this.cluster != null) {
+			dispose(clusterCommandExecutor);
+			clusterCommandExecutor = null;
 
-				this.topologyProvider = null;
-
-				try {
-					this.cluster.close();
-					this.cluster = null;
-				} catch (Exception ex) {
-					log.warn("Cannot properly close Jedis cluster", ex);
-				}
-			}
+			dispose(cluster);
+			topologyProvider = null;
+			cluster = null;
 
 			this.state.set(State.STOPPED);
 		}
@@ -882,6 +860,36 @@ public void destroy() {
 		state.set(State.DESTROYED);
 	}
 
+	private void dispose(@Nullable ClusterCommandExecutor commandExecutor) {
+		if (commandExecutor != null) {
+			try {
+				commandExecutor.destroy();
+			} catch (Exception ex) {
+				log.warn("Cannot properly close cluster command executor", ex);
+			}
+		}
+	}
+
+	private void dispose(@Nullable JedisCluster cluster) {
+		if (cluster != null) {
+			try {
+				cluster.close();
+			} catch (Exception ex) {
+				log.warn("Cannot properly close Jedis cluster", ex);
+			}
+		}
+	}
+
+	private void dispose(@Nullable Pool<Jedis> pool) {
+		if (pool != null) {
+			try {
+				pool.close();
+			} catch (Exception ex) {
+				log.warn("Cannot properly close Jedis pool", ex);
+			}
+		}
+	}
+
 	@Override
 	public RedisConnection getConnection() {
 

src/main/java/org/springframework/data/redis/connection/lettuce/LettuceConnectionFactory.java

@@ -974,33 +974,23 @@ public void stop() {
 
 			resetConnection();
 
+			dispose(clusterCommandExecutor);
+			clusterCommandExecutor = null;
+
 			dispose(connectionProvider);
 			connectionProvider = null;
 
 			dispose(reactiveConnectionProvider);
 			reactiveConnectionProvider = null;
 
-			dispose(clusterCommandExecutor);
-			clusterCommandExecutor = null;
-
-			if (client != null) {
-				try {
-					Duration quietPeriod = clientConfiguration.getShutdownQuietPeriod();
-					Duration timeout = clientConfiguration.getShutdownTimeout();
-
-					client.shutdown(quietPeriod.toMillis(), timeout.toMillis(), TimeUnit.MILLISECONDS);
-					client = null;
-				} catch (Exception ex) {
-					if (log.isWarnEnabled()) {
-						log.warn(ClassUtils.getShortName(client.getClass()) + " did not shut down gracefully.", ex);
-					}
-				}
-			}
+			dispose(client);
+			client = null;
 		}
 
 		state.set(State.STOPPED);
 	}
 
+
 	@Override
 	public boolean isRunning() {
 		return State.STARTED.equals(this.state.get());
@@ -1016,11 +1006,22 @@ public void afterPropertiesSet() {
 
 	@Override
 	public void destroy() {
-		stop();
 
+		stop();
 		this.state.set(State.DESTROYED);
 	}
 
+	private void dispose(@Nullable ClusterCommandExecutor commandExecutor) {
+
+		if (commandExecutor != null) {
+			try {
+				commandExecutor.destroy();
+			} catch (Exception ex) {
+				log.warn("Cannot properly close cluster command executor", ex);
+			}
+		}
+	}
+
 	private void dispose(@Nullable LettuceConnectionProvider connectionProvider) {
 
 		if (connectionProvider instanceof DisposableBean disposableBean) {
@@ -1034,12 +1035,18 @@ private void dispose(@Nullable LettuceConnectionProvider connectionProvider) {
 		}
 	}
 
-	private void dispose(@Nullable ClusterCommandExecutor commandExecutor) {
-		if (commandExecutor != null) {
+	private void dispose(@Nullable AbstractRedisClient client) {
+
+		if (client != null) {
 			try {
-				commandExecutor.destroy();
+				Duration quietPeriod = clientConfiguration.getShutdownQuietPeriod();
+				Duration timeout = clientConfiguration.getShutdownTimeout();
+
+				client.shutdown(quietPeriod.toMillis(), timeout.toMillis(), TimeUnit.MILLISECONDS);
 			} catch (Exception ex) {
-				log.warn("Cannot properly close cluster command executor", ex);
+				if (log.isWarnEnabled()) {
+					log.warn(ClassUtils.getShortName(client.getClass()) + " did not shut down gracefully.", ex);
+				}
 			}
 		}
 	}

src/main/java/org/springframework/data/redis/connection/lettuce/LettucePoolingConnectionProvider.java

@@ -30,14 +30,14 @@
 import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.CompletionStage;
 import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicBoolean;
 
-import java.util.concurrent.atomic.AtomicReference;
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
 import org.apache.commons.pool2.impl.GenericObjectPool;
 import org.apache.commons.pool2.impl.GenericObjectPoolConfig;
+
 import org.springframework.beans.factory.DisposableBean;
-import org.springframework.context.SmartLifecycle;
 import org.springframework.data.redis.connection.PoolException;
 import org.springframework.util.Assert;
 
@@ -62,12 +62,11 @@
  * @since 2.0
  * @see #getConnection(Class)
  */
-class LettucePoolingConnectionProvider implements LettuceConnectionProvider, RedisClientProvider, DisposableBean,
-	SmartLifecycle {
+class LettucePoolingConnectionProvider implements LettuceConnectionProvider, RedisClientProvider, DisposableBean {
 
 	private static final Log log = LogFactory.getLog(LettucePoolingConnectionProvider.class);
 
-	private final AtomicReference<State> state = new AtomicReference<>(State.CREATED);
+	private final AtomicBoolean disposed = new AtomicBoolean();
 	private final LettuceConnectionProvider connectionProvider;
 	private final GenericObjectPoolConfig<StatefulConnection<?, ?>> poolConfig;
 	private final Map<StatefulConnection<?, ?>, GenericObjectPool<StatefulConnection<?, ?>>> poolRef = new ConcurrentHashMap<>(
@@ -81,10 +80,6 @@ class LettucePoolingConnectionProvider implements LettuceConnectionProvider, Red
 	private final Map<Class<?>, AsyncPool<StatefulConnection<?, ?>>> asyncPools = new ConcurrentHashMap<>(32);
 	private final BoundedPoolConfig asyncPoolConfig;
 
-	enum State {
-		CREATED, STARTING, STARTED, STOPPING, STOPPED, DESTROYED;
-	}
-
 	LettucePoolingConnectionProvider(LettuceConnectionProvider connectionProvider,
 			LettucePoolingClientConfiguration clientConfiguration) {
 
@@ -215,51 +210,43 @@ public CompletableFuture<Void> releaseAsync(StatefulConnection<?, ?> connection)
 
 	@Override
 	public void destroy() throws Exception {
-		stop();
-		state.set(State.DESTROYED);
-	}
-
 
-	@Override
-	public void start() {
-		state.set(State.STARTED);
-	}
+		if (!disposed.compareAndSet(false, true)) {
+			return;
+		}
 
-	@Override
-	public void stop() {
-		if (state.compareAndSet(State.STARTED, State.STOPPING)) {
-			List<CompletableFuture<?>> futures = new ArrayList<>();
-			if (!poolRef.isEmpty() || !asyncPoolRef.isEmpty()) {
-				log.warn("LettucePoolingConnectionProvider contains unreleased connections");
-			}
+		List<CompletableFuture<?>> futures = new ArrayList<>();
+		if (!poolRef.isEmpty() || !asyncPoolRef.isEmpty()) {
+			log.warn("LettucePoolingConnectionProvider contains unreleased connections");
+		}
 
-			if (!inProgressAsyncPoolRef.isEmpty()) {
+		if (!inProgressAsyncPoolRef.isEmpty()) {
 
-				log.warn("LettucePoolingConnectionProvider has active connection retrievals");
-				inProgressAsyncPoolRef.forEach((k, v) -> futures.add(k.thenApply(StatefulConnection::closeAsync)));
-			}
+			log.warn("LettucePoolingConnectionProvider has active connection retrievals");
+			inProgressAsyncPoolRef.forEach((k, v) -> futures.add(k.thenApply(StatefulConnection::closeAsync)));
+		}
 
-			if (!poolRef.isEmpty()) {
+		if (!poolRef.isEmpty()) {
 
-				poolRef.forEach((connection, pool) -> pool.returnObject(connection));
-				poolRef.clear();
-			}
+			poolRef.forEach((connection, pool) -> pool.returnObject(connection));
+			poolRef.clear();
+		}
 
-			if (!asyncPoolRef.isEmpty()) {
+		if (!asyncPoolRef.isEmpty()) {
 
-				asyncPoolRef.forEach((connection, pool) -> futures.add(pool.release(connection)));
-				asyncPoolRef.clear();
-			}
+			asyncPoolRef.forEach((connection, pool) -> futures.add(pool.release(connection)));
+			asyncPoolRef.clear();
+		}
 
-			pools.forEach((type, pool) -> pool.close());
+		pools.forEach((type, pool) -> pool.close());
 
-			CompletableFuture
+		CompletableFuture
 				.allOf(futures.stream().map(it -> it.exceptionally(LettuceFutureUtils.ignoreErrors()))
-					.toArray(CompletableFuture[]::new)) //
+						.toArray(CompletableFuture[]::new)) //
 				.thenCompose(ignored -> {
 
 					CompletableFuture[] poolClose = asyncPools.values().stream().map(AsyncPool::closeAsync)
-						.map(it -> it.exceptionally(LettuceFutureUtils.ignoreErrors())).toArray(CompletableFuture[]::new);
+							.map(it -> it.exceptionally(LettuceFutureUtils.ignoreErrors())).toArray(CompletableFuture[]::new);
 
 					return CompletableFuture.allOf(poolClose);
 				}) //
@@ -269,18 +256,7 @@ public void stop() {
 				}) //
 				.join();
 
-			pools.clear();
-		}
-		state.set(State.STOPPED);
-	}
-
-	@Override
-	public boolean isRunning() {
-		return State.STARTED.equals(this.state.get());
+		pools.clear();
 	}
 
-	@Override
-	public boolean isAutoStartup() {
-		return true;
-	}
 }