From ed816393f2b30ade0ad490a523014a7100311a79 Mon Sep 17 00:00:00 2001
From: Mark Paluch <mpaluch@vmware.com>
Date: Thu, 14 Jul 2022 16:04:20 +0200
Subject: [PATCH] `JedisConnection.close()` no longer throws exceptions.

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

Closes #2356
---
 .../connection/jedis/JedisConnection.java     | 32 ++++++++++++++-----
 .../JedisConnectionIntegrationTests.java      | 27 ++++++++++++++++
 2 files changed, 51 insertions(+), 8 deletions(-)

diff --git a/src/main/java/org/springframework/data/redis/connection/jedis/JedisConnection.java b/src/main/java/org/springframework/data/redis/connection/jedis/JedisConnection.java
index 33f74d87d..1c51ddc1a 100644
--- a/src/main/java/org/springframework/data/redis/connection/jedis/JedisConnection.java
+++ b/src/main/java/org/springframework/data/redis/connection/jedis/JedisConnection.java
@@ -39,6 +39,9 @@ import java.util.function.Consumer;
 import java.util.function.Function;
 import java.util.function.Supplier;
 
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
+
 import org.springframework.core.convert.converter.Converter;
 import org.springframework.dao.DataAccessException;
 import org.springframework.dao.InvalidDataAccessApiUsageException;
@@ -72,6 +75,8 @@ import org.springframework.util.CollectionUtils;
  */
 public class JedisConnection extends AbstractRedisConnection {
 
+	private final Log LOGGER = LogFactory.getLog(getClass());
+
 	private static final ExceptionTranslationStrategy EXCEPTION_TRANSLATION = new FallbackExceptionTranslationStrategy(
 			JedisExceptionConverter.INSTANCE);
 
@@ -300,8 +305,13 @@ public class JedisConnection extends AbstractRedisConnection {
 		super.close();
 
 		JedisSubscription subscription = this.subscription;
-		if (subscription != null) {
-			subscription.close();
+		try {
+			if (subscription != null) {
+				subscription.close();
+			}
+		} catch (Exception ex) {
+			LOGGER.debug("Cannot terminate subscription", ex);
+		} finally {
 			this.subscription = null;
 		}
 
@@ -312,21 +322,27 @@ public class JedisConnection extends AbstractRedisConnection {
 		}
 
 		// else close the connection normally (doing the try/catch dance)
-		Exception exc = null;
+
 		try {
 			jedis.quit();
 		} catch (Exception ex) {
-			exc = ex;
+			LOGGER.debug("Failed to QUIT during close", ex);
 		}
+
 		try {
 			jedis.disconnect();
 		} catch (Exception ex) {
-			exc = ex;
+			LOGGER.debug("Failed to disconnect during close", ex);
+		}
+	}
+
+	private Exception handleCloseException(@Nullable Exception exceptionToThrow, Exception cause) {
+
+		if (exceptionToThrow == null) {
+			return cause;
 		}
 
-		if (exc != null) {
-			throw convertJedisAccessException(exc);
-		}
+		return exceptionToThrow;
 	}
 
 	@Override
diff --git a/src/test/java/org/springframework/data/redis/connection/jedis/JedisConnectionIntegrationTests.java b/src/test/java/org/springframework/data/redis/connection/jedis/JedisConnectionIntegrationTests.java
index 76c292951..5a6ff699c 100644
--- a/src/test/java/org/springframework/data/redis/connection/jedis/JedisConnectionIntegrationTests.java
+++ b/src/test/java/org/springframework/data/redis/connection/jedis/JedisConnectionIntegrationTests.java
@@ -16,6 +16,7 @@
 package org.springframework.data.redis.connection.jedis;
 
 import static org.assertj.core.api.Assertions.*;
+import static org.mockito.Mockito.*;
 
 import redis.clients.jedis.JedisPoolConfig;
 
@@ -47,6 +48,7 @@ import org.springframework.data.redis.connection.StringRedisConnection.StringTup
 import org.springframework.data.redis.test.condition.EnabledOnRedisSentinelAvailable;
 import org.springframework.test.context.ContextConfiguration;
 import org.springframework.test.context.junit.jupiter.SpringExtension;
+import org.springframework.test.util.ReflectionTestUtils;
 
 /**
  * Integration test of {@link JedisConnection}
@@ -339,6 +341,31 @@ public class JedisConnectionIntegrationTests extends AbstractConnectionIntegrati
 		factory2.destroy();
 	}
 
+	@Test // GH-2356
+	void closeWithFailureShouldReleaseConnection() {
+
+		JedisPoolConfig config = new JedisPoolConfig();
+		config.setMaxTotal(1);
+
+		JedisConnectionFactory factory = new JedisConnectionFactory(config);
+		factory.setUsePool(true);
+		factory.setHostName(SettingsUtils.getHost());
+		factory.setPort(SettingsUtils.getPort());
+		factory.afterPropertiesSet();
+
+		RedisConnection conn = factory.getConnection();
+
+		JedisSubscription subscriptionMock = mock(JedisSubscription.class);
+		doThrow(new IllegalStateException()).when(subscriptionMock).close();
+		ReflectionTestUtils.setField(conn, "subscription", subscriptionMock);
+
+		conn.close();
+
+		// Make sure we don't end up with broken connection
+		factory.getConnection().dbSize();
+		factory.destroy();
+	}
+
 	@SuppressWarnings("unchecked")
 	@Test // DATAREDIS-285
 	void testExecuteShouldConvertArrayReplyCorrectly() {