Skip to content

Commit

Permalink
fix(cluster): handle connection errors by reconnection (#762)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
luin authored Dec 16, 2018
1 parent 77231b5 commit 21138af
Show file tree
Hide file tree
Showing 4 changed files with 95 additions and 7 deletions.
2 changes: 1 addition & 1 deletion lib/cluster/ClusterOptions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
13 changes: 10 additions & 3 deletions lib/cluster/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)
})
})
}

Expand All @@ -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')
Expand Down
19 changes: 16 additions & 3 deletions test/functional/cluster/connect.js
Original file line number Diff line number Diff line change
Expand Up @@ -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()
}
})
})
});
68 changes: 68 additions & 0 deletions test/functional/cluster/dnsLookup.js
Original file line number Diff line number Diff line change
Expand Up @@ -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();
})
})
})

0 comments on commit 21138af

Please sign in to comment.