Skip to content

Commit c846ed7

Browse files
committed
Merge pull request #826 from fintura/multi
Fix multi error handling
2 parents 4d13903 + 21d8bdb commit c846ed7

File tree

4 files changed

+80
-22
lines changed

4 files changed

+80
-22
lines changed

.npmignore

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,3 +6,4 @@ generate_commands.js
66
multi_bench.js
77
test-unref.js
88
changelog.md
9+
*.log

index.js

Lines changed: 20 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -381,7 +381,6 @@ RedisClient.prototype.ready_check = function () {
381381
RedisClient.prototype.send_offline_queue = function () {
382382
var command_obj, buffered_writes = 0;
383383

384-
// TODO: Implement queue.pop() as it should be faster than shift and evaluate petka antonovs queue
385384
while (command_obj = this.offline_queue.shift()) {
386385
debug("Sending offline command: " + command_obj.command);
387386
buffered_writes += !this.send_command(command_obj.command, command_obj.args, command_obj.callback);
@@ -989,15 +988,15 @@ Multi.prototype.hmset = Multi.prototype.HMSET = function (key, args, callback) {
989988
return this;
990989
};
991990

992-
Multi.prototype.send_command = function (command, args, cb) {
991+
Multi.prototype.send_command = function (command, args, index, cb) {
993992
var self = this;
994993
this._client.send_command(command, args, function (err, reply) {
995994
if (err) {
996995
if (cb) {
997996
cb(err);
998-
} else {
999-
self.errors.push(err);
1000997
}
998+
err.position = index - 1;
999+
self.errors.push(err);
10011000
}
10021001
});
10031002
};
@@ -1023,7 +1022,7 @@ Multi.prototype.exec = Multi.prototype.EXEC = function (callback) {
10231022
break;
10241023
}
10251024
}
1026-
this.send_command(command, args, cb);
1025+
this.send_command(command, args, index, cb);
10271026
}
10281027

10291028
this._client.send_command('exec', [], function(err, replies) {
@@ -1036,9 +1035,10 @@ Multi.prototype.execute_callback = function (err, replies) {
10361035

10371036
if (err) {
10381037
if (err.code !== 'CONNECTION_BROKEN') {
1038+
err.code = 'EXECABORT';
1039+
err.errors = this.errors;
10391040
if (this.callback) {
1040-
this.errors.push(err);
1041-
this.callback(this.errors);
1041+
this.callback(err);
10421042
} else {
10431043
// Exclude CONNECTION_BROKEN so that error won't be emitted twice
10441044
this._client.emit('error', err);
@@ -1053,17 +1053,22 @@ Multi.prototype.execute_callback = function (err, replies) {
10531053
args = this.queue[i];
10541054

10551055
// If we asked for strings, even in detect_buffers mode, then return strings:
1056-
if (this._client.options.detect_buffers && this.wants_buffers[i] === false) {
1057-
replies[i - 1] = reply = reply_to_strings(reply);
1058-
}
1059-
1060-
// TODO - confusing and error-prone that hgetall is special cased in two places
1061-
if (reply && args[0] === "hgetall") {
1062-
replies[i - 1] = reply = reply_to_object(reply);
1056+
if (reply) {
1057+
if (this._client.options.detect_buffers && this.wants_buffers[i] === false) {
1058+
replies[i - 1] = reply = reply_to_strings(reply);
1059+
}
1060+
if (args[0] === "hgetall") {
1061+
// TODO - confusing and error-prone that hgetall is special cased in two places
1062+
replies[i - 1] = reply = reply_to_object(reply);
1063+
}
10631064
}
10641065

10651066
if (typeof args[args.length - 1] === "function") {
1066-
args[args.length - 1](null, reply);
1067+
if (reply instanceof Error) {
1068+
args[args.length - 1](reply);
1069+
} else {
1070+
args[args.length - 1](null, reply);
1071+
}
10671072
}
10681073
}
10691074
}

test/commands/multi.spec.js

Lines changed: 44 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -249,18 +249,55 @@ describe("The 'multi' method", function () {
249249
});
250250
});
251251

252-
it('reports multiple exceptions when they occur', function (done) {
253-
helper.serverVersionAtLeast.call(this, client, [2, 6, 5]);
252+
it('reports EXECABORT exceptions when they occur (while queueing)', function (done) {
253+
client.multi().config("bar").set("foo").exec(function (err, reply) {
254+
assert.equal(err.code, "EXECABORT");
255+
assert.equal(reply, undefined, "The reply should have been discarded");
256+
assert(err.message.match(/^EXECABORT/), "Error message should begin with EXECABORT");
257+
assert.equal(err.errors.length, 1, "err.errors should have 1 items");
258+
assert.strictEqual(err.errors[0].command_used, 'SET');
259+
assert.strictEqual(err.errors[0].position, 1);
260+
assert(/^ERR/.test(err.errors[0].message), "Actuall error message should begin with ERR");
261+
return done();
262+
});
263+
});
254264

255-
client.multi().set("foo").exec(function (err, reply) {
256-
assert(Array.isArray(err), "err should be an array");
257-
assert.equal(2, err.length, "err should have 2 items");
258-
assert(err[0].message.match(/^ERR/), "First error message should begin with ERR");
259-
assert(err[1].message.match(/^EXECABORT/), "First error message should begin with EXECABORT");
265+
it('reports multiple exceptions when they occur (while EXEC is running)', function (done) {
266+
client.multi().config("bar").debug("foo").exec(function (err, reply) {
267+
assert.strictEqual(reply.length, 2);
268+
assert(/^ERR/.test(reply[0].message), "Error message should begin with ERR");
269+
assert(/^ERR/.test(reply[1].message), "Error message should begin with ERR");
260270
return done();
261271
});
262272
});
263273

274+
it('reports multiple exceptions when they occur (while EXEC is running) and calls cb', function (done) {
275+
var multi = client.multi();
276+
multi.config("bar", helper.isError());
277+
multi.set('foo', 'bar', helper.isString('OK'));
278+
multi.debug("foo").exec(function (err, reply) {
279+
assert.strictEqual(reply.length, 3);
280+
assert(/^ERR/.test(reply[0].message), "Error message should begin with ERR");
281+
assert(/^ERR/.test(reply[2].message), "Error message should begin with ERR");
282+
assert.strictEqual(reply[1], "OK");
283+
client.get('foo', helper.isString('bar', done));
284+
});
285+
});
286+
287+
it("emits an error if no callback has been provided and execabort error occured", function (done) {
288+
helper.serverVersionAtLeast.call(this, client, [2, 6, 5]);
289+
290+
var multi = client.multi();
291+
multi.config("bar");
292+
multi.set("foo");
293+
multi.exec();
294+
295+
client.on('error', function(err) {
296+
assert.equal(err.code, "EXECABORT");
297+
done();
298+
});
299+
});
300+
264301
});
265302
});
266303
});

test/node_redis.spec.js

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,21 @@ describe("The node_redis client", function () {
7777
});
7878
});
7979

80+
it("connects correctly to localhost and no ready check", function (done) {
81+
client = redis.createClient(undefined, undefined, {
82+
no_ready_check: true
83+
});
84+
client.on("error", done);
85+
86+
client.once("ready", function () {
87+
client.set('foo', 'bar');
88+
client.get('foo', function(err, res) {
89+
assert.strictEqual(res, 'bar');
90+
done(err);
91+
});
92+
});
93+
});
94+
8095
it("throws on strange connection info", function () {
8196
try {
8297
redis.createClient(true);

0 commit comments

Comments
 (0)