From 6f1acb0678f7288973e05456bd8a4facf889635c Mon Sep 17 00:00:00 2001 From: Raymond Myers Date: Tue, 31 Mar 2015 23:09:57 -0700 Subject: [PATCH 1/5] Fixed bug where buffer hgetall's in a multi would throw exceptions --- index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/index.js b/index.js index f2d5ed43cc..abee2973b7 100644 --- a/index.js +++ b/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; } From f384d1c774d39090b9634502b230d7c29819135b Mon Sep 17 00:00:00 2001 From: Raymond Myers Date: Wed, 1 Apr 2015 00:10:23 -0700 Subject: [PATCH 2/5] Fixed the pub/sub logic accidentally stringing the first value of a multi/exec response --- index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/index.js b/index.js index abee2973b7..399ebf2259 100644 --- a/index.js +++ b/index.js @@ -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(); } From aea03d60beb499e547ca34a6cbdfad634f36c63c Mon Sep 17 00:00:00 2001 From: Raymond Myers Date: Wed, 1 Apr 2015 00:38:21 -0700 Subject: [PATCH 3/5] Fixed exec result arrays being stringed in detect_buffers mode --- index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/index.js b/index.js index 399ebf2259..5b4c6d56b5 100644 --- a/index.js +++ b/index.js @@ -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); From ded75c8ea2cbf1194c3426a2534cffe7167b45dc Mon Sep 17 00:00:00 2001 From: Raymond Myers Date: Wed, 1 Apr 2015 01:03:10 -0700 Subject: [PATCH 4/5] Fixed detect_buffers keeping all multi/exec results as buffers --- index.js | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/index.js b/index.js index 5b4c6d56b5..70479aeb18 100644 --- a/index.js +++ b/index.js @@ -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") { From 19c55712293b2bea82e64a22a7ea63c530c81d52 Mon Sep 17 00:00:00 2001 From: Raymond Myers Date: Wed, 1 Apr 2015 01:17:13 -0700 Subject: [PATCH 5/5] Added a test case for detect_buffer behavior in a multi/exec --- test/test.js | 89 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 89 insertions(+) diff --git a/test/test.js b/test/test.js index 3328673840..e7db763914 100644 --- a/test/test.js +++ b/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("", 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("", reply[0][0].inspect(), name); + assert.strictEqual("", 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("", reply[0]["key 1"].inspect(), name); + assert.strictEqual("", 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("", reply[1].inspect(), name); + assert.strictEqual(true, Buffer.isBuffer(reply[2])); + assert.strictEqual("", 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;