-
-
Notifications
You must be signed in to change notification settings - Fork 10.8k
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
Decorate resolve and reject to clear timeout in all cases #1040
Decorate resolve and reject to clear timeout in all cases #1040
Conversation
lib/adapters/http.js
Outdated
var timer; | ||
var resolve = function resolve(value) { | ||
clearTimeout(timer); | ||
timer = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know it was in the original code but you don't need to set timer = null
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know it isn't necessary but could you also remove the && !timer
from the condition below? I think it never applies because the timer is only set inside the if
.
Could you also remove the |
@rubennorte waiting on anything else from me at this point? |
@pbarbiero travis isn't working for this specific PR and I'm not sure why. Once I figure it out I'll merge this, but the code looks good to me. |
This has been an issue for me as well. In my case, I am using axios to make requests from a cli to an external server. If that server is down, i.e. there is no response but ECONNREFUSED error instead, the cli will not exit until the timeout is expired. Would be awesome, if this could be merged. |
Any chance this will be released in the near future? I believe it will fix a few timeout issues I've run into, when the request errors. |
Politely pinging @nickuraltsev & @rubennorte for a follow up |
Any news on that? Having the same issue |
2760755
to
48a7902
Compare
@emilyemorehouse I hate to bother you, but any chance you can get us moving forward on this one? |
@pbarbiero no worries! I've got a work deadline tomorrow morning, but I'll make sure it gets merged this week. I'll also get it included in the next release. 👍 |
@pbarbiero Thank you for the fix! |
Changing from manually placed calls to clear the timeout timer, to decorating the resolve/reject promise functions instead.
Fixes #1004, #459
Replaces #487