Repository: luin/ioredis
Pull request #341 (2016-06-28 14:31:28)

Ensure node exists before being redirected via an ASK
If a master node in the Redis cluster does not have any slots, it is not returned by the `cluster slots` command, and so does not have a client in the connectionPool. If slots are migrated to this new master node, the old master node returns an `ASK` error with the new node details. At the moment ioredis looks up the node in the connectionPool but it doesn't exist, so the following error is returned:

```
TypeError: Cannot call method 'asking' of undefined
  at tryConnection (/var/www/document-updater/releases/5543/node_modules/ioredis/lib/cluster/index.js:464:19)
  at Object._this.handleError.ask (/var/www/document-updater/releases/5543/node_modules/ioredis/lib/cluster/index.js:404:11)
  at Cluster.handleError (/var/www/document-updater/releases/5543/node_modules/ioredis/lib/cluster/index.js:503:52)
  at Command.command.reject (/var/www/document-updater/releases/5543/node_modules/ioredis/lib/cluster/index.js:389:13)
  at Redis.exports.returnError (/var/www/document-updater/releases/5543/node_modules/ioredis/lib/redis/parser.js:75:18)
```

It looks like a similar issues with a `MOVED` reply was fixed in https://github.com/luin/ioredis/commit/0dcb768e2e418b9dd3fa7feaf01ad85cb01521ef. This patch applies the same fix to the `ASK` error. 


Patch:
diff --git a/API.md b/API.md
index de42c18..3f6dd24 100644
--- a/API.md
+++ b/API.md
@@ -153,14 +153,14 @@ the current connection.
 var redis = new Redis();
 redis.monitor(function (err, monitor) {
   // Entering monitoring mode.
-  monitor.on('monitor', function (time, args) {
+  monitor.on('monitor', function (time, args, source, database) {
     console.log(time + ": " + util.inspect(args));
   });
 });
 
 // supports promise as well as other commands
 redis.monitor().then(function (monitor) {
-  monitor.on('monitor', function (time, args) {
+  monitor.on('monitor', function (time, args, source, database) {
     console.log(time + ": " + util.inspect(args));
   });
 });
@@ -218,6 +218,7 @@ Create a Redis instance
     * [new Cluster(startupNodes, options)](#new_Cluster_new)
     * [.connect()](#Cluster+connect) ⇒ <code>Promise</code>
     * [.disconnect()](#Cluster+disconnect)
+    * [.quit(callback)](#Cluster+quit) ⇒ <code>Promise</code>
     * [.nodes([role])](#Cluster+nodes) ⇒ <code>[Array.&lt;Redis&gt;](#Redis)</code>
     * [.getBuiltinCommands()](#Commander+getBuiltinCommands) ⇒ <code>Array.&lt;string&gt;</code>
     * [.createBuiltinCommand(commandName)](#Commander+createBuiltinCommand) ⇒ <code>object</code>
@@ -258,6 +259,19 @@ Disconnect from every node in the cluster.
 
 **Kind**: instance method of <code>[Cluster](#Cluster)</code>  
 **Access:** public  
+<a name="Cluster+quit"></a>
+
+### cluster.quit(callback) ⇒ <code>Promise</code>
+Quit the cluster gracefully.
+
+**Kind**: instance method of <code>[Cluster](#Cluster)</code>  
+**Returns**: <code>Promise</code> - return 'OK' if successfully  
+**Access:** public  
+
+| Param | Type |
+| --- | --- |
+| callback | <code>function</code> | 
+
 <a name="Cluster+nodes"></a>
 
 ### cluster.nodes([role]) ⇒ <code>[Array.&lt;Redis&gt;](#Redis)</code>
diff --git a/lib/cluster/index.js b/lib/cluster/index.js
index e1bd887..9747c80 100644
--- a/lib/cluster/index.js
+++ b/lib/cluster/index.js
@@ -208,6 +208,29 @@ Cluster.prototype.disconnect = function (reconnect) {
   this.connectionPool.reset([]);
 };
 
+/**
+ * Quit the cluster gracefully.
+ *
+ * @param {function} callback
+ * @return {Promise} return 'OK' if successfully
+ * @public
+ */
+Cluster.prototype.quit = function (callback) {
+  this.setStatus('disconnecting');
+
+  this.manuallyClosing = true;
+
+  if (this.reconnectTimeout) {
+    clearTimeout(this.reconnectTimeout);
+    this.reconnectTimeout = null;
+  }
+  return Promise.all(this.nodes().map(function (node) {
+    return node.quit();
+  })).then(function () {
+    return 'OK';
+  }).nodeify(callback);
+};
+
 /**
  * Get nodes with the specified role
  *
@@ -401,6 +424,8 @@ Cluster.prototype.sendCommand = function (command, stream, node) {
         },
         ask: function (slot, key) {
           debug('command %s is required to ask %s:%s', command.name, key);
+          var splitKey = key.split(':');
+          _this.connectionPool.findOrCreate({ host: splitKey[0], port: Number(splitKey[1]) });
           tryConnection(false, key);
         },
         tryagain: partialTry,


Test patch:
diff --git a/test/functional/cluster.js b/test/functional/cluster.js
index 1cb353a..145777b 100644
--- a/test/functional/cluster.js
+++ b/test/functional/cluster.js
@@ -452,6 +452,38 @@ describe('cluster', function () {
         cluster.get('foo');
       });
     });
+
+    it('should be able to redirect a command to a unknown node', function (done) {
+      var asked = false;
+      var slotTable = [
+        [0, 16383, ['127.0.0.1', 30002]]
+      ];
+      var node1 = new MockServer(30001, function (argv) {
+        if (argv[0] === 'get' && argv[1] === 'foo') {
+          expect(asked).to.eql(true);
+          return "bar"
+        } else if (argv[0] === 'asking') {
+          asked = true;
+        }
+      });
+      var node2 = new MockServer(30002, function (argv) {
+        if (argv[0] === 'cluster' && argv[1] === 'slots') {
+          return slotTable;
+        }
+        if (argv[0] === 'get' && argv[1] === 'foo') {
+          return new Error('ASK ' + calculateSlot('foo') + ' 127.0.0.1:30001');
+        }
+      });
+
+      var cluster = new Redis.Cluster([
+        { host: '127.0.0.1', port: '30002' }
+      ]);
+      cluster.get('foo', function (err, res) {
+        expect(res).to.eql('bar');
+        cluster.disconnect();
+        disconnect([node1, node2], done);
+      });
+    });
   });
 
   describe('TRYAGAIN', function () {
@@ -1284,6 +1316,40 @@ describe('cluster', function () {
       });
     });
   });
+
+  describe('#quit()', function () {
+    it('should quit the connection gracefully', function (done) {
+      var slotTable = [
+        [0, 1, ['127.0.0.1', 30001]],
+        [2, 16383, ['127.0.0.1', 30002], ['127.0.0.1', 30003]]
+      ];
+      var argvHandler = function (argv) {
+        if (argv[0] === 'cluster' && argv[1] === 'slots') {
+          return slotTable;
+        }
+      };
+      var node1 = new MockServer(30001, argvHandler);
+      var node2 = new MockServer(30002, argvHandler);
+      var node3 = new MockServer(30003, argvHandler);
+
+      var cluster = new Redis.Cluster([
+        { host: '127.0.0.1', port: '30001' }
+      ]);
+
+      var setCommandHandled = false;
+      cluster.on('ready', function () {
+        cluster.set('foo', 'bar', function () {
+          setCommandHandled = true;
+        });
+        cluster.quit(function (err, state) {
+          expect(setCommandHandled).to.eql(true);
+          expect(state).to.eql('OK');
+          cluster.disconnect();
+          disconnect([node1, node2, node3], done);
+        });
+      });
+    });
+  });
 });
 
 function disconnect(clients, callback) {