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

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

Merged
merged 1 commit into from
May 22, 2017

Conversation

hargasinski
Copy link
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

var fn = wrapAsync(asyncFn);
args.push(function () {
Copy link
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
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
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