Skip to content

Allow functions wrapped in timeout to be called multiple times (fixes #1418)#1419

Merged
aearly merged 1 commit intomasterfrom
timeout-fix
May 22, 2017
Merged

Allow functions wrapped in timeout to be called multiple times (fixes #1418)#1419
aearly merged 1 commit intomasterfrom
timeout-fix

Conversation

@hargasinski
Copy link
Copy Markdown
Collaborator

Previously, the scoping of the timedOut variable in timeout meant that if any call to wrapped function timed out, any subsequent calls would also give a ETIMEDOUT error. This PR fixes that by moving the variables down one scope, so they are reinitialized each call. As a side benefit, it also lets the wrapped function be passed to functions such as async.forEach.

@hargasinski hargasinski self-assigned this May 21, 2017
@hargasinski hargasinski requested review from aearly and megawac May 21, 2017 03:25
Comment thread lib/timeout.js
}

var fn = wrapAsync(asyncFn);
args.push(function () {
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I noticed this style in ensureAsync. I think I prefer it over the args.concat(namedFunction) style. Should I change it back to the way it was before?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is fine. args.concat(namedFunction) is nice because it's a singe expression that returns the resulti, but it also creates a new array. push is fine here because nothing else re-uses the args array.

@aearly
Copy link
Copy Markdown
Collaborator

aearly commented May 22, 2017

Amazing that this wasn't caught until now. It makes me feel like no one was actually using timeout....

@aearly aearly merged commit b4c80c0 into master May 22, 2017
@hargasinski hargasinski deleted the timeout-fix branch May 22, 2017 03:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants