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

MacroTask and MicroTask execution order #22257

Closed
jpsfs opened this issue Aug 10, 2018 · 23 comments
Closed

MacroTask and MicroTask execution order #22257

jpsfs opened this issue Aug 10, 2018 · 23 comments
Labels
timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.

Comments

@jpsfs
Copy link

jpsfs commented Aug 10, 2018

  • Version: 10.8.0
  • Platform: macOs High Sierra 10.13.6 / Windows 10
  • Subsystem:

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:

for (let i = 0; i < 2; i++) {
	setTimeout(() => {
		console.log("Timeout ", i);
		Promise.resolve().then(() => {
			console.log("Promise 1 ", i);
		}).then(() => {
			console.log("Promise 2 ", i);
		});
	})
}

Results:

Chrome / Edge / Safari / Firefox

Timeout  0
Promise 1  0
Promise 2  0
Timeout  1
Promise 1  1
Promise 2  1

Node

The result is actually inconsistent. From my tests, the most frequent result is:

Timeout  0
Timeout  1
Promise 1  0
Promise 2  0
Promise 1  1
Promise 2  1

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.

@addaleax addaleax added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Aug 10, 2018
@addaleax
Copy link
Member

@nodejs/timers

@devsnek
Copy link
Member

devsnek commented Aug 10, 2018

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.

@addaleax
Copy link
Member

@devsnek I do think that this is a bug that should be fixed, to be clear.

@devsnek
Copy link
Member

devsnek commented Aug 11, 2018

@addaleax agreed 😄

@TimothyGu
Copy link
Member

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:

  1. Dequeue a task from one of the task queues. This allows the browser to prioritize different types of tasks. For example, a requestAnimationFrame task put into their own task queue might be higher priority than a timer task when appropriate, while a requestIdleCallback task might be lower priority.
  2. Run that task.
  3. While the microtask queue is not empty, dequeue and run the earliest-queued microtask. (HTML calls this perform a microtask checkpoint.)
  4. If this is a renderer thread, update the page rendering.
  5. If this is a worker thread, and there are no more tasks left in any task queues, exit the worker.

The way setTimeout() works is roughly:

  1. In another thread, run these steps:
    1. Wait for some time, usually based on but could differ slightly from the provided timeout.
    2. Queue a task to run the provided callback.
  2. Return a number that could be passed to clearTimeout().

In comparison, the way Node.js timers run is this:

  1. Add the provided callback to the timer queue.
  2. If the timer thread hasn't been started, then run these steps in another thread:
    1. Wait for some time, usually the shortest timeout of a registered callback in the timer queue.
    2. Queue a task to run these steps:
      1. Run all callbacks with this timeout, one by one..
      2. If there are remaining callbacks in the timer, then redo step 2 "If the timer thread…" again.
      3. Run all microtasks. It's actually run in the destructor of InternalCallbackScope here (when there are no process.nextTick() callbacks scheduled) or here (when there are).
      4. Otherwise, terminate this thread.

