Reset the socket after a timeout to make sure no wrong data is received (phpredis#1417)

* Reset the socket after a timeout to make sure no wrong data is received

* Remove the lazy_connect completely

* Missing TSRMLS_CC

* Remove redundant check if the stream exists

* Add the redis_sock_server_open to the CLUSTER_SEND_PAYLOAD macro

Author: marcdejonge

cluster_library.c

@@ -270,10 +270,7 @@ static int cluster_send_direct(RedisSock *redis_sock, char *cmd, int cmd_len,
 {
     char buf[1024];
 
-    /* Connect to the socket if we aren't yet */
-    CLUSTER_LAZY_CONNECT(redis_sock);
-
-    /* Send our command, validate the reply type, and consume the first line */
+    /* Connect to the socket if we aren't yet and send our command, validate the reply type, and consume the first line */
     if (!CLUSTER_SEND_PAYLOAD(redis_sock,cmd,cmd_len) ||
         !CLUSTER_VALIDATE_REPLY_TYPE(redis_sock, type) ||
         !php_stream_gets(redis_sock->stream, buf, sizeof(buf))) return -1;
@@ -1088,7 +1085,6 @@ PHP_REDIS_API void cluster_disconnect(redisCluster *c, int force TSRMLS_DC) {
     ZEND_HASH_FOREACH_PTR(c->nodes, node) {
         if (node == NULL) continue;
         redis_sock_disconnect(node->sock, force TSRMLS_CC);
-        node->sock->lazy_connect = 1;
     } ZEND_HASH_FOREACH_END();
 }
 
@@ -1124,7 +1120,9 @@ static int cluster_dist_write(redisCluster *c, const char *cmd, size_t sz,
         if (!redis_sock) continue;
 
         /* Connect to this node if we haven't already */
-        CLUSTER_LAZY_CONNECT(redis_sock);
+        if(redis_sock_server_open(redis_sock TSRMLS_CC)) {
+            continue;
+        }
 
         /* If we're not on the master, attempt to send the READONLY command to
          * this slave, and skip it if that fails */
@@ -1200,11 +1198,9 @@ static int cluster_sock_write(redisCluster *c, const char *cmd, size_t sz,
      * at random. */
     if (failover == REDIS_FAILOVER_NONE) {
         /* Success if we can send our payload to the master */
-        CLUSTER_LAZY_CONNECT(redis_sock);
         if (CLUSTER_SEND_PAYLOAD(redis_sock, cmd, sz)) return 0;
     } else if (failover == REDIS_FAILOVER_ERROR) {
         /* Try the master, then fall back to any slaves we may have */
-        CLUSTER_LAZY_CONNECT(redis_sock);
         if (CLUSTER_SEND_PAYLOAD(redis_sock, cmd, sz) ||
            !cluster_dist_write(c, cmd, sz, 1 TSRMLS_CC)) return 0;
     } else {
@@ -1225,10 +1221,7 @@ static int cluster_sock_write(redisCluster *c, const char *cmd, size_t sz,
         /* Skip this node if it's the one that failed, or if it's a slave */
         if (seed_node == NULL || seed_node->sock == redis_sock || seed_node->slave) continue;
 
-        /* Connect to this node if we haven't already */
-        CLUSTER_LAZY_CONNECT(seed_node->sock);
-
-        /* Attempt to write our request to this node */
+        /* Connect to this node if we haven't already and attempt to write our request to this node */
         if (CLUSTER_SEND_PAYLOAD(seed_node->sock, cmd, sz)) {
             c->cmd_slot = seed_node->slot;
             c->cmd_sock = seed_node->sock;
@@ -1456,6 +1449,9 @@ PHP_REDIS_API short cluster_send_command(redisCluster *c, short slot, const char
             "The Redis Cluster is down (CLUSTERDOWN)", 0 TSRMLS_CC);
         return -1;
     } else if (timedout) {
+        // Make sure the socket is reconnected, it such that it is in a clean state
+        redis_sock_disconnect(c->cmd_sock, 1 TSRMLS_CC);
+
         zend_throw_exception(redis_cluster_exception_ce,
             "Timed out attempting to find data in the correct node!", 0 TSRMLS_CC);
     }

cluster_library.h

@@ -51,13 +51,6 @@
     ZSTR_LEN(SLOT_SOCK(c,c->redir_slot)->host) != c->redir_host_len || \
     memcmp(ZSTR_VAL(SLOT_SOCK(c,c->redir_slot)->host),c->redir_host,c->redir_host_len))
 
-/* Lazy connect logic */
-#define CLUSTER_LAZY_CONNECT(s) \
-    if(s->lazy_connect) { \
-        s->lazy_connect = 0; \
-        redis_sock_server_open(s TSRMLS_CC); \
-    }
-
 /* Clear out our "last error" */
 #define CLUSTER_CLEAR_ERROR(c) do { \
     if (c->err) { \
@@ -69,7 +62,7 @@
 
 /* Protected sending of data down the wire to a RedisSock->stream */
 #define CLUSTER_SEND_PAYLOAD(sock, buf, len) \
-    (sock && sock->stream && !redis_check_eof(sock, 1 TSRMLS_CC) && \
+    (sock && !redis_sock_server_open(sock TSRMLS_CC) && sock->stream && !redis_check_eof(sock, 1 TSRMLS_CC) && \
      php_stream_write(sock->stream, buf, len)==len)
 
 /* Macro to read our reply type character */

common.h

@@ -678,8 +678,6 @@ typedef struct {
 
     zend_string    *err;
 
-    zend_bool      lazy_connect;
-
     int            scan;
 
     int            readonly;

library.c

@@ -1687,7 +1687,6 @@ redis_sock_create(char *host, int host_len, unsigned short port,
     redis_sock->dbNumber = 0;
     redis_sock->retry_interval = retry_interval * 1000;
     redis_sock->persistent = persistent;
-    redis_sock->lazy_connect = lazy_connect;
     redis_sock->persistent_id = NULL;
 
     if (persistent && persistent_id != NULL) {

redis.c

@@ -647,11 +647,8 @@ redis_sock_get(zval *id TSRMLS_DC, int no_throw)
         return NULL;
     }
 
-    if (redis_sock->lazy_connect) {
-        redis_sock->lazy_connect = 0;
-        if (redis_sock_server_open(redis_sock TSRMLS_CC) < 0) {
-            return NULL;
-        }
+    if (redis_sock_server_open(redis_sock TSRMLS_CC) < 0) {
+        return NULL;
     }
 
     return redis_sock;