Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

clears commandTimeout timer as each respective command gets fulfilled #1336

Merged
merged 9 commits into from
Apr 24, 2021
19 changes: 17 additions & 2 deletions lib/command.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down Expand Up @@ -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) {
Expand Down
8 changes: 2 additions & 6 deletions lib/redis/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand All @@ -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") {
Expand Down
2 changes: 1 addition & 1 deletion lib/utils/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
12 changes: 12 additions & 0 deletions test/functional/commandTimeout.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that #1328 is merged, you can use await server.disconnectPromise() here

});
});