Issue.1765 (#1774)

Various improvements and fixes to cluster slot caching.

* Improves slot caching so any unique set of seeds all hash to the same key

* Fix a couple of memory leaks.

* Fixes a segfault when executing a multiple key command such as `MGET` or `MSET` while the cluster is resharding.

1 files changed, 32 insertions, 38 deletions

diff --git a/redis_cluster.c b/redis_cluster.c
index 6b3dfa5e..a2ced22a 100644
--- a/redis_cluster.c
+++ b/redis_cluster.c
@@ -340,55 +340,51 @@ void free_cluster_context(zend_object *object) {
     zend_object_std_dtor(&cluster->std);
 }
 
-/* Validate redis cluster construction arguments */
-static int
-cluster_validate_args(double timeout, double read_timeout, HashTable *seeds) {
-    if (timeout < 0L || timeout > INT_MAX) {
-        CLUSTER_THROW_EXCEPTION("Invalid timeout", 0);
-        return FAILURE;
-    }
-    if (read_timeout < 0L || read_timeout > INT_MAX) {
-        CLUSTER_THROW_EXCEPTION("Invalid read timeout", 0);
-        return FAILURE;
-    }
-    /* Make sure there are some seeds */
-    if (zend_hash_num_elements(seeds) == 0) {
-        CLUSTER_THROW_EXCEPTION("Must pass seeds", 0);
-        return FAILURE;
-    }
-
-    return SUCCESS;
-}
-
+/* Take user provided seeds and return unique and valid ones */
 /* Attempt to connect to a Redis cluster provided seeds and timeout options */
 static void redis_cluster_init(redisCluster *c, HashTable *ht_seeds, double timeout,
                                double read_timeout, int persistent, char *auth,
                                size_t auth_len)
 {
+    zend_string *hash = NULL, **seeds;
     redisCachedCluster *cc;
+    uint32_t nseeds;
+    char *err;
 
-    cluster_validate_args(timeout, read_timeout, ht_seeds);
+    /* Validate our arguments and get a sanitized seed array */
+    seeds = cluster_validate_args(timeout, read_timeout, ht_seeds, &nseeds, &err);
+    if (seeds == NULL) {
+        CLUSTER_THROW_EXCEPTION(err, 0);
+        return;
+    }
 
-    if (auth && auth_len > 0) {
+    if (auth && auth_len) {
         c->flags->auth = zend_string_init(auth, auth_len, 0);
     }
 
     c->timeout = timeout;
     c->read_timeout = read_timeout;
     c->persistent = persistent;
+    c->waitms = timeout * 1000L;
+
+    /* Attempt to load slots from cache if caching is enabled */
+    if (CLUSTER_CACHING_ENABLED()) {
+        /* Exit early if we can load from cache */
+        hash = cluster_hash_seeds(seeds, nseeds);
+        if ((cc = cluster_cache_load(hash))) {
+            cluster_init_cache(c, cc);
+            goto cleanup;
+        }
+    }
 
-    /* Calculate the number of milliseconds we will wait when bouncing around,
-     * (e.g. a node goes down), which is not the same as a standard timeout. */
-    c->waitms = (long)(timeout * 1000);
+    /* Initialize seeds and attempt to map keyspace */
+    cluster_init_seeds(c, seeds, nseeds);
+    if (cluster_map_keyspace(c) == SUCCESS && hash)
+        cluster_cache_store(hash, c->nodes);
 
-    /* Attempt to load from cache */
-    if ((cc = cluster_cache_load(ht_seeds))) {
-        cluster_init_cache(c, cc);
-    } else if (cluster_init_seeds(c, ht_seeds) == SUCCESS &&
-               cluster_map_keyspace(c) == SUCCESS)
-    {
-        cluster_cache_store(ht_seeds, c->nodes);
-    }
+cleanup:
+    if (hash) zend_string_release(hash);
+    free_seed_array(seeds, nseeds);
 }
 
 
@@ -556,11 +552,7 @@ distcmd_resp_handler(INTERNAL_FUNCTION_PARAMETERS, redisCluster *c, short slot,
     ctx->last    = last;
 
     // Attempt to send the command
-    if (cluster_send_command(c,slot,mc->cmd.c,mc->cmd.len) < 0 ||
-       c->err != NULL)
-    {
-        cluster_multi_free(mc);
-        zval_dtor(z_ret);
+    if (cluster_send_command(c,slot,mc->cmd.c,mc->cmd.len) < 0 || c->err != NULL) {
         efree(ctx);
         return -1;
     }
@@ -865,6 +857,7 @@ static int cluster_mset_cmd(INTERNAL_FUNCTION_PARAMETERS, char *kw, int kw_len,
             if (distcmd_resp_handler(INTERNAL_FUNCTION_PARAM_PASSTHRU, c,
                                     slot, &mc, z_ret, i == argc, cb) < 0)
             {
+                cluster_multi_free(&mc);
                 return -1;
             }
         }
@@ -890,6 +883,7 @@ static int cluster_mset_cmd(INTERNAL_FUNCTION_PARAMETERS, char *kw, int kw_len,
         if (distcmd_resp_handler(INTERNAL_FUNCTION_PARAM_PASSTHRU, c, slot, &mc,
                                 z_ret, 1, cb) < 0)
         {
+            cluster_multi_free(&mc);
             return -1;
         }
     }