From 4b6be1db8548816c9a5f4ad8c64c38ea7cd7d5b7 Mon Sep 17 00:00:00 2001
From: rprevot <be.prevot.richard@gmail.com>
Date: Sun, 12 Apr 2020 16:05:30 +0200
Subject: [PATCH] Fix JedisConnection.close(), ensure socket close on exit

---
 .../dyno/jedis/JedisConnectionFactory.java    | 21 +++++-
 .../jedis/JedisConnectionFactoryTest.java     | 73 +++++++++++++++++++
 2 files changed, 91 insertions(+), 3 deletions(-)
 create mode 100644 dyno-jedis/src/test/java/com/netflix/dyno/jedis/JedisConnectionFactoryTest.java

diff --git a/dyno-jedis/src/main/java/com/netflix/dyno/jedis/JedisConnectionFactory.java b/dyno-jedis/src/main/java/com/netflix/dyno/jedis/JedisConnectionFactory.java
index 855140c1..1c5b82e4 100644
--- a/dyno-jedis/src/main/java/com/netflix/dyno/jedis/JedisConnectionFactory.java
+++ b/dyno-jedis/src/main/java/com/netflix/dyno/jedis/JedisConnectionFactory.java
@@ -162,10 +162,25 @@ public <R> ListenableFuture<OperationResult<R>> executeAsync(AsyncOperation<Jedi
 
         @Override
         public void close() {
-            jedisClient.quit();
-            jedisClient.disconnect();
+            RuntimeException lastException = null;
+            
+            try {
+                jedisClient.quit();
+            } catch (RuntimeException e) {
+                lastException = e;
+            }
+            
+            try {
+                jedisClient.disconnect(); 
+            } catch (RuntimeException e) {
+                lastException = e;
+            }
+            
+            if (lastException != null) {
+                throw lastException;
+            }
         }
-
+            
         @Override
         public Host getHost() {
             return hostPool.getHost();
diff --git a/dyno-jedis/src/test/java/com/netflix/dyno/jedis/JedisConnectionFactoryTest.java b/dyno-jedis/src/test/java/com/netflix/dyno/jedis/JedisConnectionFactoryTest.java
new file mode 100644
index 00000000..4714c6e3
--- /dev/null
+++ b/dyno-jedis/src/test/java/com/netflix/dyno/jedis/JedisConnectionFactoryTest.java
@@ -0,0 +1,73 @@
+package com.netflix.dyno.jedis;
+
+import com.netflix.dyno.connectionpool.ConnectionPoolConfiguration;
+import com.netflix.dyno.connectionpool.Host;
+import com.netflix.dyno.connectionpool.Host.Status;
+import com.netflix.dyno.connectionpool.HostBuilder;
+import com.netflix.dyno.connectionpool.HostConnectionPool;
+import com.netflix.dyno.connectionpool.impl.ConnectionPoolConfigurationImpl;
+import com.netflix.dyno.connectionpool.impl.CountingConnectionPoolMonitor;
+import com.netflix.dyno.connectionpool.impl.HostConnectionPoolImpl;
+import com.netflix.dyno.contrib.DynoOPMonitor;
+import com.netflix.dyno.jedis.JedisConnectionFactory.JedisConnection;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Assume;
+import org.junit.Before;
+import org.junit.Test;
+import redis.clients.jedis.Jedis;
+import redis.embedded.RedisServer;
+import redis.clients.jedis.exceptions.JedisConnectionException;
+
+public class JedisConnectionFactoryTest {
+
+    private static final int REDIS_PORT = 8998;
+    private static final String REDIS_RACK = "rack-1c";
+
+    private RedisServer redisServer;
+
+    @Before
+    public void setUp() throws Exception {
+        // skip tests on windows due to https://github.com/spinnaker/embedded-redis#redis-version
+        Assume.assumeFalse(System.getProperty("os.name").toLowerCase().startsWith("win"));
+    }
+
+    @After
+    public void tearDown() throws Exception {
+        if (redisServer != null) {
+            redisServer.stop();
+        }
+    }
+
+    @Test
+    public void testJedisConnFactory_ensureSocketClose() throws Exception {
+        redisServer = new RedisServer(REDIS_PORT);
+        redisServer.start();
+        
+        Host noAuthHost = new HostBuilder()
+                .setHostname("localhost")
+                .setPort(REDIS_PORT)
+                .setRack(REDIS_RACK)
+                .setStatus(Status.Up)
+                .createHost();
+
+        JedisConnectionFactory conFactory = new JedisConnectionFactory(new DynoOPMonitor("some-application-name"), null);
+        ConnectionPoolConfiguration cpConfig = new ConnectionPoolConfigurationImpl("some-name");
+        CountingConnectionPoolMonitor poolMonitor = new CountingConnectionPoolMonitor();
+        HostConnectionPool<Jedis> hostConnectionPool = new HostConnectionPoolImpl<>(noAuthHost, conFactory, cpConfig, poolMonitor);
+        
+        JedisConnection connection = (JedisConnection) conFactory.createConnection(hostConnectionPool);
+        connection.execPing();        
+        Assert.assertTrue(connection.getClient().isConnected());
+        
+        redisServer.stop();
+        
+        try {
+            connection.close();     
+            Assert.fail("expected to throw");
+        } catch (JedisConnectionException e) {
+            Assert.assertFalse(connection.getClient().isConnected());
+        }
+    }
+
+}