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

process: add deferTick #51471

Closed
wants to merge 1 commit into from
Closed

process: add deferTick #51471

wants to merge 1 commit into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Jan 15, 2024

Adds a new scheduling primitive to resolve Zalgo when mixing traditional Node async programming (events and callbacks) with the micro task queue (i.e. async/await, Promise, queueMicrotask).

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.

queueMicrotask is not sufficient to solve this as it is re-entrant.

See following examples:

import { EventEmitter } from 'events'

process.on('uncaughtException', err => {
  console.error("###", err)
})

// Unhandled exception
setImmediate(async () => {
  const e = await new Promise(resolve => {
    const e = new EventEmitter()
    resolve(e)
    process.nextTick(() => {
      e.emit('error', new Error('nextTick'))
    })
  })
  e.on('error', () => {})
})

// Unhandled exception
setImmediate(async () => {
  const e = await new Promise(resolve => {
    const e = new EventEmitter()
    resolve(e)
    queueMicrotask(() => {
      e.emit('error', new Error('queueMicrotask'))
    })
  })
  e.on('error', () => {})
})

// OK, but slow
setImmediate(async () => {
  const e = await new Promise(resolve => {
    const e = new EventEmitter()
    resolve(e)
    setImmediate(() => {
      e.emit('error', new Error('setImmediate'))
    })
  })
  e.on('error', () => {})
})

// OK
setImmediate(async () => {
  const e = await new Promise(resolve => {
    const e = new EventEmitter()
    resolve(e)
    process.deferTick(() => {
      e.emit('error', new Error('deferTick'))
    })
  })
  e.on('error', () => {})
})

Refs: #51156
Refs: #51280
Refs: #51114
Refs: #51070
Refs: nodejs/undici#2497

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. process Issues and PRs related to the process subsystem. labels Jan 15, 2024
@ronag

This comment was marked as resolved.

@ronag ronag force-pushed the defer-tick branch 2 times, most recently from 4bcb49e to 7df14ac Compare January 15, 2024 08:09
@ronag ronag marked this pull request as draft January 15, 2024 08:20
@ronag ronag force-pushed the defer-tick branch 11 times, most recently from bca84c2 to e3ffd48 Compare January 15, 2024 14:22
@ronag ronag marked this pull request as ready for review January 15, 2024 14:22
@ronag ronag requested a review from jasnell January 15, 2024 14:22
@ronag ronag added the async_hooks Issues and PRs related to the async hooks subsystem. label Jan 15, 2024
@ronag ronag force-pushed the defer-tick branch 6 times, most recently from 8520eff to 2515178 Compare January 15, 2024 14:45
doc/api/process.md Outdated Show resolved Hide resolved
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
Copy link
Member Author

ronag commented Jan 15, 2024

This has the same bug just moved right? If someone adds the callback inside a deferTick (caused either by them or the library) they get the same behavior.

No... I'm pretty sure this resolves it properly since it's not re-entrant. Can you provide an example?

I suspect if we encourage people to deferTick we'll get the same bug just later and complicate our scheduling which would ideally be just queueMicrotask and promises (and we should discourage nextTick since there is a parallel standard mechanism).

I think you might be wrong. Again do you have an example)

So I don't think there is a way around "educate people to attach error events synchronously since they are emitted with microtask queue semantics" - this is the case with promises APIs anyway.

What do you mean with promises api anyway? Example?

@ronag
Copy link
Member Author

ronag commented Jan 15, 2024

Please don't get hung up on just solving the error handling case. We have the same problem with any other event. In those cases we have a deadlock instead of an uncaught error, which is even worse.

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!

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 requested a review from benjamingr January 16, 2024 13:46
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
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
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

@ronag ronag added notable-change PRs with changes that should be highlighted in changelogs. request-ci Add this label to start a Jenkins CI on a PR. labels Jan 16, 2024
Copy link
Contributor

The notable-change PRs with changes that should be highlighted in changelogs. label has been added by @ronag.

Please suggest a text for the release notes if you'd like to include a more detailed summary, then proceed to update the PR description with the text or a link to the notable change suggested text comment. Otherwise, the commit will be placed in the Other Notable Changes section.

@ronag ronag added the blocked PRs that are blocked by other issues or PRs. label Jan 16, 2024
@ronag
Copy link
Member Author

ronag commented Jan 16, 2024

I'm putting blocked label on this for now to make sure it doesn't land without more reviews. In particular I would appreciate feedback from @benjamingr and @jasnell

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 16, 2024
@nodejs-github-bot
Copy link
Collaborator

@benjamingr
Copy link
Member

This has the same bug just moved right? If someone adds the callback inside a deferTick (caused either by them or the library) they get the same behavior.

No... I'm pretty sure this resolves it properly since it's not re-entrant. Can you provide an example?

I'm not sure what you mean by re-entrant in this context but basically:

// Unhandled exception
setImmediate(async () => {
  const e = await new Promise(resolve => {
    const e = new EventEmitter()
    resolve(e)
    process.deferTick(() => {
      e.emit('error', new Error('queueMicrotask'))
    })
  })
  // also listen in a defer tick callback, presumably because of a library
  process.deferTick(() => {
    e.on('error', () => {})
  });
})

Where the example is contrived (like the others) and in real-world usage the deferTick call would likely come from a library trying to "do the right thing" with a callback

@benjamingr
Copy link
Member

What do you mean with promises api anyway? Example?

Just the fact you have to attach an error listener synchronously or you get an unhandled rejection - just like emitter errors

@ronag
Copy link
Member Author

ronag commented Jan 17, 2024

@benjamingr I feel we are out of sync. Do you have any possibility for a 15 min call?

@ronag
Copy link
Member Author

ronag commented Jan 17, 2024

Where the example is contrived (like the others) and in real-world usage the deferTick call would likely come from a library trying to "do the right thing" with a callback

Exactly, this is something that would be mostly used by library developers. We have already the problem with e.g. undici.request which returns a Promise<Stream> and there is no perfect solution atm without process.deferTick.

@benjamingr
Copy link
Member

@benjamingr I feel we are out of sync. Do you have any possibility for a 15 min call?

Due to war I'm less available but I'll come to the meeting in 3m but also you have my phone number and are always welcome to whatsapp me.

which returns a Promise and there is no perfect solution atm without process.deferTick.

The question is what happens if the promise resolution is itself deferred by a deferTick in this case for example which would again prevent people from adding an error listener ahead of time.

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

I'm not thrilled with the idea of adding Yet Another Scheduling Primitive but not sure there's much of a choice if we want these cases to Just Work... so, LGTM

@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
async_hooks Issues and PRs related to the async hooks subsystem. blocked PRs that are blocked by other issues or PRs. needs-ci PRs that need a full CI run. notable-change PRs with changes that should be highlighted in changelogs. process Issues and PRs related to the process subsystem. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants