From 21138af6d323f0e87ec9775bd8c0baacfaa81165 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=E5=AD=90=E9=AA=85?= <luin@users.noreply.github.com>
Date: Sun, 16 Dec 2018 22:30:14 +0800
Subject: [PATCH] fix(cluster): handle connection errors by reconnection (#762)

Previously, exceptions caused by DNS resolving will not trigger any reconnections and fail silently. That makes errors much harder to be handled. This fix catches these exceptions and triggers a reconnection to keep the same behaviors as v3.

Closes #753
---
 lib/cluster/ClusterOptions.ts        |  2 +-
 lib/cluster/index.ts                 | 13 ++++--
 test/functional/cluster/connect.js   | 19 ++++++--
 test/functional/cluster/dnsLookup.js | 68 ++++++++++++++++++++++++++++
 4 files changed, 95 insertions(+), 7 deletions(-)

diff --git a/lib/cluster/ClusterOptions.ts b/lib/cluster/ClusterOptions.ts
index b3cfb558..a26fe793 100644
--- a/lib/cluster/ClusterOptions.ts
+++ b/lib/cluster/ClusterOptions.ts
@@ -16,7 +16,7 @@ export interface IClusterOptions {
    *
    * @default (times) => Math.min(100 + times * 2, 2000)
    */
-  clusterRetryStrategy?: (times: number) => number | null
+  clusterRetryStrategy?: (times: number, reason?: Error) => number | null
 
   /**
    * See Redis class.
diff --git a/lib/cluster/index.ts b/lib/cluster/index.ts
index bf403c9a..719b95e8 100644
--- a/lib/cluster/index.ts
+++ b/lib/cluster/index.ts
@@ -190,7 +190,11 @@ class Cluster extends EventEmitter {
           }
         }.bind(this))
         this.subscriber.start()
-      }).catch(reject)
+      }).catch((err) => {
+        this.setStatus('close')
+        this.handleCloseEvent(err)
+        reject(err)
+      })
     })
   }
 
@@ -200,10 +204,13 @@ class Cluster extends EventEmitter {
    * @private
    * @memberof Cluster
    */
-  private handleCloseEvent(): void {
+  private handleCloseEvent(reason?: Error): void {
+    if (reason) {
+      debug('closed because %s', reason)
+    }
     let retryDelay
     if (!this.manuallyClosing && typeof this.options.clusterRetryStrategy === 'function') {
-      retryDelay = this.options.clusterRetryStrategy.call(this, ++this.retryAttempts)
+      retryDelay = this.options.clusterRetryStrategy.call(this, ++this.retryAttempts, reason)
     }
     if (typeof retryDelay === 'number') {
       this.setStatus('reconnecting')
diff --git a/test/functional/cluster/connect.js b/test/functional/cluster/connect.js
index 29a8826e..4e7f9c14 100644
--- a/test/functional/cluster/connect.js
+++ b/test/functional/cluster/connect.js
@@ -355,11 +355,24 @@ describe('cluster:connect', function () {
   });
 
   it('throws when startupNodes is empty', (done) => {
-    const cluster = new Redis.Cluster(null, {lazyConnect: true})
+    const message = '`startupNodes` should contain at least one node.'
+    let pending = 2
+    const cluster = new Redis.Cluster(null, {
+      lazyConnect: true,
+      clusterRetryStrategy(_, reason) {
+        expect(reason.message).to.eql(message)
+        if (!--pending) {
+          done()
+        }
+        return false
+      }
+    })
     cluster.connect().catch(err => {
-      expect(err.message).to.eql('`startupNodes` should contain at least one node.')
+      expect(err.message).to.eql(message)
       cluster.disconnect()
-      done()
+      if (!--pending) {
+        done()
+      }
     })
   })
 });
diff --git a/test/functional/cluster/dnsLookup.js b/test/functional/cluster/dnsLookup.js
index da493598..9901de00 100644
--- a/test/functional/cluster/dnsLookup.js
+++ b/test/functional/cluster/dnsLookup.js
@@ -55,4 +55,72 @@ describe('cluster:dnsLookup', () => {
       done()
     })
   })
+
+  it('reconnects when dns lookup fails', (done) => {
+    const slotTable = [
+      [0, 1000, ['127.0.0.1', 30001]],
+      [1001, 16383, ['127.0.0.1', 30002]]
+    ]
+    new MockServer(30001, (argv, c) => {
+    }, slotTable)
+    new MockServer(30002, (argv, c) => {
+    }, slotTable)
+
+    let retried = false
+    const cluster = new Redis.Cluster([
+      { host: 'localhost', port: '30001' }
+    ], {
+      dnsLookup (_, callback) {
+        if (retried) {
+          callback(null, '127.0.0.1')
+        } else {
+          callback(new Error('Random Exception'))
+        }
+      },
+      clusterRetryStrategy: function (_, reason) {
+        expect(reason.message).to.eql('Random Exception')
+        expect(retried).to.eql(false)
+        retried = true
+        return 0;
+      }
+    })
+    cluster.on('ready', () => {
+      cluster.disconnect();
+      done();
+    })
+  })
+
+  it('reconnects when dns lookup thrown an error', (done) => {
+    const slotTable = [
+      [0, 1000, ['127.0.0.1', 30001]],
+      [1001, 16383, ['127.0.0.1', 30002]]
+    ]
+    new MockServer(30001, (argv, c) => {
+    }, slotTable)
+    new MockServer(30002, (argv, c) => {
+    }, slotTable)
+
+    let retried = false
+    const cluster = new Redis.Cluster([
+      { host: 'localhost', port: '30001' }
+    ], {
+      dnsLookup (_, callback) {
+        if (retried) {
+          callback(null, '127.0.0.1')
+        } else {
+          throw new Error('Random Exception')
+        }
+      },
+      clusterRetryStrategy: function (_, reason) {
+        expect(reason.message).to.eql('Random Exception')
+        expect(retried).to.eql(false)
+        retried = true
+        return 0;
+      }
+    })
+    cluster.on('ready', () => {
+      cluster.disconnect();
+      done();
+    })
+  })
 })