Repository: luin/ioredis
Pull request #648 (2018-06-29 18:06:44)

feat: `Redis#connect()` will be resolved when status is ready
BREAKING CHANGE:
`Redis#connect()` will be resolved when status is ready
instead of `connect`:

```
const redis = new Redis({ lazyConnect: true })
redis.connect().then(() => {
  assert(redis.status === 'ready')
})
```


Patch:
diff --git a/API.md b/API.md
index f77ff53..8d8072d 100644
--- a/API.md
+++ b/API.md
@@ -18,7 +18,7 @@
 * [Redis](#Redis) ⇐ <code>[EventEmitter](http://nodejs.org/api/events.html#events_class_events_eventemitter)</code>
     * [new Redis([port], [host], [options])](#new_Redis_new)
     * _instance_
-        * [.connect([callback])](#Redis+connect) ⇒ <code>Promise</code>
+        * [.connect([callback])](#Redis+connect) ⇒ <code>Promise.&lt;void&gt;</code>
         * [.disconnect()](#Redis+disconnect)
         * ~~[.end()](#Redis+end)~~
         * [.duplicate()](#Redis+duplicate)
@@ -68,7 +68,6 @@ Creates a Redis instance
 var Redis = require('ioredis');
 
 var redis = new Redis();
-// or: var redis = Redis();
 
 var redisOnPort6380 = new Redis(6380);
 var anotherRedis = new Redis(6380, '192.168.100.1');
@@ -80,9 +79,13 @@ var authedRedis = new Redis(6380, '192.168.100.1', { password: 'password' });
 ```
 <a name="Redis+connect"></a>
 
-### redis.connect([callback]) ⇒ <code>Promise</code>
+### redis.connect([callback]) ⇒ <code>Promise.&lt;void&gt;</code>
 Create a connection to Redis.
-This method will be invoked automatically when creating a new Redis instance.
+This method will be invoked automatically when creating a new Redis instance
+unless `lazyConnect: true` is passed.
+
+When calling this method manually, a Promise is returned, which will
+be resolved when the connection status is ready.
 
 **Kind**: instance method of [<code>Redis</code>](#Redis)  
 **Access**: public  
@@ -234,6 +237,7 @@ Creates a Redis Cluster instance
 | [options.retryDelayOnClusterDown] | <code>number</code> | <code>100</code> | When a CLUSTERDOWN error is received, client will retry if `retryDelayOnClusterDown` is valid delay time. |
 | [options.retryDelayOnTryAgain] | <code>number</code> | <code>100</code> | When a TRYAGAIN error is received, client will retry if `retryDelayOnTryAgain` is valid delay time. |
 | [options.slotsRefreshTimeout] | <code>number</code> | <code>1000</code> | The milliseconds before a timeout occurs while refreshing slots from the cluster. |
+| [options.slotsRefreshInterval] | <code>number</code> | <code>5000</code> | The milliseconds between every automatic slots refresh. |
 | [options.redisOptions] | <code>Object</code> |  | Passed to the constructor of `Redis`. |
 
 <a name="Cluster+connect"></a>
diff --git a/lib/cluster/index.js b/lib/cluster/index.js
index 473e869..7b63f67 100644
--- a/lib/cluster/index.js
+++ b/lib/cluster/index.js
@@ -135,13 +135,6 @@ Cluster.prototype.resetNodesRefreshInterval = function () {
  * @public
  */
 Cluster.prototype.connect = function () {
-  function readyHandler() {
-    this.setStatus('ready');
-    this.retryAttempts = 0;
-    this.executeOfflineCommands();
-    this.resetNodesRefreshInterval();
-  }
-
   var Promise = PromiseContainer.get();
   return new Promise(function (resolve, reject) {
     if (this.status === 'connecting' || this.status === 'connect' || this.status === 'ready') {
@@ -156,6 +149,14 @@ Cluster.prototype.connect = function () {
 
     this.connectionPool.reset(this.startupNodes);
 
+    function readyHandler() {
+      this.setStatus('ready');
+      this.retryAttempts = 0;
+      this.executeOfflineCommands();
+      this.resetNodesRefreshInterval();
+      resolve();
+    }
+
     var closeListener;
     var refreshListener = function () {
       this.removeListener('close', closeListener);
@@ -164,7 +165,7 @@ Cluster.prototype.connect = function () {
       if (this.options.enableReadyCheck) {
         this._readyCheck(function (err, fail) {
           if (err || fail) {
-            debug('Ready check failed (%s). Reconnecting...', err || fail)
+            debug('Ready check failed (%s). Reconnecting...', err || fail);
             if (this.status === 'connect') {
               this.disconnect(true);
             }
@@ -175,7 +176,6 @@ Cluster.prototype.connect = function () {
       } else {
         readyHandler.call(this);
       }
-      resolve();
     };
 
     closeListener = function () {
@@ -276,7 +276,7 @@ Cluster.prototype.quit = function (callback) {
 
   var Promise = PromiseContainer.get();
   if (status === 'wait') {
-    var ret = asCallback(Promise.resolve('OK'), callback)
+    var ret = asCallback(Promise.resolve('OK'), callback);
 
     // use setImmediate to make sure "close" event
     // being emitted after quit() is returned
@@ -530,9 +530,9 @@ Cluster.prototype.sendCommand = function (command, stream, node) {
             if (typeof to === 'function') {
               var nodes =
                   nodeKeys
-                  .map(function (key) {
-                    return _this.connectionPool.nodes.all[key];
-                  });
+                    .map(function (key) {
+                      return _this.connectionPool.nodes.all[key];
+                    });
               redis = to(nodes, command);
               if (Array.isArray(redis)) {
                 redis = utils.sample(redis);
@@ -603,7 +603,11 @@ Cluster.prototype.handleError = function (error, ttl, handlers) {
       timeout: this.options.retryDelayOnClusterDown,
       callback: this.refreshSlotsCache.bind(this)
     });
-  } else if (error.message === utils.CONNECTION_CLOSED_ERROR_MSG && this.options.retryDelayOnFailover > 0 && this.status === 'ready') {
+  } else if (
+    error.message === utils.CONNECTION_CLOSED_ERROR_MSG &&
+    this.options.retryDelayOnFailover > 0 &&
+    this.status === 'ready'
+  ) {
     this.delayQueue.push('failover', handlers.connectionClosed, {
       timeout: this.options.retryDelayOnFailover,
       callback: this.refreshSlotsCache.bind(this)
@@ -683,16 +687,16 @@ Cluster.prototype._readyCheck = function (callback) {
 };
 
 ['sscan', 'hscan', 'zscan', 'sscanBuffer', 'hscanBuffer', 'zscanBuffer']
-.forEach(function (command) {
-  Cluster.prototype[command + 'Stream'] = function (key, options) {
-    return new ScanStream(_.defaults({
-      objectMode: true,
-      key: key,
-      redis: this,
-      command: command
-    }, options));
-  };
-});
+  .forEach(function (command) {
+    Cluster.prototype[command + 'Stream'] = function (key, options) {
+      return new ScanStream(_.defaults({
+        objectMode: true,
+        key: key,
+        redis: this,
+        command: command
+      }, options));
+    };
+  });
 
 require('../transaction').addTransactionSupport(Cluster.prototype);
 
diff --git a/lib/redis.js b/lib/redis.js
index fc5addd..c9cf2c0 100644
--- a/lib/redis.js
+++ b/lib/redis.js
@@ -239,9 +239,13 @@ Redis.prototype.setStatus = function (status, arg) {
 
 /**
  * Create a connection to Redis.
- * This method will be invoked automatically when creating a new Redis instance.
+ * This method will be invoked automatically when creating a new Redis instance
+ * unless `lazyConnect: true` is passed.
+ *
+ * When calling this method manually, a Promise is returned, which will
+ * be resolved when the connection status is ready.
  * @param {function} [callback]
- * @return {Promise}
+ * @return {Promise<void>}
  * @public
  */
 Redis.prototype.connect = function (callback) {
@@ -253,9 +257,11 @@ Redis.prototype.connect = function (callback) {
     }
     this.setStatus('connecting');
 
+    const {options} = this;
+
     this.condition = {
-      select: this.options.db,
-      auth: this.options.password,
+      select: options.db,
+      auth: options.password,
       subscriber: false
     };
 
@@ -268,11 +274,11 @@ Redis.prototype.connect = function (callback) {
         _this.setStatus('end');
         return;
       }
-      var CONNECT_EVENT = _this.options.tls ? 'secureConnect' : 'connect';
+      var CONNECT_EVENT = options.tls ? 'secureConnect' : 'connect';
 
       _this.stream = stream;
-      if (typeof _this.options.keepAlive === 'number') {
-        stream.setKeepAlive(true, _this.options.keepAlive);
+      if (typeof options.keepAlive === 'number') {
+        stream.setKeepAlive(true, options.keepAlive);
       }
 
       stream.once(CONNECT_EVENT, eventHandler.connectHandler(_this));
@@ -280,8 +286,8 @@ Redis.prototype.connect = function (callback) {
       stream.once('close', eventHandler.closeHandler(_this));
       stream.on('data', eventHandler.dataHandler(_this));
 
-      if (_this.options.connectTimeout) {
-        stream.setTimeout(_this.options.connectTimeout, function () {
+      if (options.connectTimeout) {
+        stream.setTimeout(options.connectTimeout, function () {
           stream.setTimeout(0);
           stream.destroy();
 
@@ -296,19 +302,19 @@ Redis.prototype.connect = function (callback) {
         });
       }
 
-      if (_this.options.noDelay) {
+      if (options.noDelay) {
         stream.setNoDelay(true);
       }
 
-      var connectionConnectHandler = function () {
+      var connectionReadyHandler = function () {
         _this.removeListener('close', connectionCloseHandler);
         resolve();
       };
       var connectionCloseHandler = function () {
-        _this.removeListener(CONNECT_EVENT, connectionConnectHandler);
+        _this.removeListener('ready', connectionReadyHandler);
         reject(new Error(utils.CONNECTION_CLOSED_ERROR_MSG));
       };
-      _this.once(CONNECT_EVENT, connectionConnectHandler);
+      _this.once('ready', connectionReadyHandler);
       _this.once('close', connectionCloseHandler);
     }, function (type, err) {
       _this.silentEmit(type, err);


Test patch:
diff --git a/test/functional/cluster/connect.js b/test/functional/cluster/connect.js
index a2368e6..f20da4e 100644
--- a/test/functional/cluster/connect.js
+++ b/test/functional/cluster/connect.js
@@ -75,6 +75,34 @@ describe('cluster:connect', function () {
     });
   });
 
+  it('should wait for ready state before resolving', function (done) {
+    var slotTable = [
+      [0, 16383, ['127.0.0.1', 30001]]
+    ];
+    var argvHandler = function (argv) {
+      if (argv[0] === 'info') {
+        // return 'role:master'
+      }
+      if (argv[0] === 'cluster' && argv[1] === 'slots') {
+        return slotTable;
+      }
+      if (argv[0] === 'cluster' && argv[1] === 'info') {
+        return 'cluster_state:ok';
+      }
+    };
+    var node = new MockServer(30001, argvHandler);
+
+    var cluster = new Redis.Cluster([
+      { host: '127.0.0.1', port: '30001' }
+    ], { lazyConnect: true });
+
+    cluster.connect().then(function () {
+      expect(cluster.status).to.eql('ready');
+      cluster.disconnect();
+      disconnect([node], done);
+    });
+  });
+
   it('should support url schema', function (done) {
     var node = new MockServer(30001);
 
@@ -249,6 +277,7 @@ describe('cluster:connect', function () {
       expect(err.message).to.eql(errorMessage);
       checkDone();
     });
+
     function checkDone() {
       if (!--pending) {
         cluster.disconnect();
diff --git a/test/functional/connection.js b/test/functional/connection.js
index 9301d77..fb033d0 100644
--- a/test/functional/connection.js
+++ b/test/functional/connection.js
@@ -96,7 +96,9 @@ describe('connection', function () {
 
     it('should stop reconnecting when disconnected', function (done) {
       var redis = new Redis(8999, {
-        retryStrategy: function () { return 0; }
+        retryStrategy: function () {
+          return 0;
+        }
       });
 
       redis.on('close', function () {
@@ -116,6 +118,42 @@ describe('connection', function () {
         done();
       });
     });
+
+    it('should resolve when the status become ready', function (done) {
+      var redis = new Redis({ lazyConnect: true });
+      redis.connect().then(function () {
+        expect(redis.status).to.eql('ready');
+        done();
+      });
+    });
+
+    it('should reject when closed (reconnecting)', function (done) {
+      var redis = new Redis({
+        port: 8989,
+        lazyConnect: true,
+        retryStrategy: function () {
+          return 0;
+        }
+      });
+
+      redis.connect().catch(function () {
+        expect(redis.status).to.eql('reconnecting');
+        done();
+      });
+    });
+
+    it('should reject when closed (end)', function (done) {
+      var redis = new Redis({
+        port: 8989,
+        lazyConnect: true,
+        retryStrategy: false
+      });
+
+      redis.connect().catch(function () {
+        expect(redis.status).to.eql('end');
+        done();
+      });
+    });
   });
 
   describe('retryStrategy', function () {