From 9e07efed0de26fc32e5be14a15f53dabbf3f7c14 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 | 76 +++++++++++++++--------------
 test/async-hooks/test-defertick.js  | 15 ++++++
 4 files changed, 125 insertions(+), 37 deletions(-)
 create mode 100644 test/async-hooks/test-defertick.js

diff --git a/doc/api/process.md b/doc/api/process.md
index b53986f04ae18c..af09227fc721ff 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', () => {}); // e.emit executes before we reach this...
+});
+
+// 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', () => {}); // e.emit executes before we reach this...
+});
+```
+
+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', () => {}); // e.emit executes *after* we reach this.
+});
+```
+
 ## `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..38bb8d284e6645 100644
--- a/lib/internal/process/task_queues.js
+++ b/lib/internal/process/task_queues.js
@@ -1,7 +1,6 @@
 'use strict';
 
 const {
-  Array,
   FunctionPrototypeBind,
 } = primordials;
 
@@ -44,21 +43,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();
@@ -72,18 +64,12 @@ function processTicksAndRejections() {
       emitBefore(asyncId, tock[trigger_async_id_symbol], tock);
 
       try {
+        const args = tock.args;
         const callback = tock.callback;
-        if (tock.args === undefined) {
+        if (args === undefined) {
           callback();
         } else {
-          const args = tock.args;
-          switch (args.length) {
-            case 1: callback(args[0]); break;
-            case 2: callback(args[0], args[1]); break;
-            case 3: callback(args[0], args[1], args[2]); break;
-            case 4: callback(args[0], args[1], args[2], args[3]); break;
-            default: callback(...args);
-          }
+          callback(...args);
         }
       } finally {
         if (destroyHooksExist())
@@ -93,46 +79,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 : undefined,
   };
   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 : undefined,
+  };
+  if (initHooksExist())
+    emitInit(asyncId, 'TickObject', triggerAsyncId, tickObject);
+  deferQueue.push(tickObject);
+}
+
 function runMicrotask() {
   this.runInAsyncScope(() => {
     const callback = this.callback;
@@ -166,6 +169,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..2f884bf5202155
--- /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());
+});