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.
 functionprocessPromiseRejections(){
+letmaybeScheduledTicksOrMicrotasks=asyncHandledRejections.length>0;
+
 while(asyncHandledRejections.length>0){
 const{ promise, warning }=asyncHandledRejections.shift();
 if(!process.emit('rejectionHandled',promise)){
 process.emitWarning(warning);
 }
 }
 
-letmaybeScheduledTicks=false;
 letlen=pendingUnhandledRejections.length;
 while(len--){
 constpromise=pendingUnhandledRejections.shift();
@@ -167,9 +169,10 @@ function processPromiseRejections(){
 state===states.warn){
 emitWarning(uid,reason);
 }
-maybeScheduledTicks=true;
+maybeScheduledTicksOrMicrotasks=true;
 }
-returnmaybeScheduledTicks||pendingUnhandledRejections.length!==0;
+returnmaybeScheduledTicksOrMicrotasks||
+pendingUnhandledRejections.length!==0;
 }
 
 functiongetError(name,message){

lib/internal/process/task_queues.js

@@ -65,46 +65,38 @@ function processTicksAndRejections(){
 while(tock=queue.shift()){
 constasyncId=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);
-
-constcallback=tock.callback;
-if(tock.args===undefined)
-callback();
-else
-callback(...tock.args);
+
+try{
+constcallback=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);
 }
 
 classTickObject{
-constructor(callback,args,triggerAsyncId){
+constructor(callback,args){
 this.callback=callback;
 this.args=args;
 
 constasyncId=newAsyncId();
+consttriggerAsyncId=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(newTickObject(callback,args,getDefaultTriggerAsyncId()));
+queue.push(newTickObject(callback,args));
 }
 
 letAsyncResource;

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

@@ -0,0 +1,49 @@
+'use strict';
+constcommon=require('../common');
+constassert=require('assert');
+constasync_hooks=require('async_hooks');
+
+// Checks that enabling async hooks in a callback actually
+// triggers after & destroy as expected.
+
+constfnsToTest=[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);
+});
+});
+}];
+
+consthook=async_hooks.createHook({
+before: common.mustNotCall(),
+after: common.mustCall(()=>{},3),
+destroy: common.mustCall(()=>{
+hook.disable();
+nextTest();
+},3)
+});
+
+nextTest();
+
+functionnextTest(){
+if(fnsToTest.length>0){
+fnsToTest.shift()(common.mustCall(()=>{
+hook.enable();
+}));
+}
+}