Merge pull request #826 from fintura/multi

Fix multi error handling

Author: BridgeAR

.npmignore

@@ -6,3 +6,4 @@ generate_commands.js
 multi_bench.js
 test-unref.js
 changelog.md
+*.log

index.js

@@ -381,7 +381,6 @@ RedisClient.prototype.ready_check = function () {
 RedisClient.prototype.send_offline_queue = function () {
     var command_obj, buffered_writes = 0;
 
-    // TODO: Implement queue.pop() as it should be faster than shift and evaluate petka antonovs queue
     while (command_obj = this.offline_queue.shift()) {
         debug("Sending offline command: " + command_obj.command);
         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) {
     return this;
 };
 
-Multi.prototype.send_command = function (command, args, cb) {
+Multi.prototype.send_command = function (command, args, index, cb) {
     var self = this;
     this._client.send_command(command, args, function (err, reply) {
         if (err) {
             if (cb) {
                 cb(err);
-            } else {
-                self.errors.push(err);
             }
+            err.position = index - 1;
+            self.errors.push(err);
         }
     });
 };
@@ -1023,7 +1022,7 @@ Multi.prototype.exec = Multi.prototype.EXEC = function (callback) {
                 break;
             }
         }
-        this.send_command(command, args, cb);
+        this.send_command(command, args, index, cb);
     }
 
     this._client.send_command('exec', [], function(err, replies) {
@@ -1036,9 +1035,10 @@ Multi.prototype.execute_callback = function (err, replies) {
 
     if (err) {
         if (err.code !== 'CONNECTION_BROKEN') {
+            err.code = 'EXECABORT';
+            err.errors = this.errors;
             if (this.callback) {
-                this.errors.push(err);
-                this.callback(this.errors);
+                this.callback(err);
             } else {
                 // Exclude CONNECTION_BROKEN so that error won't be emitted twice
                 this._client.emit('error', err);
@@ -1053,17 +1053,22 @@ Multi.prototype.execute_callback = function (err, replies) {
             args = this.queue[i];
 
             // If we asked for strings, even in detect_buffers mode, then return strings:
-            if (this._client.options.detect_buffers && this.wants_buffers[i] === 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] === "hgetall") {
-                replies[i - 1] = reply = reply_to_object(reply);
+            if (reply) {
+                if (this._client.options.detect_buffers && this.wants_buffers[i] === false) {
+                    replies[i - 1] = reply = reply_to_strings(reply);
+                }
+                if (args[0] === "hgetall") {
+                    // TODO - confusing and error-prone that hgetall is special cased in two places
+                    replies[i - 1] = reply = reply_to_object(reply);
+                }
             }
 
             if (typeof args[args.length - 1] === "function") {
-                args[args.length - 1](null, reply);
+                if (reply instanceof Error) {
+                    args[args.length - 1](reply);
+                } else {
+                    args[args.length - 1](null, reply);
+                }
             }
         }
     }

test/commands/multi.spec.js

@@ -249,18 +249,55 @@ describe("The 'multi' method", function () {
                         });
                 });
 
-                it('reports multiple exceptions when they occur', function (done) {
-                    helper.serverVersionAtLeast.call(this, client, [2, 6, 5]);
+                it('reports EXECABORT exceptions when they occur (while queueing)', function (done) {
+                    client.multi().config("bar").set("foo").exec(function (err, reply) {
+                        assert.equal(err.code, "EXECABORT");
+                        assert.equal(reply, undefined, "The reply should have been discarded");
+                        assert(err.message.match(/^EXECABORT/), "Error message should begin with EXECABORT");
+                        assert.equal(err.errors.length, 1, "err.errors should have 1 items");
+                        assert.strictEqual(err.errors[0].command_used, 'SET');
+                        assert.strictEqual(err.errors[0].position, 1);
+                        assert(/^ERR/.test(err.errors[0].message), "Actuall error message should begin with ERR");
+                        return done();
+                    });
+                });
 
-                    client.multi().set("foo").exec(function (err, reply) {
-                        assert(Array.isArray(err), "err should be an array");
-                        assert.equal(2, err.length, "err should have 2 items");
-                        assert(err[0].message.match(/^ERR/), "First error message should begin with ERR");
-                        assert(err[1].message.match(/^EXECABORT/), "First error message should begin with EXECABORT");
+                it('reports multiple exceptions when they occur (while EXEC is running)', function (done) {
+                    client.multi().config("bar").debug("foo").exec(function (err, reply) {
+                        assert.strictEqual(reply.length, 2);
+                        assert(/^ERR/.test(reply[0].message), "Error message should begin with ERR");
+                        assert(/^ERR/.test(reply[1].message), "Error message should begin with ERR");
                         return done();
                     });
                 });
 
+                it('reports multiple exceptions when they occur (while EXEC is running) and calls cb', function (done) {
+                    var multi = client.multi();
+                    multi.config("bar", helper.isError());
+                    multi.set('foo', 'bar', helper.isString('OK'));
+                    multi.debug("foo").exec(function (err, reply) {
+                        assert.strictEqual(reply.length, 3);
+                        assert(/^ERR/.test(reply[0].message), "Error message should begin with ERR");
+                        assert(/^ERR/.test(reply[2].message), "Error message should begin with ERR");
+                        assert.strictEqual(reply[1], "OK");
+                        client.get('foo', helper.isString('bar', done));
+                    });
+                });
+
+                it("emits an error if no callback has been provided and execabort error occured", function (done) {
+                    helper.serverVersionAtLeast.call(this, client, [2, 6, 5]);
+
+                    var multi = client.multi();
+                    multi.config("bar");
+                    multi.set("foo");
+                    multi.exec();
+
+                    client.on('error', function(err) {
+                        assert.equal(err.code, "EXECABORT");
+                        done();
+                    });
+                });
+
             });
         });
     });

test/node_redis.spec.js

@@ -77,6 +77,21 @@ describe("The node_redis client", function () {
                     });
                 });
 
+                it("connects correctly to localhost and no ready check", function (done) {
+                    client = redis.createClient(undefined, undefined, {
+                        no_ready_check: true
+                    });
+                    client.on("error", done);
+
+                    client.once("ready", function () {
+                        client.set('foo', 'bar');
+                        client.get('foo', function(err, res) {
+                            assert.strictEqual(res, 'bar');
+                            done(err);
+                        });
+                    });
+                });
+
                 it("throws on strange connection info", function () {
                     try {
                         redis.createClient(true);