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

Author: mp911de

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

@@ -39,6 +39,9 @@
 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 @@
  */
 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 void close() throws DataAccessException {
 		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 void close() throws DataAccessException {
 		}
 
 		// 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);
 		}
+	}
 
-		if (exc != null) {
-			throw convertJedisAccessException(exc);
+	private Exception handleCloseException(@Nullable Exception exceptionToThrow, Exception cause) {
+
+		if (exceptionToThrow == null) {
+			return cause;
 		}
+
+		return exceptionToThrow;
 	}
 
 	@Override

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.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 @@ void testPoolNPE() {
 		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() {