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