Merge pull request #1016 from NodeRedis/properly-quit

Calling quit should always close the connection

Author: BridgeAR

README.md

@@ -277,10 +277,16 @@ something like this `Error: Ready check failed: ERR operation not permitted`.
 The client exposed the used [stream](https://nodejs.org/api/stream.html) in `client.stream` and if the stream or client had to [buffer](https://nodejs.org/api/stream.html#stream_writable_write_chunk_encoding_callback) the command in `client.should_buffer`.
 In combination this can be used to implement backpressure by checking the buffer state before sending a command and listening to the stream [drain](https://nodejs.org/api/stream.html#stream_event_drain) event.
 
+## client.quit()
+
+This sends the quit command to the redis server and ends cleanly right after all running commands were properly handled.
+If this is called while reconnecting (and therefor no connection to the redis server exists) it is going to end the connection right away instead of
+resulting in further reconnections! All offline commands are going to be flushed with an error in that case.
+
 ## client.end(flush)
 
 Forcibly close the connection to the Redis server. Note that this does not wait until all replies have been parsed.
-If you want to exit cleanly, call `client.quit()` to send the `QUIT` command after you have handled all replies.
+If you want to exit cleanly, call `client.quit()` as mentioned above.
 
 You should set flush to true, if you are not absolutely sure you do not care about any other commands.
 If you set flush to false all still running commands will silently fail.

changelog.md

@@ -10,6 +10,7 @@ Features
 -  Activating monitor mode does now work together with arbitrary commands including pub sub mode
 -  Pub sub mode is completly rewritten and all known issues fixed
 -  Added `string_numbers` option to get back strings instead of numbers
+-  Quit command is from now on always going to end the connection properly
 
 Bugfixes
 
@@ -21,6 +22,7 @@ Bugfixes
 -  Fixed pub sub mode crashing if calling unsubscribe / subscribe in various combinations
 -  Fixed pub sub mode emitting unsubscribe even if no channels were unsubscribed
 -  Fixed pub sub mode emitting a message without a message published
+-  Fixed quit command not ending the connection and resulting in further reconnection if called while reconnecting
 
 ## v.2.5.3 - 21 Mar, 2016
 

index.js

@@ -751,6 +751,7 @@ function handle_offline_command (self, command_obj) {
         }
         err = new Error(command + " can't be processed. " + msg);
         err.command = command;
+        err.code = 'NR_OFFLINE';
         utils.reply_in_order(self, callback, err);
     } else {
         debug('Queueing ' + command + ' for next server connection.');
@@ -845,8 +846,6 @@ RedisClient.prototype.send_command = function (command, args, callback) {
         if (!this.pub_sub_mode) {
             this.pub_sub_mode = this.command_queue.length + 1;
         }
-    } else if (command === 'quit') {
-        this.closing = true;
     }
     this.command_queue.push(command_obj);
 

lib/individualCommands.js

@@ -68,6 +68,30 @@ RedisClient.prototype.monitor = RedisClient.prototype.MONITOR = function (callba
     });
 };
 
+RedisClient.prototype.quit = RedisClient.prototype.QUIT = function (callback) {
+    var self = this;
+    var callback_hook = function (err, res) {
+        // TODO: Improve this by handling everything with coherend error codes and find out if there's anything missing
+        if (err && (err.code === 'NR_OFFLINE' ||
+            err.message === 'Redis connection gone from close event.' ||
+            err.message === 'The command can\'t be processed. The connection has already been closed.'
+        )) {
+            // Pretent the quit command worked properly in this case.
+            // Either the quit landed in the offline queue and was flushed at the reconnect
+            // or the offline queue is deactivated and the command was rejected right away
+            // or the stream is not writable
+            // or while sending the quit, the connection dropped
+            err = null;
+            res = 'OK';
+        }
+        utils.callback_or_emit(self, callback, err, res);
+    };
+    var backpressure_indicator = this.send_command('quit', [], callback_hook);
+    // Calling quit should always end the connection, no matter if there's a connection or not
+    this.closing = true;
+    return backpressure_indicator;
+};
+
 // Store info in this.server_info after each call
 RedisClient.prototype.info = RedisClient.prototype.INFO = function info (section, callback) {
     var self = this;

test/connection.spec.js

@@ -34,6 +34,85 @@ describe('connection tests', function () {
         assert.strictEqual(client.stream.listeners('error').length, 1);
     });
 
+    describe('quit on lost connections', function () {
+
+        it('calling quit while the connection is down should not end in reconnecting version a', function (done) {
+            var called = 0;
+            client = redis.createClient({
+                port: 9999,
+                retry_strategy: function (options) {
+                    var bool = client.quit(function (err, res) {
+                        assert.strictEqual(res, 'OK');
+                        assert.strictEqual(err, null);
+                        assert.strictEqual(called++, -1);
+                        setTimeout(done, 25);
+                    });
+                    assert.strictEqual(bool, false);
+                    assert.strictEqual(called++, 0);
+                    return 5;
+                }
+            });
+            client.set('foo', 'bar', function (err, res) {
+                assert.strictEqual(err.message, 'Redis connection gone from close event.');
+                called = -1;
+            });
+        });
+
+        it('calling quit while the connection is down should not end in reconnecting version b', function (done) {
+            var called = false;
+            client = redis.createClient(9999);
+            client.set('foo', 'bar', function (err, res) {
+                assert.strictEqual(err.message, 'Redis connection gone from close event.');
+                called = true;
+            });
+            var bool = client.quit(function (err, res) {
+                assert.strictEqual(res, 'OK');
+                assert.strictEqual(err, null);
+                assert(called);
+                done();
+            });
+            assert.strictEqual(bool, false);
+        });
+
+        it('calling quit while the connection is down without offline queue should end the connection right away', function (done) {
+            var called = false;
+            client = redis.createClient(9999, {
+                enable_offline_queue: false
+            });
+            client.set('foo', 'bar', function (err, res) {
+                assert.strictEqual(err.message, 'SET can\'t be processed. The connection is not yet established and the offline queue is deactivated.');
+                called = true;
+            });
+            var bool = client.quit(function (err, res) {
+                assert.strictEqual(res, 'OK');
+                assert.strictEqual(err, null);
+                assert(called);
+                done();
+            });
+            assert.strictEqual(bool, false);
+        });
+
+        it('do not quit before connected or a connection issue is detected', function (done) {
+            client = redis.createClient();
+            client.set('foo', 'bar', helper.isString('OK'));
+            var bool = client.quit(done);
+            assert.strictEqual(bool, false);
+        });
+
+        it('quit right away if connection drops while quit command is on the fly', function (done) {
+            client = redis.createClient();
+            client.once('ready', function () {
+                client.set('foo', 'bar', helper.isError());
+                var bool = client.quit(done);
+                assert.strictEqual(bool, true);
+                process.nextTick(function () {
+                    client.stream.destroy();
+                });
+            });
+        });
+
+    });
+
     helper.allTests(function (parser, ip, args) {
 
         describe('using ' + parser + ' and ' + ip, function () {

test/node_redis.spec.js

@@ -235,12 +235,11 @@ describe('The node_redis client', function () {
                                 assert.strictEqual(client.offline_queue.length, 0);
                                 done();
                             });
-                        }, 100);
+                        }, 50);
                     });
 
                     it('emit an error', function (done) {
                         if (helper.redisProcess().spawnFailed()) this.skip();
-
                         client.quit();
                         client.on('error', function (err) {
                             assert.strictEqual(err.message, 'SET can\'t be processed. The connection has already been closed.');