Inconsistent behavior of nextTick and queueMicrotask

[mcollina]

Consider this snippet:

const { EventEmitter } = require('node:events')

function run (name) {
  return new Promise((resolve) => {
    const e = new EventEmitter()
    process.nextTick(() => {
      e.emit('error', new Error())
    });
    resolve(e)
  }).then(((e) => {
    e.on('error', err => {
      console.error(`### ${name} ###`, err)
    })
  }))
}

queueMicrotask(run.bind(null, 'queueMicrotask'))
setImmediate(run.bind(null, 'setImmediate'))

This results in

### queueMicrotask ### Error
    at /Users/matteo/c.cjs:7:23
    at process.processTicksAndRejections (node:internal/process/task_queues:77:11)
node:events:492
      throw er; // Unhandled 'error' event
      ^

Error
    at /Users/matteo/c.cjs:7:23
    at process.processTicksAndRejections (node:internal/process/task_queues:77:11)
Emitted 'error' event at:
    at /Users/matteo/c.cjs:7:9
    at process.processTicksAndRejections (node:internal/process/task_queues:77:11)

Node.js v20.10.0

Why is this an issue? In most part of Node.js core,
we use process.nextTick(() => ee.emit('error')) to let users install event handlers.
However, if an EventEmitter is returned by an async function, there is a significant possibility that the error could not be caught.

(This is essentially a problem for Node.js streams).

[mcollina]

@jasnell what do you think?

[ronag]

Just to expand a bit. There is an assumption that process.nextTick tasks always run before micro-tasks. However, as demonstrated above. This is a false assumption. The reality is more inconsistent and depends whether or not the caller is inside our outside a micro tick.

[ronag]

I'm adding this to the TSC agenda for visibility and help with triaging the severity of the issue.

[ronag]

Would probably be a good idea to update https://nodejs.org/en/guides/event-loop-timers-and-nexttick to also include promises and microticks.

[joyeecheung]

It's not entirely clear to me what is the expected output in the OP, did you mean to say that you expect a ### setImmediate ### Error log in addition to the queueMicrotask one? Or did you mean to expect to see just the setImmediate one?

[ronag]

It's not entirely clear to me what is the expected output in the OP, did you mean to say that you expect a ### setImmediate ### Error log in addition to the queueMicrotask one? Or did you mean to expect to see just the setImmediate one?

The expectation is that:

queueMicrotask(run.bind(null, 'queueMicrotask'))

and

setImmediate(run.bind(null, 'setImmediate'))

Should have the same output, which they don't.

[joyeecheung]

You mean neither of them should log anything or they should both log or either is fine as long as they are the same? I am guessing from the OP (in the context of error handling) that you want both to log?

[ronag]

I think you could argue that either behavior is correct (or at least not incorrect).

The problem here is that they are not consistent.

[NiharPhansalkar]

Hello @ronag, I would like to continue working on this issue, however, as mentioned on your PR, I guess a simple search and replace won't work?
I am actually new to contributing to NodeJS, so I am a little confused as to what we can implement here. If you could give me some pointers, it would be of great help.

[ronag]

@NiharPhansalkar I don't think this is a good starter issue. Not even us in the technical steering are sure what to do about this issue.

[mcollina]

@benjamingr here is the patch that fixes the behavior... but it breaks a lot of tests:

diff --git a/lib/internal/process/task_queues.js b/lib/internal/process/task_queues.js
index bcb5eef841..04bdb07122 100644
--- a/lib/internal/process/task_queues.js
+++ b/lib/internal/process/task_queues.js
@@ -67,6 +67,7 @@ function runNextTicks() {
 function processTicksAndRejections() {
   let tock;
   do {
+    runMicrotasks();
     while ((tock = queue.shift()) !== null) {
       const asyncId = tock[async_id_symbol];
       emitBefore(asyncId, tock[trigger_async_id_symbol], tock);
@@ -92,7 +93,6 @@ function processTicksAndRejections() {

       emitAfter(asyncId);
     }
-    runMicrotasks();
   } while (!queue.isEmpty() || processPromiseRejections());
   setHasTickScheduled(false);
   setHasRejectionToWarn(false);

[jasnell]

Unfortunately I don't believe there's a great fix here other than to begin transitioning from process.nextTick to queueMicrotask (or reimplementing process.nextTick in terms of queueMicrotask). But as evidenced, that's a massively breaking change.

[ronag]

Unfortunately I don't believe there's a great fix here other than to begin transitioning from process.nextTick to queueMicrotask (or reimplementing process.nextTick in terms of queueMicrotask). But as evidenced, that's a massively breaking change.

Even that doesn't fully resolve the issue. We need a primitive that runs after micro tasks.

[jasnell]

Well, it's not even really that. The key issue is the way we communicate errors and other state changes using non-persistent synchronous events that are easily missed. That is actually the thing I'd prefer we start to change here, but obviously that's also quite difficult. This is absolutely one of the ways in which web streams handily beats node.js streams.

For instance, we could hypothetically introduce a new whenErrored property on EventEmitter whose value is a promise that is resolved when the error event is emitted. This would allow us to be sure we can always capture the errored state. (I'm not saying we should, I'm just illustrating the point)

function run (name) {
  return new Promise((resolve) => {
    const e = new EventEmitter()
    process.nextTick(() => {
      e.emit('error', new Error())
    });
    resolve(e)
  }).then(((e) => {
    e.whenErrored.then((err) => /* ... */);
  }))
}

[ronag]

I disagree with your view of the root problem.

The problem is that we have two API paradigms here (async callbacks and async events) that are inherently incompatible with the micro task queue.

Note that this is a much more generic problem than just the error event. Though I agree it's more prevalent in that context.