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

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

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() {