Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for multi/exec logic with detect_buffers enabled #733

Merged
merged 5 commits into from
Jul 12, 2015
Merged

Fix for multi/exec logic with detect_buffers enabled #733

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6f1acb0
Fixed bug where buffer hgetall's in a multi would throw exceptions
raydog Apr 1, 2015
f384d1c
Fixed the pub/sub logic accidentally stringing the first value of a m…
raydog Apr 1, 2015
aea03d6
Fixed exec result arrays being stringed in detect_buffers mode
raydog Apr 1, 2015
ded75c8
Fixed detect_buffers keeping all multi/exec results as buffers
raydog Apr 1, 2015
19c5571
Added a test case for detect_buffer behavior in a multi/exec
raydog Apr 1, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 20 additions & 5 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down Expand Up @@ -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();
}

Expand All @@ -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);
Expand Down Expand Up @@ -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];
}
Expand Down Expand Up @@ -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") {
Expand Down
89 changes: 89 additions & 0 deletions test/test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down