run microtasks before ticks #51267

ronag · 2023-12-23T10:01:41Z

This resolve multiple timing issues related to promises and nextTick. As well as resolving zaldo in promise only code, i.e. our current best practice of using process.nextTick will always apply and work.

Enable experimental task ordering. Always drain micro task queue before running process.nextTick to avoid unintuitive behavior and unexpected logical deadlocks when mixing async callback and event API's with Promise, async/await and queueMicroTask.

Refs: #51156
Refs: #51156
Refs: #51114 (comment)
Refs: #51070
Refs: #51156

This resolve multiple timing issues related to promises and nextTick. As well as resolving zaldo in promise only code, i.e. our current best practice of using process.nextTick will always apply and work. Refs: nodejs#51156 Refs: nodejs#51156 (comment) Refs: nodejs#51114 (comment) Refs: nodejs#51070 Refs: nodejs#51156 PR-URL: nodejs#51267

This resolve multiple timing issues related to promises and nextTick. As well as resolving zaldo in promise only code, i.e. our current best practice of using process.nextTick will always apply and work. Refs: nodejs#51156 Refs: nodejs#51156 (comment) Refs: nodejs#51114 Refs: nodejs#51070 Refs: nodejs#51156 PR-URL: nodejs#51267

jasnell · 2023-12-23T14:52:54Z

If think that IF we do this, which is something we need to very carefully consider given the very real chance of breakage, we should include an escape hatch in the form of a command-line argument that restores the original ordering. We can eventually deprecate that flag once the ecosystem has had a while to make the transition.

./node --legacy-microtask-ordering

This resolve multiple timing issues related to promises and nextTick. As well as resolving zaldo in promise only code, i.e. our current best practice of using process.nextTick will always apply and work. Refs: nodejs#51156 Refs: nodejs#51156 (comment) Refs: nodejs#51114 Refs: nodejs#51070 Refs: nodejs#51156 PR-URL: nodejs#51267

ronag · 2023-12-24T10:35:31Z

What about having it as an opt-in for now?

This resolve multiple timing issues related to promises and nextTick. As well as resolving zaldo in promise only code, i.e. our current best practice of using process.nextTick will always apply and work. Refs: nodejs#51156 Refs: nodejs#51156 (comment) Refs: nodejs#51114 Refs: nodejs#51070 Refs: nodejs#51156 PR-URL: nodejs#51267

ronag · 2023-12-24T10:40:03Z

It might be difficult or impossible to have this by default but I still think we need to have the possibility to opt-in as this is the only way to get "correct" behavior.

This resolve multiple timing issues related to promises and nextTick. As well as resolving zaldo in promise only code, i.e. our current best practice of using process.nextTick will always apply and work. Refs: nodejs#51156 Refs: nodejs#51156 (comment) Refs: nodejs#51114 Refs: nodejs#51070 Refs: nodejs#51156 PR-URL: nodejs#51267

Refs: nodejs#51267

mcollina

I would prefer if this super-hot function would not get a few more ifs. I think you could duplicate the implementation and only change the export.

This pattern would also allow us for more experimentation.

This resolve multiple timing issues related to promises and nextTick. As well as resolving zaldo in promise only code, i.e. our current best practice of using process.nextTick will always apply and work. Refs: nodejs#51156 Refs: nodejs#51156 (comment) Refs: nodejs#51114 Refs: nodejs#51070 Refs: nodejs#51156 PR-URL: nodejs#51267

mcollina

lgtm

ronag · 2023-12-24T18:33:04Z

Please note that this is necessary as queueMicrotask is not sufficient to replace the current nextTick due to its re-entrance behavior.

ronag · 2023-12-25T09:28:38Z

@mcollina I can't get this to work without a function wrapper due to the following lint rule:

node:internal/options:20
    ({ options: optionsMap } = getCLIOptions());
                               ^

Error: Should not query options before bootstrapping is done
    at getCLIOptionsFromBinding (node:internal/options:20:32)
    at getOptionValue (node:internal/options:45:19)
    at setupTaskQueue (node:internal/process/task_queues:210:34)
    at node:internal/bootstrap/node:306:38

Any suggestions? Or who might have some idea?

ronag · 2023-12-25T09:34:21Z

@joyeecheung You are the one that added the rule I believe?

lib/internal/process/task_queues.js

nodejs-github-bot · 2023-12-27T16:28:09Z

CI: https://ci.nodejs.org/job/node-test-pull-request/56557/

benjamingr · 2023-12-28T07:50:42Z

lib/internal/process/task_queues.js

