From de7f0c1cbc6e1cb44bcf25d14d07f2f74874bd94 Mon Sep 17 00:00:00 2001
From: Robert Nagy <ronagy@icloud.com>
Date: Mon, 15 Jan 2024 08:59:46 +0100
Subject: [PATCH] process: add deferTick

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: https://github.com/nodejs/node/issues/51156
Refs: https://github.com/nodejs/node/pull/51280
Refs: https://github.com/nodejs/node/pull/51114
Refs: https://github.com/nodejs/node/pull/51070
Refs: https://github.com/nodejs/undici/pull/2497
PR-URL: https://github.com/nodejs/node/pull/51471
---
 doc/api/process.md                  | 68 +++++++++++++++++++++++++++++
 lib/internal/bootstrap/node.js      |  3 +-
 lib/internal/process/task_queues.js | 63 +++++++++++++++-----------
 test/async-hooks/test-defertick.js  | 15 +++++++
 4 files changed, 122 insertions(+), 27 deletions(-)
 create mode 100644 test/async-hooks/test-defertick.js

diff --git a/doc/api/process.md b/doc/api/process.md
index b53986f04ae18c..7c8f8336f6111a 100644
--- a/doc/api/process.md
+++ b/doc/api/process.md
@@ -1219,6 +1219,74 @@ const process = require('node:process');
 process.debugPort = 5858;
 ```
 
+## `process.deferTick(callback[, ...args])`
+
+<!-- YAML
+added: REPLACEME
+-->
+
+* `callback` {Function}
+* `...args` {any} Additional arguments to pass when invoking the `callback`
+
+`process.deferTick()` adds `callback` to the "defer tick queue". This queue is
+fully drained after the current operation on the JavaScript stack runs to
+completion and before the event loop is allowed to continue. It's possible to
+create an infinite loop if one were to recursively call `process.deferTick()`.
+See the [Event Loop][] guide for more background.
+
+Unlike `process.nextTick`, `process.deferTick()` will run after the "next tick
+queue" and the microtask queue has been fully drained as to avoid Zalgo when
+combinding traditional node asynchronous code with Promises.
+
+Consider the following example:
+
+```js
+// uncaughtException!
+setImmediate(async () => {
+  const e = await new Promise(resolve => {
+    const e = new EventEmitter()
+    resolve(e)
+    process.nextTick(() => {
+      e.emit('error', new Error('process.nextTick'))
+    })
+  })
+  e.on('error', () => {})
+})
+
+// uncaughtException!
+setImmediate(async () => {
+  const e = await new Promise(resolve => {
+    const e = new EventEmitter()
+    resolve(e)
+    queueMicrotask(() => {
+      e.emit('error', new Error('queueMicrotask'))
+    })
+  })
+  e.on('error', () => {})
+})
+```
+
+In both of these cases the user will encounter an
+`uncaughtException` error since the inner task
+will execute before control is returned to the
+caller of `await`. In order to fix this one should
+use `process.deferTick` which will execute in the
+expected order:
+
+```js
+// OK!
+setImmediate(async () => {
+  const e = await new Promise(resolve => {
+    const e = new EventEmitter()
+    resolve(e)
+    process.deferTick(() => {
+      e.emit('error', new Error('process.deferTick'))
+    })
+  })
+  e.on('error', () => {})
+})
+```
+
 ## `process.disconnect()`
 
 <!-- YAML
diff --git a/lib/internal/bootstrap/node.js b/lib/internal/bootstrap/node.js
index 7a773d5208e250..684c6f7bdae218 100644
--- a/lib/internal/bootstrap/node.js
+++ b/lib/internal/bootstrap/node.js
@@ -303,8 +303,9 @@ process.emitWarning = emitWarning;
 // bootstrap to make sure that any operation done before this are synchronous.
 // If any ticks or timers are scheduled before this they are unlikely to work.
 {
-  const { nextTick, runNextTicks } = setupTaskQueue();
+  const { nextTick, runNextTicks, deferTick } = setupTaskQueue();
   process.nextTick = nextTick;
+  process.deferTick = deferTick;
   // Used to emulate a tick manually in the JS land.
   // A better name for this function would be `runNextTicks` but
   // it has been exposed to the process object so we keep this legacy name
diff --git a/lib/internal/process/task_queues.js b/lib/internal/process/task_queues.js
index bcb5eef841dd00..e190bdbed2d33f 100644
--- a/lib/internal/process/task_queues.js
+++ b/lib/internal/process/task_queues.js
@@ -44,21 +44,14 @@ const { AsyncResource } = require('async_hooks');
 // *Must* match Environment::TickInfo::Fields in src/env.h.
 const kHasTickScheduled = 0;
 
-function hasTickScheduled() {
-  return tickInfo[kHasTickScheduled] === 1;
-}
-
-function setHasTickScheduled(value) {
-  tickInfo[kHasTickScheduled] = value ? 1 : 0;
-}
-
-const queue = new FixedQueue();
+let queue = new FixedQueue();
+let deferQueue = new FixedQueue();
 
 // Should be in sync with RunNextTicksNative in node_task_queue.cc
 function runNextTicks() {
-  if (!hasTickScheduled() && !hasRejectionToWarn())
+  if (tickInfo[kHasTickScheduled] === 0 && !hasRejectionToWarn())
     runMicrotasks();
-  if (!hasTickScheduled() && !hasRejectionToWarn())
+  if (tickInfo[kHasTickScheduled] === 0 && !hasRejectionToWarn())
     return;
 
   processTicksAndRejections();
@@ -93,46 +86,63 @@ function processTicksAndRejections() {
       emitAfter(asyncId);
     }
     runMicrotasks();
+
+    const tmp = queue;
+    queue = deferQueue;
+    deferQueue = tmp;
   } while (!queue.isEmpty() || processPromiseRejections());
-  setHasTickScheduled(false);
+  tickInfo[kHasTickScheduled] = 0;
   setHasRejectionToWarn(false);
 }
 
 // `nextTick()` will not enqueue any callback when the process is about to
 // exit since the callback would not have a chance to be executed.
-function nextTick(callback) {
+function nextTick(callback, ...args) {
   validateFunction(callback, 'callback');
 
   if (process._exiting)
     return;
 
-  let args;
-  switch (arguments.length) {
-    case 1: break;
-    case 2: args = [arguments[1]]; break;
-    case 3: args = [arguments[1], arguments[2]]; break;
-    case 4: args = [arguments[1], arguments[2], arguments[3]]; break;
-    default:
-      args = new Array(arguments.length - 1);
-      for (let i = 1; i < arguments.length; i++)
-        args[i - 1] = arguments[i];
+  if (tickInfo[kHasTickScheduled] === 0) {
+    tickInfo[kHasTickScheduled] = 1;
   }
 
-  if (queue.isEmpty())
-    setHasTickScheduled(true);
   const asyncId = newAsyncId();
   const triggerAsyncId = getDefaultTriggerAsyncId();
   const tickObject = {
     [async_id_symbol]: asyncId,
     [trigger_async_id_symbol]: triggerAsyncId,
     callback,
-    args,
+    args: args.length > 0 ? args : null,
   };
   if (initHooksExist())
     emitInit(asyncId, 'TickObject', triggerAsyncId, tickObject);
   queue.push(tickObject);
 }
 
+function deferTick(callback, ...args) {
+  validateFunction(callback, 'callback');
+
+  if (process._exiting)
+    return;
+
+  if (tickInfo[kHasTickScheduled] === 0) {
+    tickInfo[kHasTickScheduled] = 1;
+  }
+
+  const asyncId = newAsyncId();
+  const triggerAsyncId = getDefaultTriggerAsyncId();
+  const tickObject = {
+    [async_id_symbol]: asyncId,
+    [trigger_async_id_symbol]: triggerAsyncId,
+    callback,
+    args: args.length > 0 ? args : null,
+  };
+  if (initHooksExist())
+    emitInit(asyncId, 'TickObject', triggerAsyncId, tickObject);
+  deferQueue.push(tickObject);
+}
+
 function runMicrotask() {
   this.runInAsyncScope(() => {
     const callback = this.callback;
@@ -166,6 +176,7 @@ module.exports = {
     setTickCallback(processTicksAndRejections);
     return {
       nextTick,
+      deferTick,
       runNextTicks,
     };
   },
diff --git a/test/async-hooks/test-defertick.js b/test/async-hooks/test-defertick.js
new file mode 100644
index 00000000000000..faf29b255238ab
--- /dev/null
+++ b/test/async-hooks/test-defertick.js
@@ -0,0 +1,15 @@
+'use strict';
+
+const common = require('../common');
+const { EventEmitter } = require('events');
+
+setImmediate(async () => {
+  const e = await new Promise((resolve) => {
+    const e = new EventEmitter();
+    resolve(e);
+    process.deferTick(common.mustCall(() => {
+      e.emit('error', new Error('kaboom'));
+    }));
+  });
+  e.on('error', common.mustCall(() => {}));
+});