Issue #1252: Acquire one long lock for trying all nodes when rediscover cluster

Author: Spikhalskiy

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

@@ -1,9 +1,6 @@
 package redis.clients.jedis;
 
 import java.io.Closeable;
-import java.util.ArrayList;
-import java.util.Collections;
-import java.util.List;
 import java.util.Map;
 import java.util.Set;
 
@@ -49,39 +46,15 @@ private void initializeSlotsCache(Set<HostAndPort> startNodes, GenericObjectPool
   }
 
   public void renewSlotCache() {
-    for (JedisPool jp : getShuffledNodesPool()) {
-      Jedis jedis = null;
-      try {
-        jedis = jp.getResource();
-        cache.discoverClusterSlots(jedis);
-        break;
-      } catch (JedisConnectionException e) {
-        // try next nodes
-      } finally {
-        if (jedis != null) {
-          jedis.close();
-        }
-      }
-    }
+    cache.renewClusterSlots(null);
   }
 
   public void renewSlotCache(Jedis jedis) {
-    try {
-      cache.discoverClusterSlots(jedis);
-    } catch (JedisConnectionException e) {
-      renewSlotCache();
-    }
+    cache.renewClusterSlots(jedis);
   }
 
   @Override
   public void close() {
     cache.reset();
   }
-
-  protected List<JedisPool> getShuffledNodesPool() {
-    List<JedisPool> pools = new ArrayList<JedisPool>();
-    pools.addAll(cache.getNodes().values());
-    Collections.shuffle(pools);
-    return pools;
-  }
 }

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

@@ -1,6 +1,7 @@
 package redis.clients.jedis;
 
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
@@ -13,6 +14,8 @@
 
 import org.apache.commons.pool2.impl.GenericObjectPoolConfig;
 
+import redis.clients.jedis.exceptions.JedisConnectionException;
+import redis.clients.jedis.exceptions.JedisException;
 import redis.clients.util.SafeEncoder;
 
 public class JedisClusterInfoCache {
@@ -77,39 +80,34 @@ public void discoverClusterNodesAndSlots(Jedis jedis) {
     }
   }
 
-  public void discoverClusterSlots(Jedis jedis) {
+  public void renewClusterSlots(Jedis jedis) {
     //If rediscovering is already in process - no need to start one more same rediscovering, just return
     if (!rediscovering) {
-      w.lock();
-      rediscovering = true;
-
       try {
-        List<Object> slots = jedis.clusterSlots();
-
-        // We should clear slots after getting result about cluster slots, because if we got exception or timeout in
-        // getting new slots state, which is possible, because it's an external operation - we could got empty slots collection
-        // and ton of new Jedis instances for random nodes, see JedisSlotBasedConnectionHandler#getConnectionFromSlot
-        this.slots.clear();
-
-        for (Object slotInfoObj : slots) {
-          List<Object> slotInfo = (List<Object>) slotInfoObj;
-
-          if (slotInfo.size() <= MASTER_NODE_INDEX) {
-            continue;
+        w.lock();
+        rediscovering = true;
+
+        if (jedis != null) {
+          try {
+            discoverClusterSlots(jedis);
+            return;
+          } catch (JedisException e) {
+            //try nodes from all pools
           }
+        }
 
-          List<Integer> slotNums = getAssignedSlotArray(slotInfo);
-
-          // hostInfos
-          List<Object> hostInfos = (List<Object>) slotInfo.get(MASTER_NODE_INDEX);
-          if (hostInfos.isEmpty()) {
-            continue;
+        for (JedisPool jp : getShuffledNodesPool()) {
+          try {
+            jedis = jp.getResource();
+            discoverClusterSlots(jedis);
+            return;
+          } catch (JedisConnectionException e) {
+            // try next nodes
+          } finally {
+            if (jedis != null) {
+              jedis.close();
+            }
           }
-
-          // at this time, we just use master, discard slave information
-          HostAndPort targetNode = generateHostAndPort(hostInfos);
-
-          assignSlotsToNode(slotNums, targetNode);
         }
       } finally {
         rediscovering = false;
@@ -118,6 +116,31 @@ public void discoverClusterSlots(Jedis jedis) {
     }
   }
 
+  private void discoverClusterSlots(Jedis jedis) {
+    List<Object> slots = jedis.clusterSlots();
+    this.slots.clear();
+
+    for (Object slotInfoObj : slots) {
+      List<Object> slotInfo = (List<Object>) slotInfoObj;
+
+      if (slotInfo.size() <= MASTER_NODE_INDEX) {
+        continue;
+      }
+
+      List<Integer> slotNums = getAssignedSlotArray(slotInfo);
+
+      // hostInfos
+      List<Object> hostInfos = (List<Object>) slotInfo.get(MASTER_NODE_INDEX);
+      if (hostInfos.isEmpty()) {
+        continue;
+      }
+
+      // at this time, we just use master, discard slave information
+      HostAndPort targetNode = generateHostAndPort(hostInfos);
+      assignSlotsToNode(slotNums, targetNode);
+    }
+  }
+
   private HostAndPort generateHostAndPort(List<Object> hostInfos) {
     return new HostAndPort(SafeEncoder.encode((byte[]) hostInfos.get(0)),
         ((Long) hostInfos.get(1)).intValue());
@@ -222,6 +245,17 @@ public Map<String, JedisPool> getNodes() {
     }
   }
 
+  public List<JedisPool> getShuffledNodesPool() {
+    r.lock();
+    try {
+      List<JedisPool> pools = new ArrayList<JedisPool>(nodes.values());
+      Collections.shuffle(pools);
+      return pools;
+    } finally {
+      r.unlock();
+    }
+  }
+
   /**
    * Clear discovered nodes collections and gently release allocated resources
    */
@@ -264,5 +298,4 @@ private List<Integer> getAssignedSlotArray(List<Object> slotInfo) {
     }
     return slotNums;
   }
-
 }

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

@@ -27,7 +27,7 @@ public Jedis getConnection() {
     // ping-pong)
     // or exception if all connections are invalid
 
-    List<JedisPool> pools = getShuffledNodesPool();
+    List<JedisPool> pools = cache.getShuffledNodesPool();
 
     for (JedisPool pool : pools) {
       Jedis jedis = null;