Fix for #2248; busy event loop can cause events to fire while they shouldn't #2399
Conversation
paulrutter
changed the title
|
I do see some failed tests, so probably this change is not entirely backwards compatible. |
| request.destroy(new TimeoutError(delay, event)); | ||
| // Use queueMicroTask to allow for any cancelled events to be handled first, | ||
| // to prevent firing any TimeoutError unneeded when the event loop is busy or blocked | ||
| queueMicrotask(() => { |
There was a problem hiding this comment.
I don't think queueMicrotask will work.
Ignoring the fact that the above image isn't a perfect representation of what's happening (in Node, other runtimes may do something even more different), what we need to happen is to go around the outer loop. Since we're in the "timer callbacks" phase when we enter this handler, by queueing up another setTimeout we're going to do one full outer loop. This means we process I/O events (like connections!) and guarantee whether or not the connection truly was established.
queueMicrotask would run immediately after the timer callbacks are processed, but likely before I/O events are processed.
There was a problem hiding this comment.
Good point. I'm happy to change it back though, but we still have a few failing unit tests along with either approach.
There was a problem hiding this comment.
Given we've had this patch running in production for a long time without any issues, I'm inclined to say it's just the tests that need some tweaking 🙂
I took a super quick look, and two things stand out to me:
socket timeoutappears to set a customsetTimeouton the stream, which may not play nicely with this.send timeoutticks a clock. Not sure how that works, but maybe we need two calls to tick to ensure the additional callback is called.
There was a problem hiding this comment.
I also looked into the tests, and the point is that (even though it works) it does break backwards compatibility as before the TimeoutError would be thrown synchronously, and now it no longer is returned directly.
We can tune the tests so that it passes, but not sure if that would be acceptable or that we would need a different approach.
There was a problem hiding this comment.
Ah, fair point :)
Of course @sindresorhus can make the final call, I'd be happy to offer my two cents:
- The original handler was already part of a
setTimeout, and hence "async". There were no guarantees that it would trigger the timeout exactly as specified. - Doing another
setTimeoutdoes mean we extend the time for real connection timeouts a bit, since we have to go through the event loop one more time, but we were okay with that tradeoff (because it was sufficiently rare). - If backwards compatibility is a must, adding configuration is always an option!
- We've been running this patch in production for almost 2 years now without any issues.
There was a problem hiding this comment.
Thanks for your input, appreciated.
I think it's up to @sindresorhus to make a fair judgement on this.
There was a problem hiding this comment.
I don't think anyone should have relied on it being sync. So I'm fine with you just adjusting the tests.
There was a problem hiding this comment.
Ok, i will look into this 👍 thanks
This is a fix for #2248, as supplied by @thegedge earlier. See the original issue for a description of the issue and the proposed solution.
It's hard to add a unit test for this behavior, as it involves blocking the event loop for a certain amount of time.
Let me know if this is something that would be considered or if there are any concerns.
Checklist