Skip to content

Commit cfb04a0

Browse files
luinluin
authoredFeb 2, 2022
fix: make sure timers is cleared on exit (#1502)
·
v5.6.0v1.3.0
1 parent bf3bec7 commit cfb04a0

File tree

7 files changed

+136
-23
lines changed

7 files changed

+136
-23
lines changed
 

‎lib/cluster/index.ts

+25-16
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ class Cluster extends EventEmitter {
6969
private isRefreshing = false;
7070
public isCluster = true;
7171
private _autoPipelines: Map<string, typeof Pipeline> = new Map();
72-
private _groupsIds: {[key: string]: number} = {};
72+
private _groupsIds: { [key: string]: number } = {};
7373
private _groupsBySlot: number[] = Array(16384);
7474
private _runningAutoPipelines: Set<string> = new Set();
7575
private _readyDelayedCallbacks: CallbackFunction[] = [];
@@ -154,6 +154,13 @@ class Cluster extends EventEmitter {
154154
}
155155
}
156156

157+
clearAddedScriptHashesCleanInterval() {
158+
if (this._addedScriptHashesCleanInterval) {
159+
clearInterval(this._addedScriptHashesCleanInterval);
160+
this._addedScriptHashesCleanInterval = null;
161+
}
162+
}
163+
157164
resetNodesRefreshInterval() {
158165
if (this.slotsTimer) {
159166
return;
@@ -191,7 +198,7 @@ class Cluster extends EventEmitter {
191198
}
192199

193200
// Make sure only one timer is active at a time
194-
clearInterval(this._addedScriptHashesCleanInterval);
201+
this.clearAddedScriptHashesCleanInterval();
195202

196203
// Start the script cache cleaning
197204
this._addedScriptHashesCleanInterval = setInterval(() => {
@@ -272,12 +279,12 @@ class Cluster extends EventEmitter {
272279
this.once("close", this.handleCloseEvent.bind(this));
273280

274281
this.refreshSlotsCache(
275-
function (err) {
276-
if (err && err.message === "Failed to refresh slots cache.") {
277-
Redis.prototype.silentEmit.call(this, "error", err);
278-
this.connectionPool.reset([]);
279-
}
280-
}.bind(this)
282+
function (err) {
283+
if (err && err.message === "Failed to refresh slots cache.") {
284+
Redis.prototype.silentEmit.call(this, "error", err);
285+
this.connectionPool.reset([]);
286+
}
287+
}.bind(this)
281288
);
282289
this.subscriber.start();
283290
})
@@ -300,6 +307,9 @@ class Cluster extends EventEmitter {
300307
if (reason) {
301308
debug("closed because %s", reason);
302309
}
310+
311+
this.clearAddedScriptHashesCleanInterval();
312+
303313
let retryDelay;
304314
if (
305315
!this.manuallyClosing &&
@@ -339,8 +349,7 @@ class Cluster extends EventEmitter {
339349
const status = this.status;
340350
this.setStatus("disconnecting");
341351

342-
clearInterval(this._addedScriptHashesCleanInterval);
343-
this._addedScriptHashesCleanInterval = null;
352+
this.clearAddedScriptHashesCleanInterval();
344353

345354
if (!reconnect) {
346355
this.manuallyClosing = true;
@@ -372,8 +381,7 @@ class Cluster extends EventEmitter {
372381
const status = this.status;
373382
this.setStatus("disconnecting");
374383

375-
clearInterval(this._addedScriptHashesCleanInterval);
376-
this._addedScriptHashesCleanInterval = null;
384+
this.clearAddedScriptHashesCleanInterval();
377385

378386
this.manuallyClosing = true;
379387

@@ -632,7 +640,8 @@ class Cluster extends EventEmitter {
632640
} else {
633641
_this.slots[slot] = [key];
634642
}
635-
_this._groupsBySlot[slot] = _this._groupsIds[_this.slots[slot].join(';')];
643+
_this._groupsBySlot[slot] =
644+
_this._groupsIds[_this.slots[slot].join(";")];
636645
_this.connectionPool.findOrCreate(_this.natMapper(key));
637646
tryConnection();
638647
debug("refreshing slot caches... (triggered by MOVED error)");
@@ -867,14 +876,14 @@ class Cluster extends EventEmitter {
867876
}
868877

869878
// Assign to each node keys a numeric value to make autopipeline comparison faster.
870-
this._groupsIds = Object.create(null);
879+
this._groupsIds = Object.create(null);
871880
let j = 0;
872881
for (let i = 0; i < 16384; i++) {
873-
const target = (this.slots[i] || []).join(';');
882+
const target = (this.slots[i] || []).join(";");
874883

875884
if (!target.length) {
876885
this._groupsBySlot[i] = undefined;
877-
continue;
886+
continue;
878887
}
879888

880889
if (!this._groupsIds[target]) {

‎lib/redis/event_handler.ts

+2
Original file line numberDiff line numberDiff line change
@@ -170,6 +170,8 @@ export function closeHandler(self) {
170170
abortTransactionFragments(self.offlineQueue);
171171
}
172172

173+
self.clearAddedScriptHashesCleanInterval();
174+
173175
if (self.manuallyClosing) {
174176
self.manuallyClosing = false;
175177
debug("skip reconnecting since the connection is manually closed.");

‎lib/redis/index.ts

+10-5
Original file line numberDiff line numberDiff line change
@@ -290,6 +290,13 @@ Redis.prototype.setStatus = function (status, arg) {
290290
process.nextTick(this.emit.bind(this, status, arg));
291291
};
292292

293+
Redis.prototype.clearAddedScriptHashesCleanInterval = function () {
294+
if (this._addedScriptHashesCleanInterval) {
295+
clearInterval(this._addedScriptHashesCleanInterval);
296+
this._addedScriptHashesCleanInterval = null;
297+
}
298+
};
299+
293300
/**
294301
* Create a connection to Redis.
295302
* This method will be invoked automatically when creating a new Redis instance
@@ -314,7 +321,7 @@ Redis.prototype.connect = function (callback) {
314321
}
315322

316323
// Make sure only one timer is active at a time
317-
clearInterval(this._addedScriptHashesCleanInterval);
324+
this.clearAddedScriptHashesCleanInterval();
318325

319326
// Start the script cache cleaning
320327
this._addedScriptHashesCleanInterval = setInterval(() => {
@@ -436,8 +443,7 @@ Redis.prototype.connect = function (callback) {
436443
* @public
437444
*/
438445
Redis.prototype.disconnect = function (reconnect) {
439-
clearInterval(this._addedScriptHashesCleanInterval);
440-
this._addedScriptHashesCleanInterval = null;
446+
this.clearAddedScriptHashesCleanInterval();
441447

442448
if (!reconnect) {
443449
this.manuallyClosing = true;
@@ -742,8 +748,7 @@ Redis.prototype.sendCommand = function (command: Command, stream: NetStream) {
742748
}
743749

744750
if (command.name === "quit") {
745-
clearInterval(this._addedScriptHashesCleanInterval);
746-
this._addedScriptHashesCleanInterval = null;
751+
this.clearAddedScriptHashesCleanInterval();
747752
}
748753

749754
let writable =

‎test/functional/autopipelining.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -181,7 +181,7 @@ describe("autoPipelining for single node", function () {
181181
expect(redis.autoPipelineQueueSize).to.eql(1);
182182

183183
redis.set("foo2", (err) => {
184-
expect(err.message).to.eql(
184+
expect(err.message).to.include(
185185
"ERR wrong number of arguments for 'set' command"
186186
);
187187
done();

‎test/functional/cluster/autopipelining.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -355,7 +355,7 @@ describe("autoPipelining for cluster", function () {
355355
expect(cluster.autoPipelineQueueSize).to.eql(1);
356356

357357
cluster.set("foo5", (err) => {
358-
expect(err.message).to.eql(
358+
expect(err.message).to.include(
359359
"ERR wrong number of arguments for 'set' command"
360360
);
361361

‎test/functional/cluster/disconnection.ts

+51
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
import Redis from "../../../lib/redis";
2+
import * as sinon from "sinon";
3+
import { expect } from "chai";
4+
import { Cluster } from "../../../lib";
5+
import MockServer from "../../helpers/mock_server";
6+
7+
describe("disconnection", function () {
8+
afterEach(() => {
9+
sinon.restore();
10+
});
11+
12+
it("should clear all timers on disconnect", function (done) {
13+
const server = new MockServer(30000);
14+
15+
const setIntervalCalls = sinon.spy(global, "setInterval");
16+
const clearIntervalCalls = sinon.spy(global, "clearInterval");
17+
18+
const cluster = new Cluster([{ host: "127.0.0.1", port: "30000" }]);
19+
cluster.on("connect", function () {
20+
cluster.disconnect();
21+
});
22+
23+
cluster.on("end", function () {
24+
setTimeout(() => {
25+
// wait for disconnect with refresher.
26+
expect(setIntervalCalls.callCount).to.equal(
27+
clearIntervalCalls.callCount
28+
);
29+
server.disconnect();
30+
done();
31+
}, 500);
32+
});
33+
});
34+
35+
it("should clear all timers on server exits", function (done) {
36+
const server = new MockServer(30000);
37+
38+
const setIntervalCalls = sinon.spy(global, "setInterval");
39+
const clearIntervalCalls = sinon.spy(global, "clearInterval");
40+
41+
const cluster = new Cluster([{ host: "127.0.0.1", port: "30000" }], {
42+
clusterRetryStrategy: null,
43+
});
44+
cluster.on("end", function () {
45+
expect(setIntervalCalls.callCount).to.equal(clearIntervalCalls.callCount);
46+
done();
47+
});
48+
49+
server.disconnect();
50+
});
51+
});

‎test/functional/disconnection.ts

+46
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
import Redis from "../../lib/redis";
2+
import * as sinon from "sinon";
3+
import { expect } from "chai";
4+
import MockServer from "../helpers/mock_server";
5+
6+
describe("disconnection", function () {
7+
afterEach(() => {
8+
sinon.restore();
9+
});
10+
11+
it("should clear all timers on disconnect", function (done) {
12+
const server = new MockServer(30000);
13+
14+
const setIntervalCalls = sinon.spy(global, "setInterval");
15+
const clearIntervalCalls = sinon.spy(global, "clearInterval");
16+
17+
const redis = new Redis({});
18+
redis.on("connect", function () {
19+
redis.disconnect();
20+
});
21+
22+
redis.on("end", function () {
23+
expect(setIntervalCalls.callCount).to.equal(clearIntervalCalls.callCount);
24+
server.disconnect();
25+
done();
26+
});
27+
});
28+
29+
it("should clear all timers on server exits", function (done) {
30+
const server = new MockServer(30000);
31+
32+
const setIntervalCalls = sinon.spy(global, "setInterval");
33+
const clearIntervalCalls = sinon.spy(global, "clearInterval");
34+
35+
const redis = new Redis({
36+
port: 30000,
37+
retryStrategy: null,
38+
});
39+
redis.on("end", function () {
40+
expect(setIntervalCalls.callCount).to.equal(clearIntervalCalls.callCount);
41+
done();
42+
});
43+
44+
server.disconnect();
45+
});
46+
});

0 commit comments

Comments
 (0)
Please sign in to comment.