From 6e23e0cf7559d4fbed7cd5724ef9adffac191097 Mon Sep 17 00:00:00 2001 From: Zihua Li Date: Wed, 2 Feb 2022 18:29:04 +0800 Subject: [PATCH] Make sure timers is cleared on exit --- lib/cluster/index.ts | 40 ++++++++++--------- lib/redis/event_handler.ts | 5 +++ lib/redis/index.ts | 14 ++----- test/functional/cluster/disconnection.ts | 51 ++++++++++++++++++++++++ test/functional/disconnection.ts | 46 +++++++++++++++++++++ 5 files changed, 127 insertions(+), 29 deletions(-) create mode 100644 test/functional/cluster/disconnection.ts create mode 100644 test/functional/disconnection.ts diff --git a/lib/cluster/index.ts b/lib/cluster/index.ts index 58b7c31e8..45f9848fa 100644 --- a/lib/cluster/index.ts +++ b/lib/cluster/index.ts @@ -69,7 +69,7 @@ class Cluster extends EventEmitter { private isRefreshing = false; public isCluster = true; private _autoPipelines: Map = new Map(); - private _groupsIds: {[key: string]: number} = {}; + private _groupsIds: { [key: string]: number } = {}; private _groupsBySlot: number[] = Array(16384); private _runningAutoPipelines: Set = new Set(); private _readyDelayedCallbacks: CallbackFunction[] = []; @@ -190,8 +190,10 @@ class Cluster extends EventEmitter { return; } - // Make sure only one timer is active at a time - clearInterval(this._addedScriptHashesCleanInterval); + if (this._addedScriptHashesCleanInterval) { + // Make sure only one timer is active at a time + clearInterval(this._addedScriptHashesCleanInterval); + } // Start the script cache cleaning this._addedScriptHashesCleanInterval = setInterval(() => { @@ -272,12 +274,12 @@ class Cluster extends EventEmitter { this.once("close", this.handleCloseEvent.bind(this)); this.refreshSlotsCache( - function (err) { - if (err && err.message === "Failed to refresh slots cache.") { - Redis.prototype.silentEmit.call(this, "error", err); - this.connectionPool.reset([]); - } - }.bind(this) + function (err) { + if (err && err.message === "Failed to refresh slots cache.") { + Redis.prototype.silentEmit.call(this, "error", err); + this.connectionPool.reset([]); + } + }.bind(this) ); this.subscriber.start(); }) @@ -300,6 +302,11 @@ class Cluster extends EventEmitter { if (reason) { debug("closed because %s", reason); } + + if (this._addedScriptHashesCleanInterval) { + clearInterval(this._addedScriptHashesCleanInterval); + } + let retryDelay; if ( !this.manuallyClosing && @@ -339,9 +346,6 @@ class Cluster extends EventEmitter { const status = this.status; this.setStatus("disconnecting"); - clearInterval(this._addedScriptHashesCleanInterval); - this._addedScriptHashesCleanInterval = null; - if (!reconnect) { this.manuallyClosing = true; } @@ -372,9 +376,6 @@ class Cluster extends EventEmitter { const status = this.status; this.setStatus("disconnecting"); - clearInterval(this._addedScriptHashesCleanInterval); - this._addedScriptHashesCleanInterval = null; - this.manuallyClosing = true; if (this.reconnectTimeout) { @@ -632,7 +633,8 @@ class Cluster extends EventEmitter { } else { _this.slots[slot] = [key]; } - _this._groupsBySlot[slot] = _this._groupsIds[_this.slots[slot].join(';')]; + _this._groupsBySlot[slot] = + _this._groupsIds[_this.slots[slot].join(";")]; _this.connectionPool.findOrCreate(_this.natMapper(key)); tryConnection(); debug("refreshing slot caches... (triggered by MOVED error)"); @@ -867,14 +869,14 @@ class Cluster extends EventEmitter { } // Assign to each node keys a numeric value to make autopipeline comparison faster. - this._groupsIds = Object.create(null); + this._groupsIds = Object.create(null); let j = 0; for (let i = 0; i < 16384; i++) { - const target = (this.slots[i] || []).join(';'); + const target = (this.slots[i] || []).join(";"); if (!target.length) { this._groupsBySlot[i] = undefined; - continue; + continue; } if (!this._groupsIds[target]) { diff --git a/lib/redis/event_handler.ts b/lib/redis/event_handler.ts index 618d8aa27..ced08c2d6 100644 --- a/lib/redis/event_handler.ts +++ b/lib/redis/event_handler.ts @@ -170,6 +170,11 @@ export function closeHandler(self) { abortTransactionFragments(self.offlineQueue); } + if (self._addedScriptHashesCleanInterval) { + clearInterval(self._addedScriptHashesCleanInterval); + self._addedScriptHashesCleanInterval = null; + } + if (self.manuallyClosing) { self.manuallyClosing = false; debug("skip reconnecting since the connection is manually closed."); diff --git a/lib/redis/index.ts b/lib/redis/index.ts index ac3dd1d3f..24cfc2b98 100644 --- a/lib/redis/index.ts +++ b/lib/redis/index.ts @@ -313,8 +313,10 @@ Redis.prototype.connect = function (callback) { return; } - // Make sure only one timer is active at a time - clearInterval(this._addedScriptHashesCleanInterval); + if (this._addedScriptHashesCleanInterval) { + // Make sure only one timer is active at a time + clearInterval(this._addedScriptHashesCleanInterval); + } // Start the script cache cleaning this._addedScriptHashesCleanInterval = setInterval(() => { @@ -436,9 +438,6 @@ Redis.prototype.connect = function (callback) { * @public */ Redis.prototype.disconnect = function (reconnect) { - clearInterval(this._addedScriptHashesCleanInterval); - this._addedScriptHashesCleanInterval = null; - if (!reconnect) { this.manuallyClosing = true; } @@ -741,11 +740,6 @@ Redis.prototype.sendCommand = function (command: Command, stream: NetStream) { command.setTimeout(this.options.commandTimeout); } - if (command.name === "quit") { - clearInterval(this._addedScriptHashesCleanInterval); - this._addedScriptHashesCleanInterval = null; - } - let writable = this.status === "ready" || (!stream && diff --git a/test/functional/cluster/disconnection.ts b/test/functional/cluster/disconnection.ts new file mode 100644 index 000000000..5ae47abca --- /dev/null +++ b/test/functional/cluster/disconnection.ts @@ -0,0 +1,51 @@ +import Redis from "../../../lib/redis"; +import * as sinon from "sinon"; +import { expect } from "chai"; +import { Cluster } from "../../../lib"; +import MockServer from "../../helpers/mock_server"; + +describe("disconnection", function () { + afterEach(() => { + sinon.restore(); + }); + + it("should clear all timers on disconnect", function (done) { + const server = new MockServer(30000); + + const setIntervalCalls = sinon.spy(global, "setInterval"); + const clearIntervalCalls = sinon.spy(global, "clearInterval"); + + const cluster = new Cluster([{ host: "127.0.0.1", port: "30000" }]); + cluster.on("connect", function () { + cluster.disconnect(); + }); + + cluster.on("end", function () { + setTimeout(() => { + // wait for disconnect with refresher. + expect(setIntervalCalls.callCount).to.equal( + clearIntervalCalls.callCount + ); + server.disconnect(); + done(); + }, 500); + }); + }); + + it("should clear all timers on server exits", function (done) { + const server = new MockServer(30000); + + const setIntervalCalls = sinon.spy(global, "setInterval"); + const clearIntervalCalls = sinon.spy(global, "clearInterval"); + + const cluster = new Cluster([{ host: "127.0.0.1", port: "30000" }], { + clusterRetryStrategy: null, + }); + cluster.on("end", function () { + expect(setIntervalCalls.callCount).to.equal(clearIntervalCalls.callCount); + done(); + }); + + server.disconnect(); + }); +}); diff --git a/test/functional/disconnection.ts b/test/functional/disconnection.ts new file mode 100644 index 000000000..65905d068 --- /dev/null +++ b/test/functional/disconnection.ts @@ -0,0 +1,46 @@ +import Redis from "../../lib/redis"; +import * as sinon from "sinon"; +import { expect } from "chai"; +import MockServer from "../helpers/mock_server"; + +describe("disconnection", function () { + afterEach(() => { + sinon.restore(); + }); + + it("should clear all timers on disconnect", function (done) { + const server = new MockServer(30000); + + const setIntervalCalls = sinon.spy(global, "setInterval"); + const clearIntervalCalls = sinon.spy(global, "clearInterval"); + + const redis = new Redis({}); + redis.on("connect", function () { + redis.disconnect(); + }); + + redis.on("end", function () { + expect(setIntervalCalls.callCount).to.equal(clearIntervalCalls.callCount); + server.disconnect(); + done(); + }); + }); + + it("should clear all timers on server exits", function (done) { + const server = new MockServer(30000); + + const setIntervalCalls = sinon.spy(global, "setInterval"); + const clearIntervalCalls = sinon.spy(global, "clearInterval"); + + const redis = new Redis({ + port: 30000, + retryStrategy: null, + }); + redis.on("end", function () { + expect(setIntervalCalls.callCount).to.equal(clearIntervalCalls.callCount); + done(); + }); + + server.disconnect(); + }); +});