Fix autorehashing in RedisArray

This commit fixes auto rehashing in RedisArray as well as fixes
a couple of memory leaks found along the way

Addresses phpredis#442 and phpredis#294

Author: michael-grunder

redis_array.c

@@ -32,6 +32,12 @@
 #include "redis_array.h"
 #include "redis_array_impl.h"
 
+/* Simple macro to detect failure in a RedisArray call */
+#define RA_CALL_FAILED(rv, cmd) \
+    ((Z_TYPE_P(rv) == IS_BOOL && Z_BVAL_P(rv) == 0) || \
+    (Z_TYPE_P(rv) == IS_ARRAY && zend_hash_num_elements(Z_ARRVAL_P(rv)) == 0) || \
+    (Z_TYPE_P(rv) == IS_LONG && Z_LVAL_P(rv) == 0 && !strcasecmp(cmd, "TYPE"))) \
+
 extern zend_class_entry *redis_ce;
 zend_class_entry *redis_array_ce;
 
@@ -76,41 +82,48 @@ zend_function_entry redis_array_functions[] = {
      {NULL, NULL, NULL}
 };
 
-int le_redis_array;
-void redis_destructor_redis_array(zend_rsrc_list_entry * rsrc TSRMLS_DC)
-{
-	int i;
-	RedisArray *ra = (RedisArray*)rsrc->ptr;
+static void redis_array_free(RedisArray *ra) {
+    int i;
 
-	/* delete Redis objects */
-	for(i = 0; i < ra->count; ++i) {
-		zval_dtor(ra->redis[i]);
-		efree(ra->redis[i]);
+    // Redis objects
+    for(i=0;i<ra->count;i++) {
+        zval_dtor(ra->redis[i]);
+        efree(ra->redis[i]);
+        efree(ra->hosts[i]);
+    }
+    efree(ra->redis);
+    efree(ra->hosts);
 
-		/* remove host too */
-		efree(ra->hosts[i]);
-	}
-	efree(ra->redis);
-	efree(ra->hosts);
+    /* delete hash function */
+    if(ra->z_fun) {
+        zval_dtor(ra->z_fun);
+        efree(ra->z_fun);
+    }
 
-	/* delete function */
-	if(ra->z_fun) {
-		zval_dtor(ra->z_fun);
-		efree(ra->z_fun);
-	}
+    /* Distributor */
+    if(ra->z_dist) {
+        zval_dtor(ra->z_dist);
+        efree(ra->z_dist);
+    }
 
-	/* delete distributor */
-	if(ra->z_dist) {
-		zval_dtor(ra->z_dist);
-		efree(ra->z_dist);
-	}
+    /* Delete pur commands */
+    zval_dtor(ra->z_pure_cmds);
+    efree(ra->z_pure_cmds);
+
+    // Free structure itself
+    efree(ra);
+}
 
-	/* delete list of pure commands */
-	zval_dtor(ra->z_pure_cmds);
-	efree(ra->z_pure_cmds);
+int le_redis_array;
+void redis_destructor_redis_array(zend_rsrc_list_entry * rsrc TSRMLS_DC)
+{
+    RedisArray *ra = (RedisArray*)rsrc->ptr;
 
-	/* free container */
-	efree(ra);
+    /* Free previous ring if it's set */
+    if(ra->prev) redis_array_free(ra->prev);
+
+    /* Free parent array */
+    redis_array_free(ra);
 }
 
 /**
@@ -280,6 +293,7 @@ PHP_METHOD(RedisArray, __construct)
 
 	if(ra) {
 		ra->auto_rehash = b_autorehash;
+		if(ra->prev) ra->prev->auto_rehash = b_autorehash;
 #if PHP_VERSION_ID >= 50400
 		id = zend_list_insert(ra, le_redis_array TSRMLS_CC);
 #else
@@ -302,7 +316,6 @@ ra_forward_call(INTERNAL_FUNCTION_PARAMETERS, RedisArray *ra, const char *cmd, i
 	HashTable *h_args;
 
 	int argc;
-	int failed;
 	zend_bool b_write_cmd = 0;
 
 	h_args = Z_ARRVAL_P(z_args);
@@ -366,23 +379,15 @@ ra_forward_call(INTERNAL_FUNCTION_PARAMETERS, RedisArray *ra, const char *cmd, i
 	} else { /* call directly through. */
 		call_user_function(&redis_ce->function_table, &redis_inst, &z_fun, return_value, argc, z_callargs TSRMLS_CC);
 
-		failed = 0;
-		if((Z_TYPE_P(return_value) == IS_BOOL && Z_BVAL_P(return_value) == 0) ||
-		   (Z_TYPE_P(return_value) == IS_ARRAY && zend_hash_num_elements(Z_ARRVAL_P(return_value)) == 0) ||
-		   (Z_TYPE_P(return_value) == IS_LONG && Z_LVAL_P(return_value) == 0 && !strcasecmp(cmd, "TYPE")))
-
-		{
-			failed = 1;
-		}
-
 		/* check if we have an error. */
-		if(failed && ra->prev && !b_write_cmd) { /* there was an error reading, try with prev ring. */
-			/* ERROR, FALLBACK TO PREVIOUS RING and forward a reference to the first redis instance we were looking at. */
+		if(RA_CALL_FAILED(return_value,cmd) && ra->prev && !b_write_cmd) { /* there was an error reading, try with prev ring. */
+		    /* ERROR, FALLBACK TO PREVIOUS RING and forward a reference to the first redis instance we were looking at. */
 			ra_forward_call(INTERNAL_FUNCTION_PARAM_PASSTHRU, ra->prev, cmd, cmd_len, z_args, z_new_target?z_new_target:redis_inst);
 		}
 
-		if(!failed && !b_write_cmd && z_new_target && ra->auto_rehash) { /* move key from old ring to new ring */
-				ra_move_key(key, key_len, redis_inst, z_new_target TSRMLS_CC);
+		/* Autorehash if the key was found on the previous node if this is a read command and auto rehashing is on */
+		if(!RA_CALL_FAILED(return_value,cmd) && !b_write_cmd && z_new_target && ra->auto_rehash) { /* move key from old ring to new ring */
+		    ra_move_key(key, key_len, redis_inst, z_new_target TSRMLS_CC);
 		}
 	}
 

redis_array_impl.c

@@ -273,6 +273,7 @@ RedisArray *ra_load_array(const char *name TSRMLS_DC) {
 	/* create RedisArray object */
 	ra = ra_make_array(hHosts, z_fun, z_dist, hPrev, b_index, b_pconnect, l_retry_interval, b_lazy_connect TSRMLS_CC);
 	ra->auto_rehash = b_autorehash;
+	if(ra->prev) ra->prev->auto_rehash = b_autorehash;
 
 	/* cleanup */
 	zval_dtor(z_params_hosts);
@@ -605,6 +606,7 @@ ra_index_key(const char *key, int key_len, zval *z_redis TSRMLS_DC) {
 
 	/* don't dtor z_ret, since we're returning z_redis */
 	efree(z_args[0]);
+    zval_dtor(z_args[1]);
 	efree(z_args[1]);
 }
 
@@ -967,6 +969,7 @@ ra_move_string(const char *key, int key_len, zval *z_from, zval *z_to, long ttl
 		ZVAL_STRINGL(z_args[0], key, key_len, 0);
 		ZVAL_LONG(z_args[1], ttl);
 		ZVAL_STRINGL(z_args[2], Z_STRVAL(z_ret), Z_STRLEN(z_ret), 1); /* copy z_ret to arg 1 */
+		zval_dtor(&z_ret); /* free memory from our previous call */
 		call_user_function(&redis_ce->function_table, &z_to, &z_fun_set, &z_ret, 3, z_args TSRMLS_CC);
 		/* cleanup */
 		efree(z_args[1]);
@@ -977,6 +980,7 @@ ra_move_string(const char *key, int key_len, zval *z_from, zval *z_to, long ttl
 		ZVAL_STRINGL(&z_fun_set, "SET", 3, 0);
 		ZVAL_STRINGL(z_args[0], key, key_len, 0);
 		ZVAL_STRINGL(z_args[1], Z_STRVAL(z_ret), Z_STRLEN(z_ret), 1); /* copy z_ret to arg 1 */
+		zval_dtor(&z_ret); /* free memory from our previous return value */
 		call_user_function(&redis_ce->function_table, &z_to, &z_fun_set, &z_ret, 2, z_args TSRMLS_CC);
 		/* cleanup */
 		zval_dtor(z_args[1]);