Merge pull request #733 from raydog/multi-buffer-fix

Fix for multi/exec logic with detect_buffers enabled

Author: raydog

index.js

@@ -610,7 +610,7 @@ function try_callback(callback, reply) {
 function reply_to_object(reply) {
     var obj = {}, j, jl, key, val;
 
-    if (reply.length === 0) {
+    if (reply.length === 0 || !Array.isArray(reply)) {
         return null;
     }
 
@@ -648,7 +648,7 @@ RedisClient.prototype.return_reply = function (reply) {
     // If the "reply" here is actually a message received asynchronously due to a
     // pubsub subscription, don't pop the command queue as we'll only be consuming
     // the head command prematurely.
-    if (Array.isArray(reply) && reply.length > 0 && reply[0]) {
+    if (this.pub_sub_mode && Array.isArray(reply) && reply.length > 0 && reply[0]) {
         type = reply[0].toString();
     }
 
@@ -672,7 +672,7 @@ RedisClient.prototype.return_reply = function (reply) {
 
     if (command_obj && !command_obj.sub_command) {
         if (typeof command_obj.callback === "function") {
-            if (this.options.detect_buffers && command_obj.buffer_args === false) {
+            if (this.options.detect_buffers && command_obj.buffer_args === false && 'exec' !== command_obj.command.toLowerCase()) {
                 // If detect_buffers option was specified, then the reply from the parser will be Buffers.
                 // If this command did not use Buffer arguments, then convert the reply to Strings here.
                 reply = reply_to_strings(reply);
@@ -1106,15 +1106,24 @@ Multi.prototype.HMSET = Multi.prototype.hmset;
 Multi.prototype.exec = function (callback) {
     var self = this;
     var errors = [];
+    var wants_buffers = [];
     // drain queue, callback will catch "QUEUED" or error
     // TODO - get rid of all of these anonymous functions which are elegant but slow
     this.queue.forEach(function (args, index) {
-        var command = args[0], obj;
+        var command = args[0], obj, i, il, buffer_args;
         if (typeof args[args.length - 1] === "function") {
             args = args.slice(1, -1);
         } else {
             args = args.slice(1);
         }
+        // Keep track of who wants buffer responses:
+        buffer_args = false;
+        for (i = 0, il = args.length; i < il; i += 1) {
+            if (Buffer.isBuffer(args[i])) {
+                buffer_args = true;
+            }
+        }
+        wants_buffers.push(buffer_args);
         if (args.length === 1 && Array.isArray(args[0])) {
             args = args[0];
         }
@@ -1149,12 +1158,18 @@ Multi.prototype.exec = function (callback) {
             }
         }
 
-        var i, il, reply, args;
+        var i, il, reply, to_buffer, args;
 
         if (replies) {
             for (i = 1, il = self.queue.length; i < il; i += 1) {
                 reply = replies[i - 1];
                 args = self.queue[i];
+                to_buffer = wants_buffers[i];
+
+                // If we asked for strings, even in detect_buffers mode, then return strings:
+                if (self._client.options.detect_buffers && to_buffer === false) {
+                    replies[i - 1] = reply = reply_to_strings(reply);
+                }
 
                 // TODO - confusing and error-prone that hgetall is special cased in two places
                 if (reply && args[0].toLowerCase() === "hgetall") {

test/test.js

@@ -792,6 +792,95 @@ tests.detect_buffers = function () {
     });
 };
 
+tests.detect_buffers_multi = function () {
+    var name = "detect_buffers_multi", detect_client = redis.createClient({detect_buffers: true});
+
+    detect_client.on("ready", function () {
+        // single Buffer or String
+        detect_client.set("string key 1", "string value");
+        detect_client.multi().get("string key 1").exec(require_string("string value", name));
+        detect_client.multi().get(new Buffer("string key 1")).exec(function (err, reply) {
+            assert.strictEqual(null, err, name);
+            assert.strictEqual(1, reply.length, name);
+            assert.strictEqual(true, Buffer.isBuffer(reply[0]), name);
+            assert.strictEqual("<Buffer 73 74 72 69 6e 67 20 76 61 6c 75 65>", reply[0].inspect(), name);
+        });
+
+        detect_client.hmset("hash key 2", "key 1", "val 1", "key 2", "val 2");
+        // array of Buffers or Strings
+        detect_client.multi().hmget("hash key 2", "key 1", "key 2").exec(function (err, reply) {
+            assert.strictEqual(null, err, name);
+            assert.strictEqual(true, Array.isArray(reply), name);
+            assert.strictEqual(1, reply.length, name);
+            assert.strictEqual(2, reply[0].length, name);
+            assert.strictEqual("val 1", reply[0][0], name);
+            assert.strictEqual("val 2", reply[0][1], name);
+        });
+        detect_client.multi().hmget(new Buffer("hash key 2"), "key 1", "key 2").exec(function (err, reply) {
+            assert.strictEqual(null, err, name);
+            assert.strictEqual(true, Array.isArray(reply));
+            assert.strictEqual(1, reply.length, name);
+            assert.strictEqual(2, reply[0].length, name);
+            assert.strictEqual(true, Buffer.isBuffer(reply[0][0]));
+            assert.strictEqual(true, Buffer.isBuffer(reply[0][1]));
+            assert.strictEqual("<Buffer 76 61 6c 20 31>", reply[0][0].inspect(), name);
+            assert.strictEqual("<Buffer 76 61 6c 20 32>", reply[0][1].inspect(), name);
+        });
+
+        // array of strings with undefined values (repro #344)
+        detect_client.multi().hmget("hash key 2", "key 3", "key 4").exec(function(err, reply) {
+            assert.strictEqual(null, err, name);
+            assert.strictEqual(true, Array.isArray(reply), name);
+            assert.strictEqual(1, reply.length, name);
+            assert.strictEqual(2, reply[0].length, name);
+            assert.equal(null, reply[0][0], name);
+            assert.equal(null, reply[0][1], name);
+        });
+
+        // Object of Buffers or Strings
+        detect_client.multi().hgetall("hash key 2").exec(function (err, reply) {
+            assert.strictEqual(null, err, name);
+            assert.strictEqual(1, reply.length, name);
+            assert.strictEqual("object", typeof reply[0], name);
+            assert.strictEqual(2, Object.keys(reply[0]).length, name);
+            assert.strictEqual("val 1", reply[0]["key 1"], name);
+            assert.strictEqual("val 2", reply[0]["key 2"], name);
+        });
+        detect_client.multi().hgetall(new Buffer("hash key 2")).exec(function (err, reply) {
+            assert.strictEqual(null, err, name);
+            assert.strictEqual(1, reply.length, name);
+            assert.strictEqual("object", typeof reply, name);
+            assert.strictEqual(2, Object.keys(reply[0]).length, name);
+            assert.strictEqual(true, Buffer.isBuffer(reply[0]["key 1"]));
+            assert.strictEqual(true, Buffer.isBuffer(reply[0]["key 2"]));
+            assert.strictEqual("<Buffer 76 61 6c 20 31>", reply[0]["key 1"].inspect(), name);
+            assert.strictEqual("<Buffer 76 61 6c 20 32>", reply[0]["key 2"].inspect(), name);
+        });
+
+        // Can interleave string and buffer results:
+        detect_client.multi()
+            .hget("hash key 2", "key 1")
+            .hget(new Buffer("hash key 2"), "key 1")
+            .hget("hash key 2", new Buffer("key 2"))
+            .hget("hash key 2", "key 2")
+            .exec(function (err, reply) {
+                assert.strictEqual(null, err, name);
+                assert.strictEqual(true, Array.isArray(reply));
+                assert.strictEqual(4, reply.length, name);
+                assert.strictEqual("val 1", reply[0], name);
+                assert.strictEqual(true, Buffer.isBuffer(reply[1]));
+                assert.strictEqual("<Buffer 76 61 6c 20 31>", reply[1].inspect(), name);
+                assert.strictEqual(true, Buffer.isBuffer(reply[2]));
+                assert.strictEqual("<Buffer 76 61 6c 20 32>", reply[2].inspect(), name);
+                assert.strictEqual("val 2", reply[3], name);
+            });
+
+        detect_client.quit(function (err, res) {
+            next(name);
+        });
+    });
+};
+
 tests.socket_nodelay = function () {
     var name = "socket_nodelay", c1, c2, c3, ready_count = 0, quit_count = 0;