From c3f432cdde25c521751071246266fdbfd1ed8e91 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jose=CC=81=20Pedro=20Dias?= Date: Wed, 14 Apr 2021 14:48:58 +0100 Subject: [PATCH 1/8] making commandTimeout clean successful timers and work in cluster mode --- lib/command.ts | 4 ++++ lib/redis/index.ts | 4 ++-- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/lib/command.ts b/lib/command.ts index 40159213..2dc146af 100644 --- a/lib/command.ts +++ b/lib/command.ts @@ -342,6 +342,10 @@ export default class Command implements ICommand { private _convertValue(resolve: Function): (result: any) => void { return (value) => { try { + if (this._commandTimeoutTimer) { + clearTimeout(this._commandTimeoutTimer); + } + resolve(this.transformReply(value)); this.isResolved = true; } catch (err) { diff --git a/lib/redis/index.ts b/lib/redis/index.ts index 57459990..e4bbb8a3 100644 --- a/lib/redis/index.ts +++ b/lib/redis/index.ts @@ -710,8 +710,8 @@ Redis.prototype.sendCommand = function (command, stream) { return command.promise; } - if (typeof this.options.commandTimeout === "number") { - setTimeout(() => { + if (typeof this.options.commandTimeout === "number" && command.name !== 'cluster') { + command._commandTimeoutTimer = setTimeout(() => { if (!command.isResolved) { command.reject(new Error("Command timed out")); } From 626a98103ab501b9a66f4fee5c1134328685119c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jose=CC=81=20Pedro=20Dias?= Date: Wed, 14 Apr 2021 14:56:00 +0100 Subject: [PATCH 2/8] deleting timer attribution solves an issue with transactions; cluster command doesn't need any special behaviour after all --- lib/command.ts | 1 + lib/redis/index.ts | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/command.ts b/lib/command.ts index 2dc146af..1e3f08ba 100644 --- a/lib/command.ts +++ b/lib/command.ts @@ -344,6 +344,7 @@ export default class Command implements ICommand { try { if (this._commandTimeoutTimer) { clearTimeout(this._commandTimeoutTimer); + delete this._commandTimeoutTimer; } resolve(this.transformReply(value)); diff --git a/lib/redis/index.ts b/lib/redis/index.ts index e4bbb8a3..0a0c8918 100644 --- a/lib/redis/index.ts +++ b/lib/redis/index.ts @@ -710,7 +710,7 @@ Redis.prototype.sendCommand = function (command, stream) { return command.promise; } - if (typeof this.options.commandTimeout === "number" && command.name !== 'cluster') { + if (typeof this.options.commandTimeout === "number") { command._commandTimeoutTimer = setTimeout(() => { if (!command.isResolved) { command.reject(new Error("Command timed out")); From a145e57825cd177aba94c81ecd4852adbc83378f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jose=CC=81=20Pedro=20Dias?= Date: Wed, 14 Apr 2021 15:49:48 +0100 Subject: [PATCH 3/8] adding attribute annotation to keep typescript happy. --- lib/command.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/command.ts b/lib/command.ts index 1e3f08ba..bec97216 100644 --- a/lib/command.ts +++ b/lib/command.ts @@ -156,6 +156,7 @@ export default class Command implements ICommand { public isCustomCommand = false; public inTransaction = false; public pipelineIndex?: number; + private _commandTimeoutTimer?: number; private slot?: number | null; private keys?: Array; From 688e3883e18acbcaeba2aa2906b53a6ce1c198c9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jose=CC=81=20Pedro=20Dias?= Date: Wed, 14 Apr 2021 15:57:53 +0100 Subject: [PATCH 4/8] thought setTimeout returned a number, apparently not --- lib/command.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/command.ts b/lib/command.ts index bec97216..14c923db 100644 --- a/lib/command.ts +++ b/lib/command.ts @@ -156,7 +156,7 @@ export default class Command implements ICommand { public isCustomCommand = false; public inTransaction = false; public pipelineIndex?: number; - private _commandTimeoutTimer?: number; + private _commandTimeoutTimer?: Timeout; private slot?: number | null; private keys?: Array; From adb0d22403403b055f6051eb37ebd73a8229d973 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jose=CC=81=20Pedro=20Dias?= Date: Wed, 14 Apr 2021 16:10:16 +0100 Subject: [PATCH 5/8] this time I ran `npm run build` locally first --- lib/command.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/command.ts b/lib/command.ts index 14c923db..ae2f50ba 100644 --- a/lib/command.ts +++ b/lib/command.ts @@ -156,7 +156,7 @@ export default class Command implements ICommand { public isCustomCommand = false; public inTransaction = false; public pipelineIndex?: number; - private _commandTimeoutTimer?: Timeout; + private _commandTimeoutTimer?: ReturnType; private slot?: number | null; private keys?: Array; From f73c33ee45140ac18414014a3c1b5db0d40bf98d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jose=CC=81=20Pedro=20Dias?= Date: Thu, 15 Apr 2021 09:27:22 +0100 Subject: [PATCH 6/8] addressing feedback --- lib/command.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/command.ts b/lib/command.ts index ae2f50ba..07a15766 100644 --- a/lib/command.ts +++ b/lib/command.ts @@ -156,7 +156,7 @@ export default class Command implements ICommand { public isCustomCommand = false; public inTransaction = false; public pipelineIndex?: number; - private _commandTimeoutTimer?: ReturnType; + private _commandTimeoutTimer?: NodeJS.Timeout; private slot?: number | null; private keys?: Array; From e7b95b71a67c7c64d1564d41dd1d4c7f96a1f155 Mon Sep 17 00:00:00 2001 From: luin Date: Sat, 24 Apr 2021 18:23:33 +0800 Subject: [PATCH 7/8] Clear timeouts for commands in offline queue When client is not ready, Redis#sendCommand() will move commands to the offline queue, and once ready, all commands in the offline queue will be sent with Redis#sendCommand() again. So we should check whether the timer has been set. --- lib/command.ts | 19 +++++++++++++++++-- lib/redis/index.ts | 8 ++------ lib/utils/index.ts | 2 +- test/functional/commandTimeout.ts | 12 ++++++++++++ 4 files changed, 32 insertions(+), 9 deletions(-) diff --git a/lib/command.ts b/lib/command.ts index 07a15766..8b0c59ab 100644 --- a/lib/command.ts +++ b/lib/command.ts @@ -343,8 +343,9 @@ export default class Command implements ICommand { private _convertValue(resolve: Function): (result: any) => void { return (value) => { try { - if (this._commandTimeoutTimer) { - clearTimeout(this._commandTimeoutTimer); + const existingTimer = this._commandTimeoutTimer; + if (existingTimer) { + clearTimeout(existingTimer); delete this._commandTimeoutTimer; } @@ -376,6 +377,20 @@ export default class Command implements ICommand { return result; } + + /** + * Set the wait time before terminating the attempt to execute a command + * and generating an error. + */ + public setTimeout(ms: number) { + if (!this._commandTimeoutTimer) { + this._commandTimeoutTimer = setTimeout(() => { + if (!this.isResolved) { + this.reject(new Error("Command timed out")); + } + }, ms); + } + } } const msetArgumentTransformer = function (args) { diff --git a/lib/redis/index.ts b/lib/redis/index.ts index 0a0c8918..a2ff445d 100644 --- a/lib/redis/index.ts +++ b/lib/redis/index.ts @@ -690,7 +690,7 @@ addTransactionSupport(Redis.prototype); * ``` * @private */ -Redis.prototype.sendCommand = function (command, stream) { +Redis.prototype.sendCommand = function (command: Command, stream) { if (this.status === "wait") { this.connect().catch(noop); } @@ -711,11 +711,7 @@ Redis.prototype.sendCommand = function (command, stream) { } if (typeof this.options.commandTimeout === "number") { - command._commandTimeoutTimer = setTimeout(() => { - if (!command.isResolved) { - command.reject(new Error("Command timed out")); - } - }, this.options.commandTimeout); + command.setTimeout(this.options.commandTimeout); } if (command.name === "quit") { diff --git a/lib/utils/index.ts b/lib/utils/index.ts index d9533fcd..6ba975f5 100644 --- a/lib/utils/index.ts +++ b/lib/utils/index.ts @@ -111,7 +111,7 @@ export function wrapMultiResult(arr) { * ``` * @private */ -export function isInt(value) { +export function isInt(value): value is string { const x = parseFloat(value); return !isNaN(value) && (x | 0) === x; } diff --git a/test/functional/commandTimeout.ts b/test/functional/commandTimeout.ts index 1b0a7fec..3fa0de71 100644 --- a/test/functional/commandTimeout.ts +++ b/test/functional/commandTimeout.ts @@ -22,4 +22,16 @@ describe("commandTimeout", function () { }); clock.tick(1000); }); + + it("does not leak timers for commands in offline queue", async function () { + const server = new MockServer(30001); + + const redis = new Redis({ port: 30001, commandTimeout: 1000 }); + const clock = sinon.useFakeTimers(); + await redis.hget("foo"); + expect(clock.countTimers()).to.eql(0); + clock.restore(); + redis.disconnect(); + await new Promise((resolve) => server.disconnect(() => resolve(null))); + }); }); From 10612c397a149fbe18879f44c8d03ae4b0ca0a94 Mon Sep 17 00:00:00 2001 From: luin Date: Sat, 24 Apr 2021 20:31:45 +0800 Subject: [PATCH 8/8] Address feedbacks --- test/functional/commandTimeout.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/functional/commandTimeout.ts b/test/functional/commandTimeout.ts index 3fa0de71..6258bbbe 100644 --- a/test/functional/commandTimeout.ts +++ b/test/functional/commandTimeout.ts @@ -32,6 +32,6 @@ describe("commandTimeout", function () { expect(clock.countTimers()).to.eql(0); clock.restore(); redis.disconnect(); - await new Promise((resolve) => server.disconnect(() => resolve(null))); + await server.disconnectPromise(); }); });