FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

move away from process.nextTick by ronag · Pull Request #51114 · nodejs/node · GitHub

/ node Public

move away from process.nextTick - #51114

Closed
ronag wants to merge 1 commit into
nodejs:mainfrom
nxtedition:no-nextTick
ConversationCommits (1)ChecksFiles changed
Closed

move away from process.nextTick#51114
ronag wants to merge 1 commit into
nodejs:mainfrom
nxtedition:no-nextTick

Conversation

ronag commented Dec 11, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

There are two problems with process.nextTick.

  1. (LOW SEVERITY) It reduces performance for all async callbacks from native code.

  2. (HIGH SEVERITY) It causes weird and unpredictable behavior when trying to interop with promises and async/await code.

In particular, we have an invariant where we always emit events and invoke callbacks "asynchronously". However, that doesn't apply to Promise, since we "force" asynchronously through process.nextTick which occurs before any microtick. Hence, for any promise/micro-tick-based code things appear to occur synchronously. This leads to all kinds of weird and hard-to-debug bugs.

e.g.

const foo = await new Promise((resolve) => resolve(new Bar()))
// You can replace `'error'` with any event name, `'open'`, `'close'`, etc...
// event might be emitted through process.nextTick before 
// we ever  get a chance to install a handler.
foo.on('error', errorHandler) a handler.

Refs: #51070
Refs: #51156

nodejs-github-bot commented Dec 11, 2023

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/crypto
  • @nodejs/http
  • @nodejs/net
  • @nodejs/startup

nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Dec 11, 2023

ronag commented Dec 11, 2023

Copy link
Copy Markdown
Member Author

This should probably be split into separate PRs for each module with tests added. However, I don't have time for that.

I'm mostly opening this PR for visibility and discussion and hoping someone might take over if consensus is that a simple find and replace PR is not landable.

ronag requested review from ShogunPanda, jasnell and mcollina and removed request for ShogunPanda, jasnell and mcollina December 11, 2023 08:20
ronag added the semver-major PRs that contain breaking changes and should be released in the next major version. label Dec 11, 2023

mcollina commented Dec 11, 2023

Copy link
Copy Markdown
Member

this needs a Citgm run and investigate any failures deeply, I have a feeling this will be breaking.

I’m +1 in principle.

ronag requested a review from mcollina December 11, 2023 09:10
move away from process.nextTick
8ce0fe3 
There are two problems with process.nextTick.

Less severe is that it reduces performance for
all async callback from native code.

The more severe one is that it causes weird and
unpredictable behavior when trying to interop
with promises and async/await code.

In particular, we have an invariant where we always
emit certain events and invoke certain callbacks
"asynchronously". However, that currently doesn't
apply to Promise, since we "force" asynchronousity
throug process.nextTick which occurs before any
microtick. Hence, for any promise/micro-tick based
code things actually appear to occur synchronously.

Refs: nodejs#51070
PR: nodejs#51114
ronag added the needs-citgm PRs that need a CITGM CI run. label Dec 11, 2023

ronag commented Dec 11, 2023

Copy link
Copy Markdown
Member Author

I've done what I have time with. This needs another champion to get over finish line.

NiharPhansalkar commented Dec 18, 2023

Copy link
Copy Markdown
Contributor

Hello @ronag. I have understood the issue here, but I am not sure I understand the solution to this problem. Can you point me in the right direction? I did search around and ask AI, and it seems the suggested solution is to replace process.nextTick() with queueMicrotask?
Can you confirm if this is exactly what has to be done in order to resolve this issue? Or are there any other changes that are required?

ronag commented Dec 18, 2023

Copy link
Copy Markdown
Member Author

Related #51156

jasnell commented Dec 22, 2023

Copy link
Copy Markdown
Member

Like @mcollina I'm +1 with the idea here but I definitely think this needs to be very carefully tested as this could end up breaking a LOT of code out there.

I also wonder if we shouldn't give users a temporary escape hatch even after this moves forward to allow them to opt-back-in to using process.nextTick instead. That can be accomplished by setting up an internal alias method that by default is queueMicrotask but is set to process.nextTick if a CLI flag or env var is used.

e.g.

const internalTick = hasOption('--use-internal-nexttick') ? process.nextTick : queueMicrotask;

ShogunPanda commented Dec 22, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

I reviewed the files.
The changes looks fine to me, but definitely like James' idea. I think it's the safest approach here, for both users and us.

jasnell commented Dec 22, 2023

Copy link
Copy Markdown
Member

Another option here: Perhaps instead of changing the process.nextTick callsites into queueMicrotask calls directly... perhaps process.nextTick itself could become an alias for queueMicrotask. This would help to avoid odd timing cases where Node.js changes to queueMicrotask but user code still contains to use process.nextTick

Qard commented Dec 22, 2023

Copy link
Copy Markdown
Member

Yeah, I would go for just swapping out the internals of process.nextTick to delegate to queueMicrotask and then include a flag as an escape hatch to restore the old behaviour in case it breaks anyone. They'll no doubt come to the issue tracker and complain, which will give us a signal on impact, but having an immediate fix for them with a flag should soften the blow. 😅

ronag marked this pull request as draft December 23, 2023 10:04
marco-ippolito mentioned this pull request Dec 24, 2023
Merged
ronag added a commit to nxtedition/node that referenced this pull request Dec 29, 2023
lib: run microtasks before ticks
da76346 
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 mentioned this pull request Jan 15, 2024
Closed
ronag added a commit to nxtedition/node that referenced this pull request Jan 16, 2024
process: add deferTick
9e07efe 
Adds a new scheduling primitive to resolve zaldo when mixing
traditional Node async programming with async/await and Promises.

We cannot "fix" nextTick without breaking the whole ecosystem.
nextTick usage should be discouraged and we should try to
incrementally move to this new primitive.

Refs: nodejs#51156
Refs: nodejs#51280
Refs: nodejs#51114
Refs: nodejs#51070
Refs: nodejs/undici#2497
PR-URL: nodejs#51471
ronag mentioned this pull request Feb 21, 2024
Closed
6 tasks
ronag closed this Jun 6, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

ShogunPanda Awaiting requested review from ShogunPanda
jasnell Awaiting requested review from jasnell
mcollina Awaiting requested review from mcollina

Assignees

No one assigned

Labels

lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. needs-citgm PRs that need a CITGM CI run. semver-major PRs that contain breaking changes and should be released in the next major version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

Back | FazBrowse Home | New Git URL