(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.

@jasnell
Copy link
Member

jasnell commented Aug 11, 2018

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

@benjamingr
Copy link
Member

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.

@Fishrock123
Copy link
Contributor

Fishrock123 commented Aug 13, 2018

It seems like there are some misconceptions here

Node ... The result is actually inconsistent. From my tests, the most frequent result is: ... But sometimes it matches the one given by the browsers.

That's not actually possible. Please re-verify that.

See below

The way setTimeout() works is roughly:

setTimeout() could, for all we know, be implemented using the same system that Node.js uses. It doesn't really matter.

That said - in this particular case I believe we should align in browsers and always run microticks before timers if at all possible.

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.

@TimothyGu
Copy link
Member

TimothyGu commented Aug 13, 2018

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.

@Fishrock123
Copy link
Contributor

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.

@Fishrock123
Copy link
Contributor

Ok well it actually gets completely stuck.

[10:16|% 99|+ 2353|- 10]: release test-vm-ownpropertysymbols
=== release test-child-process-ipc-next-tick ===
Path: parallel/test-child-process-ipc-next-tick
Command: out/Release/node /Users/Jeremiah/Documents/node/test/parallel/test-child-process-ipc-next-tick.js
--- TIMEOUT ---
=== release test-common ===
Path: parallel/test-common
foo
bar
baz
test
foo
bar
baz
test
Command: out/Release/node /Users/Jeremiah/Documents/node/test/parallel/test-common.js
--- TIMEOUT ---
=== release test-domain-error-types ===
Path: parallel/test-domain-error-types
Command: out/Release/node /Users/Jeremiah/Documents/node/test/parallel/test-domain-error-types.js
--- TIMEOUT ---
=== release test-domain-multiple-errors ===
Path: parallel/test-domain-multiple-errors
Command: out/Release/node /Users/Jeremiah/Documents/node/test/parallel/test-domain-multiple-errors.js
--- TIMEOUT ---
=== release test-domain-stack ===
Path: parallel/test-domain-stack
Command: out/Release/node /Users/Jeremiah/Documents/node/test/parallel/test-domain-stack.js
--- TIMEOUT ---
=== release test-process-fatal-exception-tick ===
Path: parallel/test-process-fatal-exception-tick
assert.js:84
  throw new AssertionError(obj);
  ^

AssertionError [ERR_ASSERTION]: Expected inputs to be strictly equal:

1 !== 2

    at process.nextTick (/Users/Jeremiah/Documents/node/test/parallel/test-process-fatal-exception-tick.js:22:33)
    at _tickCallback (internal/process/next_tick.js:61:11)
    at processImmediate (timers.js:616:5)
Command: out/Release/node /Users/Jeremiah/Documents/node/test/parallel/test-process-fatal-exception-tick.js
=== release test-timers-immediate-queue ===
Path: parallel/test-timers-immediate-queue
hit 1
assert.js:84
  throw new AssertionError(obj);
  ^

AssertionError [ERR_ASSERTION]: Expected inputs to be strictly equal:

1 !== 1000

    at process.<anonymous> (/Users/Jeremiah/Documents/node/test/parallel/test-timers-immediate-queue.js:53:10)
    at process.emit (events.js:187:15)
Command: out/Release/node /Users/Jeremiah/Documents/node/test/parallel/test-timers-immediate-queue.js
=== release test-process-next-tick ===
Path: parallel/test-process-next-tick
Command: out/Release/node /Users/Jeremiah/Documents/node/test/parallel/test-process-next-tick.js
--- TIMEOUT ---
=== release test ===
Path: addons/make-callback-recurse/test
(node:81664) [DEP0097] DeprecationWarning: Using a domain property in MakeCallback is deprecated. Use the async_context variant of MakeCallback or the AsyncResource class instead.
Command: out/Release/node /Users/Jeremiah/Documents/node/test/addons/make-callback-recurse/test.js
--- TIMEOUT ---
=== release test ===
Path: addons-napi/test_make_callback_recurse/test
(node:81809) [DEP0097] DeprecationWarning: Using a domain property in MakeCallback is deprecated. Use the async_context variant of MakeCallback or the AsyncResource class instead.
Command: out/Release/node /Users/Jeremiah/Documents/node/test/addons-napi/test_make_callback_recurse/test.js
--- TIMEOUT ---
[10:16|%  99|+ 2353|-  10]: release test-vm-ownpropertysymbols

@addaleax
Copy link
Member

Node ... The result is actually inconsistent. From my tests, the most frequent result is: ... But sometimes it matches the one given by the browsers.

That's not actually possible. Please re-verify that.

@Fishrock123 I can reproduce it locally (Linux).

@devsnek
Copy link
Member

devsnek commented Aug 13, 2018

@Fishrock123 i can also reproduce that (macos)

@apapirovski
Copy link
Member

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.

@apapirovski
Copy link
Member

Also worth mentioning that it has non-trivial impact on peformance of timers and immediates.

@benjamingr
Copy link
Member

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.

Can you name one on top of your head? Should we perhaps make a PR and get a CITGM run for breakage?

@Fishrock123
Copy link
Contributor

Fishrock123 commented Aug 14, 2018

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/

@Fishrock123
Copy link
Contributor

Fishrock123 commented Sep 26, 2018

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.)

@devsnek
Copy link
Member

devsnek commented Sep 26, 2018

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 queueMicrotask which better serves the task. It is currently available in safari tech preview and node nightly builds. (chrome and firefox have bugs open to implement it, so feel free to find those and comment to push them forward)

@Fishrock123
Copy link
Contributor

No, that is not a correct solution to the actual problem, Gus.

And besides it won't run in between anyways.

@benjamingr
Copy link
Member

@devsnek to add to what Jeremiah said - would you mind explaining how that solves the timing issue?

@devsnek
Copy link
Member

devsnek commented Sep 28, 2018

@benjamingr maybe i just misunderstood the intention of the example the OP gave.

@benjamingr
Copy link
Member

@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").

jasnell pushed a commit that referenced this issue Oct 21, 2018
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants