Call renew slots before final retry

pom.xml

@@ -59,6 +59,13 @@
 			<type>jar</type>
 			<scope>test</scope>
 		</dependency>
+		<dependency>
+			<groupId>org.mockito</groupId>
+			<artifactId>mockito-core</artifactId>
+			<version>2.3.0</version>
+			<type>jar</type>
+			<scope>test</scope>
+		</dependency>
 		<dependency>
 			<groupId>org.apache.commons</groupId>
 			<artifactId>commons-pool2</artifactId>

src/main/java/redis/clients/jedis/JedisClusterCommand.java

@@ -128,15 +128,15 @@ private T runWithRetries(byte[] key, int attempts, boolean tryRandomNode, boolea
       releaseConnection(connection);
       connection = null;
 
-      if (attempts <= 1) {
-        //We need this because if node is not reachable anymore - we need to finally initiate slots renewing,
-        //or we can stuck with cluster state without one node in opposite case.
-        //But now if maxAttempts = 1 or 2 we will do it too often. For each time-outed request.
+      int attemptsRemaining = attempts - 1;
+      if (attemptsRemaining == 1 && maxAttempts > 2) {
+        // May be that the node is not reachable anymore, try calling cluster slots before our final retry
+        // Only do this if maxAttempts > 2 to avoid calling cluster slots on any connection error
         //TODO make tracking of successful/unsuccessful operations for node - do renewing only
         //if there were no successful responses from this node last few seconds
         this.connectionHandler.renewSlotCache();
-
-        //no more redirections left, throw original exception, not JedisClusterMaxRedirectionsException, because it's not MOVED situation
+      } else if (attemptsRemaining <= 0) {
+        // No more redirections left, throw original exception, not JedisClusterMaxRedirectionsException, because it's not MOVED situation
         throw jce;
       }
 

src/test/java/redis/clients/jedis/JedisClusterCommandTest.java

@@ -0,0 +1,53 @@
+package redis.clients.jedis;
+
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.InOrder;
+import org.mockito.Mock;
+import org.mockito.Mockito;
+import org.mockito.junit.MockitoJUnitRunner;
+import redis.clients.jedis.exceptions.JedisConnectionException;
+
+import static org.mockito.ArgumentMatchers.anyInt;
+import static org.mockito.Mockito.when;
+
+/**
+ * @author Louis Morgan
+ */
+@RunWith(MockitoJUnitRunner.class)
+public class JedisClusterCommandTest {
+
+  @Mock
+  private Jedis connection;
+
+  @Mock
+  private JedisClusterConnectionHandler connectionHandler;
+
+  @Test
+  public void onConnectionExceptionClusterRenewsSlotCacheAndThenRetries() {
+    when(connectionHandler.getConnectionFromSlot(anyInt())).thenReturn(connection);
+    when(connection.get("foo")).thenThrow(new JedisConnectionException("Command failed"));
+
+    try {
+      new JedisClusterCommand<String>(connectionHandler, 3) {
+        @Override
+        public String execute(Jedis connection) {
+          return connection.get("foo");
+        }
+      }.run("foo");
+    } catch (JedisConnectionException expected) {
+      // We expect this to be thrown
+    }
+
+    // maxAttempts is 3, so we expect 2 attempts to get the key, followed by a call to
+    // renewSlotCache(), followed by a further attempt to get the key
+    InOrder inOrder = Mockito.inOrder(connection, connectionHandler);
+    inOrder.verify(connection).get("foo");
+    inOrder.verify(connection).close();
+    inOrder.verify(connection).get("foo");
+    inOrder.verify(connection).close();
+    inOrder.verify(connectionHandler).renewSlotCache();
+    inOrder.verify(connection).get("foo");
+    inOrder.verify(connection).close();
+  }
+}