Add test and fix keeping the offline queue

Use a new delay after reconnecting

README.md

@@ -197,8 +197,8 @@ to maximum value, provided in milliseconds.
 * `connect_timeout` defaults to `86400000`. Setting `connect_timeout` limits total time for client to reconnect.
 Value is provided in milliseconds and is counted once the disconnect occured. The last retry is going to happen exactly at the timeout time.
 That way the default is to try reconnecting until 24h passed.
-* `max_attempts` defaults to `null`. By default client will try reconnecting until connected. Setting `max_attempts`
+* `max_attempts` defaults to `0`. By default client will try reconnecting until connected. Setting `max_attempts`
-limits total amount of reconnects. Setting this to 0 will prevent any reconnect tries.
+limits total amount of connection tries. Setting this to 1 will prevent any reconnect tries.
 * `auth_pass` defaults to `null`. By default client will try connecting without auth. If set, client will run redis auth command on connect.
 * `family` defaults to `IPv4`. The client connects in IPv4 if not specified or if the DNS resolution returns an IPv4 address.
 You can force an IPv6 if you set the family to 'IPv6'. See nodejs net or dns modules how to use the family type.

index.js

@@ -52,10 +52,7 @@ function RedisClient(stream, options) {
     this.should_buffer = false;
     this.command_queue_high_water = this.options.command_queue_high_water || 1000;
     this.command_queue_low_water = this.options.command_queue_low_water || 0;
-    this.max_attempts = null;
+    this.max_attempts = +options.max_attempts || 0;
-    if (options.max_attempts || options.max_attempts === 0 || options.max_attempts === '0') {
-        this.max_attempts = +options.max_attempts;
-    }
     this.command_queue = new Queue(); // holds sent commands to de-pipeline them
     this.offline_queue = new Queue(); // holds commands issued but not able to be sent
     this.commands_sent = 0;
@@ -418,16 +415,6 @@ RedisClient.prototype.connection_gone = function (why) {
         return;
     }
 
-    if (this.max_attempts !== null && this.attempts > this.max_attempts) {
-        this.emit('error', new Error("Redis connection in broken state: maximum connection attempts exceeded."));
-        return;
-    }
-
-    if (this.retry_totaltime >= this.connect_timeout) {
-        this.emit('error', new Error("Redis connection in broken state: connection timeout exceeded."));
-        return;
-    }
-
     debug("Redis connection is gone from " + why + " event.");
     this.connected = false;
     this.ready = false;
@@ -450,16 +437,23 @@ RedisClient.prototype.connection_gone = function (why) {
         this.emitted_end = true;
     }
 
-    this.flush_and_error("Redis connection gone from " + why + " event.");
-
     // If this is a requested shutdown, then don't retry
     if (this.closing) {
-        this.retry_timer = null;
+        debug("connection ended from quit command, not retrying.");
-        debug("Connection ended from quit command, not retrying.");
+        this.flush_and_error("Redis connection gone from " + why + " event.");
+        return;
+    }
+
+    if (this.max_attempts !== 0 && this.attempts >= this.max_attempts || this.retry_totaltime >= this.connect_timeout) {
+        this.flush_and_error("Redis connection gone from " + why + " event.");
+        this.end();
+        var message = this.retry_totaltime >= this.connect_timeout ?
+            'connection timeout exceeded.' :
+            'maximum connection attempts exceeded.';
+        this.emit('error', new Error("Redis connection in broken state: " + message));
         return;
     }
 
-    this.retry_delay = Math.floor(this.retry_delay * this.retry_backoff);
     if (this.retry_max_delay !== null && this.retry_delay > this.retry_max_delay) {
         this.retry_delay = this.retry_max_delay;
     } else if (this.retry_totaltime + this.retry_delay > this.connect_timeout) {
@@ -479,6 +473,7 @@ RedisClient.prototype.connection_gone = function (why) {
 
         self.retry_totaltime += self.retry_delay;
         self.attempts += 1;
+        self.retry_delay = Math.round(self.retry_delay * self.retry_backoff);
 
         self.stream = net.createConnection(self.connectionOption);
         self.install_stream_listeners();
@@ -834,7 +829,7 @@ RedisClient.prototype.end = function () {
     //clear retry_timer
     if(this.retry_timer){
         clearTimeout(this.retry_timer);
-        this.retry_timer=null;
+        this.retry_timer = null;
     }
     this.stream.on("error", function(){});
 

test/connection.spec.js

@@ -11,7 +11,7 @@ describe("on lost connection", function () {
         describe("using " + parser + " and " + ip, function () {
 
             it("emit an error after max retry attempts and do not try to reconnect afterwards", function (done) {
-                var max_attempts = 3;
+                var max_attempts = 4;
                 var client = redis.createClient({
                     parser: parser,
                     max_attempts: max_attempts
@@ -31,7 +31,7 @@ describe("on lost connection", function () {
                 client.on('error', function(err) {
                     if (/Redis connection in broken state: maximum connection attempts.*?exceeded./.test(err.message)) {
                         setTimeout(function () {
-                            assert.strictEqual(calls, max_attempts);
+                            assert.strictEqual(calls, max_attempts - 1);
                             done();
                         }, 1500);
                     }
@@ -40,7 +40,7 @@ describe("on lost connection", function () {
 
             it("emit an error after max retry timeout and do not try to reconnect afterwards", function (done) {
                 var connect_timeout = 1000; // in ms
-                var client = redis.createClient({
+                client = redis.createClient({
                     parser: parser,
                     connect_timeout: connect_timeout
                 });

test/node_redis.spec.js

@@ -688,7 +688,7 @@ describe("The node_redis client", function () {
                 describe('true', function () {
                     it("does not return an error and enqueues operation", function (done) {
                         var client = redis.createClient(9999, null, {
-                            max_attempts: 0,
+                            max_attempts: 1,
                             parser: parser
                         });
 
@@ -710,6 +710,34 @@ describe("The node_redis client", function () {
                             }, 25);
                         }, 50);
                     });
+
+                    it("enqueues operation and keep the queue while trying to reconnect", function (done) {
+                        var client = redis.createClient(9999, null, {
+                            max_attempts: 4,
+                            parser: parser
+                        });
+                        var i = 0;
+
+                        client.on('error', function(err) {
+                            if (err.message === 'Redis connection in broken state: maximum connection attempts exceeded.') {
+                                assert(i, 3);
+                                assert.strictEqual(client.offline_queue.length, 0);
+                                done();
+                            }
+                        });
+
+                        client.on('reconnecting', function(params) {
+                            i++;
+                            assert.equal(params.attempt, i);
+                            assert.strictEqual(client.offline_queue.length, 1);
+                        });
+
+                        client.set('foo', 'bar', function(err, result) {
+                            assert(i, 3);
+                            assert('Redis connection gone from error event', err.message);
+                            assert.strictEqual(client.offline_queue.length, 0);
+                        });
+                    });
                 });
 
                 describe('false', function () {