-
Notifications
You must be signed in to change notification settings - Fork 30.2k
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
MacroTask and MicroTask execution order #22257
Comments
@nodejs/timers |
node doesn't use the language microtask queue for timers, it has another queue above that which runs the {timer queue, immediate queue, etc} and the microtask queue. |
@devsnek I do think that this is a bug that should be fixed, to be clear. |
@addaleax agreed 😄 |
For reference, here's how HTML defines the event loop for renderer threads and worker threads: https://html.spec.whatwg.org/multipage/webappapis.html#event-loop-processing-model. Definition wise, a "task" in HTML parlance is a "macrotask" unless specified otherwise. A quick summary:
The way
In comparison, the way Node.js timers run is this:
(The "thread" here is idealized; libuv may or may not use threads to implement timers.) This results in all timers of a specific timeout being run together rather than having microtasks run after each timer callback. BTW, I realize that people who have commented here already probably knows how everything works already, but in case a summary is needed by anyone for context. |
Before we call this a bug in Node.js, there really needs to be a clear description of what the expected behavior is. Timers in Node.js have always been different than timers in the browser and JS does not define their behavior. /cc @Fishrock123 |
I think we should be very careful in what timing guarantees we make about this sort of thing because every guarantee we make means we have to keep it rather than optimize if we get the chance in the future. That said - in this particular case I believe we should align in browsers and always run microticks before timers if at all possible. This has implications on hooks people use for debugging and instrumentation and we should do it both because it aligns with browsers and because it’s right. |
It seems like there are some misconceptions here
See below
As far as I'm aware neither one works like this. IIRC the MTQ works roughly the same as our nextTick queue except I think it can be interrupted by browser frame renders (and then maybe RAF, not 100% sure on that). The proper behavior, at least in node, should be to run the MTQ, then nextTicks, after "sync user code" is finished running. As with all things adjusting the behvaior and order of timers, this is probably a moderately risky change. It is possible the manually run the MicroTaskQueue in between timer queue items (again, the timer system does not actually matter). nextTick already has this exact behavior. That being said, it seems like the root cause of this is that nextTicks are not called between timer invocations. I can't think of a good reason why that is the case other than that it has always been the case (literally). I think if we make any change, we should call the nextTick queue, which would then call the MTQ. That should keep ordering consistent and the way we want. Immediates also have this issue. I can make a patch pretty easily, but testing it's impact is likely quite difficult. |
It'd be great if you could. I see it as a net plus if we could get this working without performance regressions. |
Interestingly, calling the MTQ for individual timer callbacks has no effect on the Node.js test suite, but doing it from immediates seemingly causes all sorts of explosions. |
Ok well it actually gets completely stuck. [10:16|% 99|+ 2353|- 10]: release test-vm-ownpropertysymbols
|
@Fishrock123 I can reproduce it locally (Linux). |
@Fishrock123 i can also reproduce that (macos) |
Definitely possible since the timers could get set on a different ms and thus expire 1ms apart. Anyway, interleaving nextTick is quite easy but it will have impact on the ecosystem as there are a non-trivial number of packages that rely on the current behavior. |
Also worth mentioning that it has non-trivial impact on peformance of timers and immediates. |
Can you name one on top of your head? Should we perhaps make a PR and get a CITGM run for breakage? |
PR's up at #22316... idk how to stop the automated tests though... they are just gona stall forever. Edit: started a citgm run: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1506/ |
Ok so the current state of implementation is at #22842 - it works, but it is pretty costly (performance-wise) to do. (See that PR for more details & profiles.) |
Also, if you're using setTimeout as a form of immediate as the OP is, browsers and node are working on shipping out a method called |
No, that is not a correct solution to the actual problem, Gus. And besides it won't run in between anyways. |
@devsnek to add to what Jeremiah said - would you mind explaining how that solves the timing issue? |
@benjamingr maybe i just misunderstood the intention of the example the OP gave. |
@devsnek the way I understand it (which might be off) is that this issue is about whether or not we want to make guarantees about the order of microtasks and macrotasks (for example "no timers execute between promises and if a promise queues a nextTick that executes before any additional timers"). |
In order to better match the browser behaviour, run nextTicks (and subsequently the microtask queue) after each individual Timer and Immediate, rather than after the whole list is processed. The current behaviour is somewhat of a performance micro-optimization and also partly dictated by how timer handles were implemented. PR-URL: #22842 Fixes: #22257 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
Hi,
I believe the topic is not new (see #17181, #15081) but nevertheless worth a new issue.
Currently there is a different in the execution order of Macro and Micro tasks between browsers and NodeJS that leads to different behaviors on shared code across both platforms.
In the past browsers had inconsistent behavior but currently all major browsers - Chrome (v68), Edge (v42), Safari (v11.1) and Firefox (v61) - behave the same.
Example:
Results:
Chrome / Edge / Safari / Firefox
Node
The result is actually inconsistent. From my tests, the most frequent result is:
But sometimes it matches the one given by the browsers.
The reason why I bumped into this issue is that I need to hook a callback on the end of every event loop turn. In my case, ensuring that all microtasks are executed before the start of the next macrotask would easily solve the problem, and work everywhere.
I understand the impact of such a change in the engine, and that it could only be done in a next major version. Despite of that, having an alignment with how all browsers behave seems to me at least worth considering the topic one more time.
Thank you for you time in advance,
Best,
jpsfs
PS: Before finding the issue linked above, I created a question on StackOverflow. I will leave it here for future reference.
The text was updated successfully, but these errors were encountered: