process: code cleanup for nextTick

Fix a few edge cases and non-obvious issues with nextTick:
1. Emit destroy hook in a try-finally rather than triggering
it before the callback runs.
2. Re-word comment for processPromiseRejections and make sure
it returns true in the rejectionHandled case too.
3. Small readability improvements.

PR-URL: #28047
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Rich Trott <[email protected]>

Author: apapirovski

lib/internal/process/promises.js

@@ -139,17 +139,19 @@ function emitDeprecationWarning() {
   }
 }
 
-// If this method returns true, at least one more tick need to be
-// scheduled to process any potential pending rejections
+// If this method returns true, we've executed user code or triggered
+// a warning to be emitted which requires the microtask and next tick
+// queues to be drained again.
 function processPromiseRejections() {
+  let maybeScheduledTicksOrMicrotasks = asyncHandledRejections.length > 0;
+
   while (asyncHandledRejections.length > 0) {
     const { promise, warning } = asyncHandledRejections.shift();
     if (!process.emit('rejectionHandled', promise)) {
       process.emitWarning(warning);
     }
   }
 
-  let maybeScheduledTicks = false;
   let len = pendingUnhandledRejections.length;
   while (len--) {
     const promise = pendingUnhandledRejections.shift();
@@ -167,9 +169,10 @@ function processPromiseRejections() {
         state === states.warn) {
       emitWarning(uid, reason);
     }
-    maybeScheduledTicks = true;
+    maybeScheduledTicksOrMicrotasks = true;
   }
-  return maybeScheduledTicks || pendingUnhandledRejections.length !== 0;
+  return maybeScheduledTicksOrMicrotasks ||
+         pendingUnhandledRejections.length !== 0;
 }
 
 function getError(name, message) {

lib/internal/process/task_queues.js

@@ -65,46 +65,38 @@ function processTicksAndRejections() {
     while (tock = queue.shift()) {
       const asyncId = tock[async_id_symbol];
       emitBefore(asyncId, tock[trigger_async_id_symbol]);
-      // emitDestroy() places the async_id_symbol into an asynchronous queue
-      // that calls the destroy callback in the future. It's called before
-      // calling tock.callback so destroy will be called even if the callback
-      // throws an exception that is handled by 'uncaughtException' or a
-      // domain.
-      // TODO(trevnorris): This is a bit of a hack. It relies on the fact
-      // that nextTick() doesn't allow the event loop to proceed, but if
-      // any async hooks are enabled during the callback's execution then
-      // this tock's after hook will be called, but not its destroy hook.
-      if (destroyHooksExist())
-        emitDestroy(asyncId);
-
-      const callback = tock.callback;
-      if (tock.args === undefined)
-        callback();
-      else
-        callback(...tock.args);
+
+      try {
+        const callback = tock.callback;
+        if (tock.args === undefined)
+          callback();
+        else
+          callback(...tock.args);
+      } finally {
+        if (destroyHooksExist())
+          emitDestroy(asyncId);
+      }
 
       emitAfter(asyncId);
     }
-    setHasTickScheduled(false);
     runMicrotasks();
   } while (!queue.isEmpty() || processPromiseRejections());
+  setHasTickScheduled(false);
   setHasRejectionToWarn(false);
 }
 
 class TickObject {
-  constructor(callback, args, triggerAsyncId) {
+  constructor(callback, args) {
     this.callback = callback;
     this.args = args;
 
     const asyncId = newAsyncId();
+    const triggerAsyncId = getDefaultTriggerAsyncId();
     this[async_id_symbol] = asyncId;
     this[trigger_async_id_symbol] = triggerAsyncId;
 
     if (initHooksExist()) {
-      emitInit(asyncId,
-               'TickObject',
-               triggerAsyncId,
-               this);
+      emitInit(asyncId, 'TickObject', triggerAsyncId, this);
     }
   }
 }
@@ -132,7 +124,7 @@ function nextTick(callback) {
 
   if (queue.isEmpty())
     setHasTickScheduled(true);
-  queue.push(new TickObject(callback, args, getDefaultTriggerAsyncId()));
+  queue.push(new TickObject(callback, args));
 }
 
 let AsyncResource;

test/async-hooks/test-late-hook-enable.js

@@ -0,0 +1,49 @@
+'use strict';
+const common = require('../common');
+const assert = require('assert');
+const async_hooks = require('async_hooks');
+
+// Checks that enabling async hooks in a callback actually
+// triggers after & destroy as expected.
+
+const fnsToTest = [setTimeout, (cb) => {
+  setImmediate(() => {
+    cb();
+
+    // We need to keep the event loop open for this to actually work
+    // since destroy hooks are triggered in unrefed Immediates
+    setImmediate(() => {
+      hook.disable();
+    });
+  });
+}, (cb) => {
+  process.nextTick(() => {
+    cb();
+
+    // We need to keep the event loop open for this to actually work
+    // since destroy hooks are triggered in unrefed Immediates
+    setImmediate(() => {
+      hook.disable();
+      assert.strictEqual(fnsToTest.length, 0);
+    });
+  });
+}];
+
+const hook = async_hooks.createHook({
+  before: common.mustNotCall(),
+  after: common.mustCall(() => {}, 3),
+  destroy: common.mustCall(() => {
+    hook.disable();
+    nextTest();
+  }, 3)
+});
+
+nextTest();
+
+function nextTest() {
+  if (fnsToTest.length > 0) {
+    fnsToTest.shift()(common.mustCall(() => {
+      hook.enable();
+    }));
+  }
+}