+          callback();
+        } else {
+          const args = tock.args;
+          switch (args.length) {


is this actually faster?

This resolve multiple timing issues related to promises and nextTick. As well as resolving zaldo in promise only code, i.e. our current best practice of using process.nextTick will always apply and work. Refs: nodejs#51156 Refs: nodejs#51156 (comment) Refs: nodejs#51114 Refs: nodejs#51070 Refs: nodejs#51156 PR-URL: nodejs#51267

nodejs-github-bot · 2023-12-29T19:39:01Z

CI: https://ci.nodejs.org/job/node-test-pull-request/56587/

joyeecheung · 2024-01-08T22:37:32Z

doc/api/cli.md

+
+> Stability: 1 - Experimental
+
+Enable experimental task ordering. Always drain micro task queue


Suggested change

Enable experimental task ordering. Always drain micro task queue

Enable experimental task ordering. Always drain Microtasks queue

It would also be useful to link to a doc like https://developer.mozilla.org/en-US/docs/Web/API/HTML_DOM_API/Microtask_guide#microtasks here.

joyeecheung · 2024-01-08T22:39:57Z

doc/api/cli.md

+Enable experimental task ordering. Always drain micro task queue
+before running `process.nextTick` to avoid unintuitive behavior
+and unexpected logical deadlocks when mixing async callback and
+event API's with `Promise`, `async`/`await`` and `queueMicroTask`.


Can you provide some examples? Without examples I doubt how many users would be able to recognize whether this can be useful for them and pick up this option.

Also note that we have a section in process.md saying..

every time the "next tick queue" is drained, the microtask queue
is drained immediately after.

I think that should be updated to mention this option.

nodejs-github-bot added needs-ci PRs that need a full CI run. process Issues and PRs related to the process subsystem. labels Dec 23, 2023

ronag force-pushed the ticks-and-rejections branch from c8b5df2 to 66904c9 Compare December 23, 2023 10:02

ronag force-pushed the ticks-and-rejections branch from 66904c9 to d95c4cb Compare December 23, 2023 10:03

ronag added the semver-major PRs that contain breaking changes and should be released in the next major version. label Dec 23, 2023

ronag requested review from jasnell and benjamingr December 23, 2023 10:03

This comment was marked as resolved.

Sign in to view

ronag force-pushed the ticks-and-rejections branch from d95c4cb to 17de9ba Compare December 24, 2023 10:33

ronag force-pushed the ticks-and-rejections branch from 17de9ba to 1a5a03b Compare December 24, 2023 10:34

ronag requested a review from mcollina December 24, 2023 10:35

ronag force-pushed the ticks-and-rejections branch from 1a5a03b to 6fedd50 Compare December 24, 2023 10:38

ronag force-pushed the ticks-and-rejections branch from 6fedd50 to 9df2edf Compare December 24, 2023 10:39

ronag force-pushed the ticks-and-rejections branch from 9df2edf to c505bc5 Compare December 24, 2023 11:40

ronag added a commit to nxtedition/node that referenced this pull request Dec 24, 2023

lib: use deferred queue for all internal ticks calls

0dcb465

Refs: nodejs#51267

ronag mentioned this pull request Dec 24, 2023

lib: use deferred queue for all internal ticks calls #51279

Closed

mcollina requested changes Dec 24, 2023

View reviewed changes

ronag force-pushed the ticks-and-rejections branch from c505bc5 to 440ff33 Compare December 24, 2023 13:01

ronag force-pushed the ticks-and-rejections branch from 440ff33 to d32a6ea Compare December 24, 2023 13:02

ronag force-pushed the ticks-and-rejections branch from 0e15cc6 to ce47a8d Compare December 24, 2023 13:44

ronag force-pushed the ticks-and-rejections branch from ce47a8d to 0d9b9ca Compare December 24, 2023 13:44

mcollina approved these changes Dec 24, 2023

View reviewed changes

marco-ippolito mentioned this pull request Dec 24, 2023

doc: mark process.nextTick legacy #51280

Merged

trivikr reviewed Dec 27, 2023

View reviewed changes

lib/internal/process/task_queues.js Outdated Show resolved Hide resolved

lib/internal/process/task_queues.js Outdated Show resolved Hide resolved

trivikr approved these changes Dec 27, 2023

View reviewed changes

ronag added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 27, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 27, 2023

benjamingr reviewed Dec 28, 2023

View reviewed changes

ronag force-pushed the ticks-and-rejections branch 4 times, most recently from a88c594 to 3a1c141 Compare December 29, 2023 19:36

ronag requested review from benjamingr and mcollina December 29, 2023 19:37

ronag force-pushed the ticks-and-rejections branch from 3a1c141 to 34b7227 Compare December 29, 2023 19:37

ronag added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 29, 2023

ronag force-pushed the ticks-and-rejections branch from 34b7227 to da76346 Compare December 29, 2023 19:38

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 29, 2023

joyeecheung reviewed Jan 8, 2024

View reviewed changes

ronag closed this Jan 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

run microtasks before ticks #51267

run microtasks before ticks #51267

ronag commented Dec 23, 2023 •

edited

Loading

This comment was marked as resolved.

jasnell commented Dec 23, 2023

ronag commented Dec 24, 2023

ronag commented Dec 24, 2023 •

edited

Loading

mcollina left a comment

mcollina left a comment

ronag commented Dec 24, 2023

ronag commented Dec 25, 2023 •

edited

Loading

ronag commented Dec 25, 2023

nodejs-github-bot commented Dec 27, 2023

benjamingr Dec 28, 2023

nodejs-github-bot commented Dec 29, 2023

joyeecheung Jan 8, 2024

joyeecheung Jan 8, 2024


		> Stability: 1 - Experimental

		Enable experimental task ordering. Always drain micro task queue

run microtasks before ticks #51267

run microtasks before ticks #51267

Conversation

ronag commented Dec 23, 2023 • edited Loading

This comment was marked as resolved.

jasnell commented Dec 23, 2023

ronag commented Dec 24, 2023

ronag commented Dec 24, 2023 • edited Loading

mcollina left a comment

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

ronag commented Dec 24, 2023

ronag commented Dec 25, 2023 • edited Loading

ronag commented Dec 25, 2023

nodejs-github-bot commented Dec 27, 2023

benjamingr Dec 28, 2023

Choose a reason for hiding this comment

nodejs-github-bot commented Dec 29, 2023

joyeecheung Jan 8, 2024

Choose a reason for hiding this comment

joyeecheung Jan 8, 2024

Choose a reason for hiding this comment

ronag commented Dec 23, 2023 •

edited

Loading

ronag commented Dec 24, 2023 •

edited

Loading

ronag commented Dec 25, 2023 •

edited

Loading