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

stream: fix timing relative to promises #51070

Closed
wants to merge 1 commit into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Dec 6, 2023

Workaround for Node "bug". If the stream is destroyed in same
tick as it is created, then a user who is waiting for a
promise (i.e micro tick) for installing a 'error' listener will
never get a chance and will always encounter an unhandled exception.

  • tick => process.nextTick(fn)
  • micro tick => queueMicrotask(fn)

Refs: nodejs/undici#2497

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/streams

@ronag ronag requested review from ShogunPanda and rluvaton and removed request for rluvaton December 6, 2023 12:08
@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Dec 6, 2023
@ronag ronag force-pushed the stream-timing branch 3 times, most recently from 32b6db1 to d44bc8d Compare December 6, 2023 12:09
@ronag ronag marked this pull request as ready for review December 6, 2023 12:09
ronag added a commit to nxtedition/node that referenced this pull request Dec 6, 2023
@ronag ronag changed the title streams: fix timing relative to promises stream: fix timing relative to promises Dec 6, 2023
@ronag
Copy link
Member Author

ronag commented Dec 6, 2023

Unclear whether or not this needs to be semver major. It fixes a rather serious bug.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

I recommend shipping in v21 and analyze the breakage (wait before backporting)

@ronag ronag added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 6, 2023
ronag added a commit to nxtedition/node that referenced this pull request Dec 6, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 6, 2023
@nodejs-github-bot
Copy link
Collaborator

Copy link
Contributor

@ShogunPanda ShogunPanda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@Flarna
Copy link
Member

Flarna commented Dec 6, 2023

Could you please add a bit more info in the commit message and/or PR description what the actual problem is?
The linked issue is nice to read but doesn't really describe problem/solution.

@ronag ronag added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 10, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 10, 2023
@nodejs-github-bot
Copy link
Collaborator

ronag added a commit to nxtedition/node that referenced this pull request Dec 23, 2023
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
ronag added a commit to nxtedition/node that referenced this pull request Dec 23, 2023
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 added a commit to nxtedition/node that referenced this pull request Dec 24, 2023
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 added a commit to nxtedition/node that referenced this pull request Dec 24, 2023
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 added a commit to nxtedition/node that referenced this pull request Dec 24, 2023
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 added a commit to nxtedition/node that referenced this pull request Dec 24, 2023
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 added a commit to nxtedition/node that referenced this pull request Dec 24, 2023
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 added a commit to nxtedition/node that referenced this pull request Dec 24, 2023
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 added a commit to nxtedition/node that referenced this pull request Dec 24, 2023
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 added a commit to nxtedition/node that referenced this pull request Dec 24, 2023
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 added a commit to nxtedition/node that referenced this pull request Dec 24, 2023
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 added a commit to nxtedition/node that referenced this pull request Dec 24, 2023
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 added a commit to nxtedition/node that referenced this pull request Dec 29, 2023
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 added a commit to nxtedition/node that referenced this pull request Dec 29, 2023
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 added a commit to nxtedition/node that referenced this pull request Jan 15, 2024
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 ronag mentioned this pull request Jan 15, 2024
ronag added a commit to nxtedition/node that referenced this pull request Jan 15, 2024
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 added a commit to nxtedition/node that referenced this pull request Jan 15, 2024
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 added a commit to nxtedition/node that referenced this pull request Jan 15, 2024
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 added a commit to nxtedition/node that referenced this pull request Jan 15, 2024
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 added a commit to nxtedition/node that referenced this pull request Jan 15, 2024
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 added a commit to nxtedition/node that referenced this pull request Jan 15, 2024
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 added a commit to nxtedition/node that referenced this pull request Jan 15, 2024
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 added a commit to nxtedition/node that referenced this pull request Jan 15, 2024
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 added a commit to nxtedition/node that referenced this pull request Jan 16, 2024
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 added a commit to nxtedition/node that referenced this pull request Jan 16, 2024
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 added a commit to nxtedition/node that referenced this pull request Jan 16, 2024
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 added a commit to nxtedition/node that referenced this pull request Jan 16, 2024
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 ronag closed this Jun 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lts-watch-v20.x PRs that may need to be released in v20.x needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants