From c600d0e22dd7be5324693c2cd4a5dc0b27d9100d Mon Sep 17 00:00:00 2001
From: Mark Paluch <mpaluch@vmware.com>
Date: Thu, 14 Jul 2022 16:04:37 +0200
Subject: [PATCH] Polishing.

We now ensure proper exception handling in connection close methods to avoid resource leaks. Also, exceptions during connection close are no longer thrown to ensure proper resource cleanup behavior and API design.

See #2356
---
 .../connection/AbstractRedisConnection.java   | 33 ++++++++++++-------
 .../connection/lettuce/LettuceConnection.java | 19 +++++++----
 2 files changed, 34 insertions(+), 18 deletions(-)

diff --git a/src/main/java/org/springframework/data/redis/connection/AbstractRedisConnection.java b/src/main/java/org/springframework/data/redis/connection/AbstractRedisConnection.java
index 82fef3865..12f19a556 100644
--- a/src/main/java/org/springframework/data/redis/connection/AbstractRedisConnection.java
+++ b/src/main/java/org/springframework/data/redis/connection/AbstractRedisConnection.java
@@ -19,10 +19,12 @@ import java.io.IOException;
 import java.util.Map;
 import java.util.concurrent.ConcurrentHashMap;
 
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
+
 import org.springframework.dao.DataAccessException;
 import org.springframework.dao.InvalidDataAccessApiUsageException;
 import org.springframework.dao.InvalidDataAccessResourceUsageException;
-import org.springframework.data.redis.RedisSystemException;
 import org.springframework.lang.Nullable;
 import org.springframework.util.Assert;
 
@@ -33,6 +35,8 @@ import org.springframework.util.Assert;
  */
 public abstract class AbstractRedisConnection implements RedisConnection {
 
+	private final Log LOGGER = LogFactory.getLog(getClass());
+
 	private @Nullable RedisSentinelConfiguration sentinelConfiguration;
 	private final Map<RedisNode, RedisSentinelConnection> connectionCache = new ConcurrentHashMap<>();
 
@@ -96,18 +100,23 @@ public abstract class AbstractRedisConnection implements RedisConnection {
 	@Override
 	public void close() throws DataAccessException {
 
-		if (!connectionCache.isEmpty()) {
-			for (RedisNode node : connectionCache.keySet()) {
-				RedisSentinelConnection connection = connectionCache.remove(node);
-				if (connection.isOpen()) {
-					try {
-						connection.close();
-					} catch (IOException e) {
-						throw new RedisSystemException("Failed to close sentinel connection", e);
-					}
-				}
+		if (connectionCache.isEmpty()) {
+			return;
+		}
+
+		for (RedisNode node : connectionCache.keySet()) {
+
+			RedisSentinelConnection connection = connectionCache.remove(node);
+
+			if (!connection.isOpen()) {
+				continue;
+			}
+
+			try {
+				connection.close();
+			} catch (IOException e) {
+				LOGGER.info("Failed to close sentinel connection", e);
 			}
 		}
 	}
-
 }
diff --git a/src/main/java/org/springframework/data/redis/connection/lettuce/LettuceConnection.java b/src/main/java/org/springframework/data/redis/connection/lettuce/LettuceConnection.java
index 0afb6d493..1aade0292 100644
--- a/src/main/java/org/springframework/data/redis/connection/lettuce/LettuceConnection.java
+++ b/src/main/java/org/springframework/data/redis/connection/lettuce/LettuceConnection.java
@@ -55,6 +55,9 @@ import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicLong;
 import java.util.function.Supplier;
 
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
+
 import org.springframework.beans.BeanUtils;
 import org.springframework.core.convert.converter.Converter;
 import org.springframework.dao.DataAccessException;
@@ -90,6 +93,8 @@ import org.springframework.util.ObjectUtils;
  */
 public class LettuceConnection extends AbstractRedisConnection {
 
+	private final Log LOGGER = LogFactory.getLog(getClass());
+
 	static final RedisCodec<byte[], byte[]> CODEC = ByteArrayCodec.INSTANCE;
 
 	private static final ExceptionTranslationStrategy EXCEPTION_TRANSLATION = new FallbackExceptionTranslationStrategy(
@@ -342,7 +347,7 @@ public class LettuceConnection extends AbstractRedisConnection {
 	}
 
 	@Override
-	public void close() throws DataAccessException {
+	public void close() {
 
 		super.close();
 
@@ -352,7 +357,11 @@ public class LettuceConnection extends AbstractRedisConnection {
 
 		isClosed = true;
 
-		reset();
+		try {
+			reset();
+		} catch (RuntimeException e) {
+			LOGGER.debug("Failed to reset connection during close", e);
+		}
 	}
 
 	private void reset() {
@@ -629,8 +638,7 @@ public class LettuceConnection extends AbstractRedisConnection {
 		checkSubscription();
 
 		if (isQueueing() || isPipelined()) {
-			throw new InvalidDataAccessApiUsageException(
-					"Transaction/Pipelining is not supported for Pub/Sub subscriptions");
+			throw new InvalidDataAccessApiUsageException("Transaction/Pipelining is not supported for Pub/Sub subscriptions");
 		}
 
 		try {
@@ -647,8 +655,7 @@ public class LettuceConnection extends AbstractRedisConnection {
 		checkSubscription();
 
 		if (isQueueing() || isPipelined()) {
-			throw new InvalidDataAccessApiUsageException(
-					"Transaction/Pipelining is not supported for Pub/Sub subscriptions");
+			throw new InvalidDataAccessApiUsageException("Transaction/Pipelining is not supported for Pub/Sub subscriptions");
 		}
 
 		try {