Use connect_timeout also as the socket_timeout if explicitly provided

Fixes #587
Fixes #393
Closes #652
Closes #394

Author: Ruben Bridgewater

README.md

@@ -200,9 +200,9 @@ once the connection has been established. Setting `enable_offline_queue` to
 with an error, or an error will be emitted if no callback is specified.
 * `retry_max_delay`: *null*; By default every time the client tries to connect and fails the reconnection delay almost doubles.
 This delay normally grows infinitely, but setting `retry_max_delay` limits it to the maximum value, provided in milliseconds.
-* `connect_timeout`: *86400000*; Setting `connect_timeout` limits total time for client to reconnect.
-The value is provided in milliseconds and is counted once the disconnect occurred. The last retry is going to happen exactly at the timeout time.
-That way the default is to try reconnecting until 24h passed.
+* `connect_timeout`: *3600000*; Setting `connect_timeout` limits total time for client to connect and reconnect.
+The value is provided in milliseconds and is counted from the moment on a new client is created / a connection is lost. The last retry is going to happen exactly at the timeout time.
+Default is to try connecting until the default system socket timeout has been exceeded and to try reconnecting until 1h passed.
 * `max_attempts`: *0*; By default client will try reconnecting until connected. Setting `max_attempts`
 limits total amount of connection tries. Setting this to 1 will prevent any reconnect tries.
 * `auth_pass`: *null*; If set, client will run redis auth command on connect.

changelog.md

@@ -13,12 +13,14 @@ Features
 -  Refactored manual backpressure control ([@BridgeAR](https://github.com/BridgeAR))
  -  Removed the high water mark and low water mark. Such a mechanism should be implemented by a user instead
  -  The `drain` event is from now on only emitted if the stream really had to buffer
+-  Reduced the default connect_timeout to be one hour instead of 24h ([@BridgeAR](https://github.com/BridgeAR))
 
 Bugfixes
 
 -  Fixed a js parser error that could result in a timeout ([@BridgeAR](https://github.com/BridgeAR))
 -  Fixed .multi / .batch used with Node.js 0.10.x not working properly after a reconnect ([@BridgeAR](https://github.com/BridgeAR))
 -  Fixed fired but not yet returned commands not being rejected after a connection loss ([@BridgeAR](https://github.com/BridgeAR))
+-  Fixed connect_timeout not respected if no connection has ever been established ([@gagle](https://github.com/gagle) & [@benjie](https://github.com/benjie))
 
 ## v.2.2.5 - 18 Oct, 2015
 

index.js

@@ -85,7 +85,7 @@ function RedisClient(stream, options) {
     this.max_attempts = options.max_attempts | 0;
     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.connect_timeout = +options.connect_timeout || 86400000; // 24 * 60 * 60 * 1000 ms
+    this.connect_timeout = +options.connect_timeout || 3600000; // 60 * 60 * 1000 ms
     this.enable_offline_queue = options.enable_offline_queue === false ? false : true;
     this.retry_max_delay = +options.retry_max_delay || null;
     this.initialize_retry_vars();
@@ -108,6 +108,13 @@ util.inherits(RedisClient, events.EventEmitter);
 RedisClient.prototype.install_stream_listeners = function() {
     var self = this;
 
+    if (this.options.connect_timeout) {
+        this.stream.setTimeout(this.connect_timeout, function () {
+            self.retry_totaltime = self.connect_timeout;
+            self.connection_gone('timeout');
+        });
+    }
+
     this.stream.on('connect', function () {
         self.on_connect();
     });

test/connection.spec.js

@@ -125,6 +125,48 @@ describe("connection tests", function () {
 
             describe("when not connected", function () {
 
+                it("emit an error after the socket timeout exceeded the connect_timeout time", function (done) {
+                    var connect_timeout = 1000; // in ms
+                    var client = redis.createClient({
+                        parser: parser,
+                        host: '1.2.3.4',
+                        connect_timeout: connect_timeout
+                    });
+                    assert(client.stream._events.timeout);
+                    assert.strictEqual(client.address, '1.2.3.4:6379');
+                    var time = Date.now();
+
+                    client.on("reconnecting", function (params) {
+                        throw new Error('No reconnect, since no connection was ever established');
+                    });
+
+                    client.on('error', function(err) {
+                        assert(/Redis connection in broken state: connection timeout.*?exceeded./.test(err.message));
+                        assert(Date.now() - time < connect_timeout + 50);
+                        done();
+                    });
+                });
+
+                it("use the system socket timeout if the connect_timeout has not been provided", function () {
+                    var client = redis.createClient({
+                        parser: parser,
+                        host: '1.2.3.4'
+                    });
+                    assert(client.stream._events.timeout === undefined);
+                });
+
+                it("clears the socket timeout after a connection has been established", function (done) {
+                    var client = redis.createClient({
+                        parser: parser,
+                        connect_timeout: 1000
+                    });
+                    assert.strictEqual(client.stream._idleTimeout, 1000);
+                    client.on('connect', function () {
+                        assert.strictEqual(client.stream._idleTimeout, -1);
+                        done();
+                    });
+                });
+
                 it("connect with host and port provided in the options object", function (done) {
                     client = redis.createClient({
                         host: 'localhost',