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.

Sign up for GitHub

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

Jump to bottom

clears commandTimeout timer as each respective command gets fulfilled #1336

Merged
merged 9 commits into from
Apr 24, 2021
Merged

clears commandTimeout timer as each respective command gets fulfilled #1336

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
c3f432c
making commandTimeout clean successful timers and work in cluster mode
JosePedroDiasSky Apr 14, 2021
626a981
deleting timer attribution solves an issue with transactions; cluster…
JosePedroDiasSky Apr 14, 2021
a145e57
adding attribute annotation to keep typescript happy.
JosePedroDiasSky Apr 14, 2021
688e388
thought setTimeout returned a number, apparently not
JosePedroDiasSky Apr 14, 2021
adb0d22
this time I ran `npm run build` locally first
JosePedroDiasSky Apr 14, 2021
f73c33e
addressing feedback
JosePedroDiasSky Apr 15, 2021
e7b95b7
Clear timeouts for commands in offline queue
luin Apr 24, 2021
7c187b5
Merge branch 'master' into sky-master
luin Apr 24, 2021
10612c3
Address feedbacks
luin Apr 24, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions lib/command.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,7 @@ export default class Command implements ICommand {
public isCustomCommand = false;
public inTransaction = false;
public pipelineIndex?: number;
private _commandTimeoutTimer?: ReturnType<typeof setTimeout>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be able to just use NodeJS.Timeout as the type

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks ok, but I'm not sure if it's necessary.
Are the timers causing actual measurable performance problems or other issues without this change?

If we start clearing them just in case, we should also consider the cost of increased code complexity (maintenance overhead, wider surface area for possible future bugs etc).
Sometimes an optimization saves a few microseconds of CPU time, but can cost hours of developer time at some point in the future.
Might not be the case here, but just something to consider 🙂

If you have a lingering timeout per command and you're doing several commands I'd say it's relevant and the additional complexity for this single operation is marginal, but it's up to you.

Regarding the type change, I can do as your suggested if you feel it's a better fit than using return type. I had tried Timeout without the namespace and number as well and ended up here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated the type as suggested.


private slot?: number | null;
private keys?: Array<string | Buffer>;
Expand Down Expand Up @@ -342,6 +343,11 @@ export default class Command implements ICommand {
private _convertValue(resolve: Function): (result: any) => void {
return (value) => {
try {
if (this._commandTimeoutTimer) {
clearTimeout(this._commandTimeoutTimer);
delete this._commandTimeoutTimer;
}

resolve(this.transformReply(value));
this.isResolved = true;
} catch (err) {
Expand Down
2 changes: 1 addition & 1 deletion lib/redis/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -711,7 +711,7 @@ Redis.prototype.sendCommand = function (command, stream) {
}

if (typeof this.options.commandTimeout === "number") {
setTimeout(() => {
command._commandTimeoutTimer = setTimeout(() => {
if (!command.isResolved) {
command.reject(new Error("Command timed out"));
}
Expand Down