run microtasks before ticks by ronag · Pull Request #51267 · nodejs/node · GitHub

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

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

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

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.

Refs: nodejs#51267

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.

lgtm

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

@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?

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

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

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

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

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.

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 benjamingr and jasnell 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

Comment thread lib/internal/process/task_queues.js Outdated

Comment thread lib/internal/process/task_queues.js Outdated

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


		> Stability: 1 - Experimental

		Enable experimental task ordering. Always drain micro task queue

+              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`.

run microtasks before ticks by ronag · Pull Request #51267 · nodejs/node · GitHub

Uh oh!

Uh oh!

run microtasks before ticks - #51267

run microtasks before ticks#51267ronag wants to merge 1 commit intonodejs:mainnodejs/node:mainfrom nxtedition:ticks-and-rejectionsnxtedition/node:ticks-and-rejectionsCopy head branch name to clipboard

Conversation

ronag commented Dec 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as resolved.

jasnell commented Dec 23, 2023

Uh oh!

ronag commented Dec 24, 2023

Uh oh!

ronag commented Dec 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

ronag commented Dec 24, 2023

Uh oh!

ronag commented Dec 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ronag commented Dec 25, 2023

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Dec 27, 2023

Uh oh!

benjamingr Dec 28, 2023

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Dec 29, 2023

Uh oh!

joyeecheung Jan 8, 2024

Choose a reason for hiding this comment

Uh oh!

joyeecheung Jan 8, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

run microtasks before ticks#51267
ronag wants to merge 1 commit into
nodejs:mainfrom
nxtedition:ticks-and-rejections

ronag commented Dec 23, 2023 •
edited
Loading

ronag commented Dec 24, 2023 •
edited
Loading

ronag commented Dec 25, 2023 •
edited
Loading