Merge pull request phpredis#1220 from phpredis/pipe-multi-warning-revert

Pipe multi warning revert

Author: michael-grunder

library.c

@@ -1411,7 +1411,7 @@ PHP_REDIS_API int redis_sock_connect(RedisSock *redis_sock TSRMLS_DC)
     struct timeval tv, read_tv, *tv_ptr = NULL;
     char host[1024], *persistent_id = NULL;
     const char *fmtstr = "%s:%d";
-    int host_len, err = 0;
+    int host_len, usocket = 0, err = 0;
     php_netstream_data_t *sock;
     int tcp_flag = 1;
 
@@ -1430,6 +1430,7 @@ PHP_REDIS_API int redis_sock_connect(RedisSock *redis_sock TSRMLS_DC)
 
     if(redis_sock->host[0] == '/' && redis_sock->port < 1) {
         host_len = snprintf(host, sizeof(host), "unix://%s", redis_sock->host);
+        usocket = 1;
     } else {
         if(redis_sock->port == 0)
             redis_sock->port = 6379;
@@ -1466,10 +1467,11 @@ PHP_REDIS_API int redis_sock_connect(RedisSock *redis_sock TSRMLS_DC)
         return -1;
     }
 
-    /* set TCP_NODELAY */
+    /* Attempt to set TCP_NODELAY if we're not using a unix socket. */
     sock = (php_netstream_data_t*)redis_sock->stream->abstract;
-    if (setsockopt(sock->socket, IPPROTO_TCP, TCP_NODELAY, (char *) &tcp_flag, sizeof(int)) < 0) {
-        php_error_docref(NULL TSRMLS_CC, E_ERROR, "Can't activate TCP_NODELAY option!");
+    if (!usocket) {
+        err = setsockopt(sock->socket, IPPROTO_TCP, TCP_NODELAY, (char *) &tcp_flag, sizeof(int));
+        PHPREDIS_NOTUSED(err);
     }
 
     php_stream_auto_cleanup(redis_sock->stream);

redis.c

@@ -2605,19 +2605,20 @@ PHP_METHOD(Redis, multi)
     }
 
     if (multi_value == PIPELINE) {
-        IF_PIPELINE() {
-            php_error_docref(NULL TSRMLS_CC, E_WARNING, "Already in pipeline mode");
-        } else IF_MULTI() {
+        /* Cannot enter pipeline mode in a MULTI block */
+        IF_MULTI() {
             php_error_docref(NULL TSRMLS_CC, E_ERROR, "Can't activate pipeline in multi mode!");
             RETURN_FALSE;
-        } else {
+        }
+
+        /* Enable PIPELINE if we're not already in one */
+        IF_ATOMIC() {
             free_reply_callbacks(redis_sock);
             REDIS_ENABLE_MODE(redis_sock, PIPELINE);
         }
     } else if (multi_value == MULTI) {
-        IF_MULTI() {
-            php_error_docref(NULL TSRMLS_CC, E_WARNING, "Already in multi mode");
-        } else {
+        /* Don't want to do anything if we're alredy in MULTI mode */
+        IF_NOT_MULTI() {
             cmd_len = REDIS_SPPRINTF(&cmd, "MULTI", "");
             IF_PIPELINE() {
                 PIPELINE_ENQUEUE_COMMAND(cmd, cmd_len);
@@ -2638,8 +2639,10 @@ PHP_METHOD(Redis, multi)
             }
         }
     } else {
+        php_error_docref(NULL TSRMLS_CC, E_WARNING, "Unknown mode sent to Redis::multi");
         RETURN_FALSE;
     }
+
     RETURN_ZVAL(getThis(), 1, 0);
 }
 
@@ -2823,21 +2826,22 @@ PHP_METHOD(Redis, pipeline)
         RETURN_FALSE;
     }
 
-    IF_PIPELINE() {
-        php_error_docref(NULL TSRMLS_CC, E_WARNING,
-            "Already in pipeline mode");
-    } else {
-        IF_MULTI() {
-            php_error_docref(NULL TSRMLS_CC, E_ERROR,
-                "Can't activate pipeline in multi mode!");
-            RETURN_FALSE;
-        }
+    /* User cannot enter MULTI mode if already in a pipeline */
+    IF_MULTI() {
+        php_error_docref(NULL TSRMLS_CC, E_ERROR, "Can't activate pipeline in multi mode!");
+        RETURN_FALSE;
+    }
+
+    /* Enable pipeline mode unless we're already in that mode in which case this
+     * is just a NO OP */
+    IF_ATOMIC() {
         /* NB : we keep the function fold, to detect the last function.
          * We need the response format of the n - 1 command. So, we can delete
          * when n > 2, the { 1 .. n - 2} commands */
         free_reply_callbacks(redis_sock);
         REDIS_ENABLE_MODE(redis_sock, PIPELINE);
     }
+
     RETURN_ZVAL(getThis(), 1, 0);
 }
 

tests/RedisClusterTest.php

@@ -33,6 +33,7 @@ public function testWait()     { return $this->markTestSkipped(); }
     public function testSelect()   { return $this->markTestSkipped(); }
     public function testReconnectSelect() { return $this->markTestSkipped(); }
     public function testMultipleConnect() { return $this->markTestSkipped(); }
+    public function testDoublePipeNoOp() { return $this->markTestSkipped(); }
 
     /* Load our seeds on construction */
     public function __construct() {

tests/RedisTest.php

@@ -2554,6 +2554,31 @@ public function testPipelineMultiExec()
         $this->assertEquals(5, count($ret)); // should be 5 atomic operations
     }
 
+    /* Github issue #1211 (ignore redundant calls to pipeline or multi) */
+    public function testDoublePipeNoOp() {
+        /* Only the first pipeline should be honored */
+        for ($i = 0; $i < 6; $i++) {
+            $this->redis->pipeline();
+        }
+
+        /* Set and get in our pipeline */
+        $this->redis->set('pipecount','over9000')->get('pipecount');
+
+        $data = $this->redis->exec();
+        $this->assertEquals(Array(true,'over9000'), $data);
+
+        /* Only the first MULTI should be honored */
+        for ($i = 0; $i < 6; $i++) {
+            $this->redis->multi();
+        }
+
+        /* Set and get in our MULTI block */
+        $this->redis->set('multicount', 'over9000')->get('multicount');
+
+        $data = $this->redis->exec();
+        $this->assertEquals(Array(true, 'over9000'), $data);
+    }
+
     protected function sequence($mode) {
         $ret = $this->redis->multi($mode)
             ->set('x', 42)