Fix commands not being rejected after calling .quit as reported in #791

Ruben Bridgewater · Ruben Bridgewater · commit a0c906256c80 · 2015-09-16T05:29:34.000+02:00
diff --git a/index.js b/index.js
@@ -661,7 +661,7 @@ function Command(command, args, sub_command, buffer_args, callback) {
 }
 
 RedisClient.prototype.send_command = function (command, args, callback) {
-    var arg, command_obj, i, elem_count, buffer_args, stream = this.stream, command_str = "", buffered_writes = 0;
+    var arg, command_obj, i, elem_count, buffer_args, stream = this.stream, command_str = "", buffered_writes = 0, err;
 
     // if (typeof callback === "function") {}
     // probably the fastest way:
@@ -698,7 +698,9 @@ RedisClient.prototype.send_command = function (command, args, callback) {
             return;
         }
         if (args[args.length - 1] === undefined || args[args.length - 1] === null) {
-            var err = new Error('send_command: ' + command + ' value must not be undefined or null');
+            command = command.toUpperCase();
+            err = new Error('send_command: ' + command + ' value must not be undefined or null');
+            err.command_used = command;
             if (callback) {
                 return callback && callback(err);
             }
@@ -718,24 +720,25 @@ RedisClient.prototype.send_command = function (command, args, callback) {
     command_obj = new Command(command, args, false, buffer_args, callback);
 
     if (!this.ready && !this.send_anyway || !stream.writable) {
-        if (this.enable_offline_queue) {
-            if (!stream.writable) {
-                debug("send command: stream is not writeable.");
+        if (this.closing || !this.enable_offline_queue) {
+            command = command.toUpperCase();
+            if (!this.closing) {
+                err = new Error(command + ' can\'t be processed. Stream not writeable and enable_offline_queue is deactivated.');
+            } else {
+                err = new Error(command + ' can\'t be processed. The connection has already been closed.');
+            }
+            err.command_used = command;
+            if (callback) {
+                callback(err);
+            } else {
+                this.emit('error', err);
             }
+        } else {
             debug("Queueing " + command + " for next server connection.");
             this.offline_queue.push(command_obj);
             this.should_buffer = true;
-        } else {
-            var not_writeable_error = new Error('send_command: stream not writeable. enable_offline_queue is false');
-            if (command_obj.callback) {
-                command_obj.callback(not_writeable_error);
-            } else {
-                this.emit("error", not_writeable_error);
-                return;
-            }
         }
-
-        return false;
+        return;
     }
 
     if (command === "subscribe" || command === "psubscribe" || command === "unsubscribe" || command === "punsubscribe") {
diff --git a/test/commands/dbsize.spec.js b/test/commands/dbsize.spec.js
@@ -34,7 +34,7 @@ describe("The 'dbsize' method", function () {
 
                 it("reports an error", function (done) {
                     client.dbsize([], function (err, res) {
-                        assert(err.message.match(/Redis connection gone/));
+                        assert(err.message.match(/The connection has already been closed/));
                         done();
                     });
                 });
diff --git a/test/commands/flushdb.spec.js b/test/commands/flushdb.spec.js
@@ -34,7 +34,7 @@ describe("The 'flushdb' method", function () {
 
                 it("reports an error", function (done) {
                     client.flushdb(function (err, res) {
-                        assert(err.message.match(/Redis connection gone/));
+                        assert(err.message.match(/The connection has already been closed/));
                         done();
                     });
                 });
diff --git a/test/commands/get.spec.js b/test/commands/get.spec.js
@@ -34,7 +34,7 @@ describe("The 'get' method", function () {
 
                 it("reports an error", function (done) {
                     client.get(key, function (err, res) {
-                        assert(err.message.match(/Redis connection gone/));
+                        assert(err.message.match(/The connection has already been closed/));
                         done();
                     });
                 });
diff --git a/test/commands/getset.spec.js b/test/commands/getset.spec.js
@@ -36,7 +36,7 @@ describe("The 'getset' method", function () {
                 it("reports an error", function (done) {
                     client.GET(key, redis.print); // Use the utility function to print the error
                     client.get(key, function (err, res) {
-                        assert(err.message.match(/Redis connection gone/));
+                        assert(err.message.match(/The connection has already been closed/));
                         done();
                     });
                 });
diff --git a/test/commands/incr.spec.js b/test/commands/incr.spec.js
@@ -35,7 +35,7 @@ describe("The 'incr' method", function () {
 
                 it("reports an error", function (done) {
                     client.incr(function (err, res) {
-                        assert(err.message.match(/Redis connection gone/));
+                        assert(err.message.match(/The connection has already been closed/));
                         done();
                     });
                 });
diff --git a/test/commands/mset.spec.js b/test/commands/mset.spec.js
@@ -36,7 +36,7 @@ describe("The 'mset' method", function () {
 
                 it("reports an error", function (done) {
                     client.mset(key, value, key2, value2, function (err, res) {
-                        assert(err.message.match(/Redis connection gone/));
+                        assert(err.message.match(/The connection has already been closed/));
                         done();
                     });
                 });
diff --git a/test/commands/multi.spec.js b/test/commands/multi.spec.js
@@ -35,7 +35,7 @@ describe("The 'multi' method", function () {
                 it("reports an error", function (done) {
                     client.multi();
                     client.exec(function (err, res) {
-                        assert(err.message.match(/Redis connection gone/));
+                        assert(err.message.match(/The connection has already been closed/));
                         done();
                     });
                 });
diff --git a/test/commands/select.spec.js b/test/commands/select.spec.js
@@ -26,7 +26,7 @@ describe("The 'select' method", function () {
 
                 it("returns an error if redis is not connected", function (done) {
                     client.select(1, function (err, res) {
-                        assert(err.message.match(/Redis connection gone/));
+                        assert(err.message.match(/The connection has already been closed/));
                         done();
                     });
                 });
diff --git a/test/commands/set.spec.js b/test/commands/set.spec.js
@@ -34,7 +34,7 @@ describe("The 'set' method", function () {
 
                 it("reports an error", function (done) {
                     client.set(key, value, function (err, res) {
-                        assert(err.message.match(/Redis connection gone/));
+                        assert(err.message.match(/The connection has already been closed/));
                         done();
                     });
                 });
diff --git a/test/node_redis.spec.js b/test/node_redis.spec.js
@@ -155,6 +155,54 @@ describe("The node_redis client", function () {
 
                 });
 
+                describe("commands after using .quit should fail", function () {
+
+                    it("return an error in the callback", function (done) {
+                        if (helper.redisProcess().spawnFailed()) this.skip();
+
+                        var client = redis.createClient();
+                        client.quit(function() {
+                            client.get("foo", function(err, res) {
+                                assert.strictEqual(err.message, 'Redis connection gone from end event.');
+                                assert.strictEqual(client.offline_queue.length, 0);
+                                done();
+                            });
+                        });
+                    });
+
+                    it("return an error in the callback version two", function (done) {
+                        if (helper.redisProcess().spawnFailed()) this.skip();
+
+                        var client = redis.createClient();
+                        client.quit();
+                        setTimeout(function() {
+                            client.get("foo", function(err, res) {
+                                assert.strictEqual(err.message, 'GET can\'t be processed. The connection has already been closed.');
+                                assert.strictEqual(err.command_used, 'GET');
+                                assert.strictEqual(client.offline_queue.length, 0);
+                                done();
+                            });
+                        }, 100);
+                    });
+
+                    it("emit an error", function (done) {
+                        if (helper.redisProcess().spawnFailed()) this.skip();
+
+                        var client = redis.createClient();
+                        client.quit();
+                        client.on('error', function(err) {
+                            assert.strictEqual(err.message, 'SET can\'t be processed. The connection has already been closed.');
+                            assert.strictEqual(err.command_used, 'SET');
+                            assert.strictEqual(client.offline_queue.length, 0);
+                            done();
+                        });
+                        setTimeout(function() {
+                            client.set('foo', 'bar');
+                        }, 50);
+                    });
+
+                });
+
                 describe("when redis closes unexpectedly", function () {
                     it("reconnects and can retrieve the pre-existing data", function (done) {
                         client.on("reconnecting", function on_recon(params) {
@@ -712,15 +760,18 @@ describe("The node_redis client", function () {
                 });
 
                 describe('false', function () {
-                    it("does not emit an error and enqueues operation", function (done) {
+                    it("does emit an error and does not enqueues operation", function (done) {
                         var client = redis.createClient(9999, null, {
                             parser: parser,
                             max_attempts: 1,
                             enable_offline_queue: false
                         });
+                        var end = helper.callFuncAfter(done, 3);
 
                         client.on('error', function(err) {
-                            assert(/send_command: stream not writeable|ECONNREFUSED/.test(err.message));
+                            assert(/Stream not writeable|ECONNREFUSED/.test(err.message));
+                            assert.equal(client.command_queue.length, 0);
+                            end();
                         });
 
                         client.set('foo', 'bar');
@@ -729,9 +780,7 @@ describe("The node_redis client", function () {
                             client.set('foo', 'bar', function (err) {
                                 // should callback with an error
                                 assert.ok(err);
-                                setTimeout(function () {
-                                    return done();
-                                }, 50);
+                                setTimeout(end, 50);
                             });
                         });
                     });
