process: code cleanup for nextTick

apapirovski · apapirovski · commit 1a66c0dd41cb · 2019-06-04T07:51:57.000+02:00
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.
diff --git a/lib/internal/process/promises.js b/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) {
diff --git a/lib/internal/process/task_queues.js b/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;

Original file line number	Diff line number	Diff line change
`@@ -139,17 +139,19 @@ function emitDeprecationWarning() {`
`139`	`139`	`}`
`140`	`140`	`}`
`141`	`141`
`142`		`-// If this method returns true, at least one more tick need to be`
`143`		`-// scheduled to process any potential pending rejections`
	`142`	`+// If this method returns true, we've executed user code or triggered`
	`143`	`+// a warning to be emitted which requires the microtask and next tick`
	`144`	`+// queues to be drained again.`
`144`	`145`	`function processPromiseRejections() {`
	`146`	`+ let maybeScheduledTicksOrMicrotasks = asyncHandledRejections.length > 0;`
	`147`	`+`
`145`	`148`	`while (asyncHandledRejections.length > 0) {`
`146`	`149`	`const { promise, warning } = asyncHandledRejections.shift();`
`147`	`150`	`if (!process.emit('rejectionHandled', promise)) {`
`148`	`151`	`process.emitWarning(warning);`
`149`	`152`	`}`
`150`	`153`	`}`
`151`	`154`
`152`		`- let maybeScheduledTicks = false;`
`153`	`155`	`let len = pendingUnhandledRejections.length;`
`154`	`156`	`while (len--) {`
`155`	`157`	`const promise = pendingUnhandledRejections.shift();`
`@@ -167,9 +169,10 @@ function processPromiseRejections() {`
`167`	`169`	`state === states.warn) {`
`168`	`170`	`emitWarning(uid, reason);`
`169`	`171`	`}`
`170`		`- maybeScheduledTicks = true;`
	`172`	`+ maybeScheduledTicksOrMicrotasks = true;`
`171`	`173`	`}`
`172`		`- return maybeScheduledTicks \|\| pendingUnhandledRejections.length !== 0;`
	`174`	`+ return maybeScheduledTicksOrMicrotasks \|\|`
	`175`	`+ pendingUnhandledRejections.length !== 0;`
`173`	`176`	`}`
`174`	`177`
`175`	`178`	`function getError(name, message) {`