async_hooks: fix default nextTick triggerAsyncId

AndreasMadsen · addaleax · commit 7b369d12cfa6 · 2017-07-18T19:58:08.000+02:00
In the case where triggerAsyncId is null it should default to the current executionAsyncId. This worked but as a side-effect the resource object was changed too. This fix also makes the null check more strict. EmitInitS is not a documented API, thus there is no reason to be flexible in its input. Ref: #13548 (comment) PR-URL: #14026 Reviewed-By: Refael Ackermann <refack@gmail.com>
diff --git a/lib/async_hooks.js b/lib/async_hooks.js
@@ -322,9 +322,7 @@ function emitInitS(asyncId, type, triggerAsyncId, resource) {
 
   // This can run after the early return check b/c running this function
   // manually means that the embedder must have used initTriggerId().
-  if (!Number.isSafeInteger(triggerAsyncId)) {
-    if (triggerAsyncId !== undefined)
-      resource = triggerAsyncId;
+  if (triggerAsyncId === null) {
     triggerAsyncId = initTriggerId();
   }
 
diff --git a/lib/internal/process/next_tick.js b/lib/internal/process/next_tick.js
@@ -60,7 +60,7 @@ function setupNextTick() {
   // The needed emit*() functions.
   const { emitInit, emitBefore, emitAfter, emitDestroy } = async_hooks;
   // Grab the constants necessary for working with internal arrays.
-  const { kInit, kBefore, kAfter, kDestroy, kAsyncUidCntr, kInitTriggerId } =
+  const { kInit, kBefore, kAfter, kDestroy, kAsyncUidCntr } =
       async_wrap.constants;
   const { async_id_symbol, trigger_id_symbol } = async_wrap;
   var nextTickQueue = new NextTickQueue();
@@ -302,6 +302,10 @@ function setupNextTick() {
     if (process._exiting)
       return;
 
+    if (triggerAsyncId === null) {
+      triggerAsyncId = async_hooks.initTriggerId();
+    }
+
     var args;
     switch (arguments.length) {
       case 2: break;
@@ -320,8 +324,5 @@ function setupNextTick() {
     ++tickInfo[kLength];
     if (async_hook_fields[kInit] > 0)
       emitInit(asyncId, 'TickObject', triggerAsyncId, obj);
-
-    // The call to initTriggerId() was skipped, so clear kInitTriggerId.
-    async_uid_fields[kInitTriggerId] = 0;
   }
 }
diff --git a/test/async-hooks/init-hooks.js b/test/async-hooks/init-hooks.js
@@ -34,13 +34,13 @@ class ActivityCollector {
     this._logid = logid;
     this._logtype = logtype;
 
-    // register event handlers if provided
+    // Register event handlers if provided
     this.oninit = typeof oninit === 'function' ? oninit : noop;
     this.onbefore = typeof onbefore === 'function' ? onbefore : noop;
     this.onafter = typeof onafter === 'function' ? onafter : noop;
     this.ondestroy = typeof ondestroy === 'function' ? ondestroy : noop;
 
-    // create the hook with which we'll collect activity data
+    // Create the hook with which we'll collect activity data
     this._asyncHook = async_hooks.createHook({
       init: this._init.bind(this),
       before: this._before.bind(this),
@@ -106,10 +106,15 @@ class ActivityCollector {
             '\nExpected "destroy" to be called after "after"');
         }
       }
+      if (!a.handleIsObject) {
+        v('No resource object\n' + activityString(a) +
+          '\nExpected "init" to be called with a resource object');
+      }
     }
     if (violations.length) {
-      console.error(violations.join('\n'));
-      assert.fail(violations.length, 0, `Failed sanity checks: ${violations}`);
+      console.error(violations.join('\n\n') + '\n');
+      assert.fail(violations.length, 0,
+                  `${violations.length} failed sanity checks`);
     }
   }
 
@@ -143,11 +148,11 @@ class ActivityCollector {
   _getActivity(uid, hook) {
     const h = this._activities.get(uid);
     if (!h) {
-      // if we allowed handles without init we ignore any further life time
+      // If we allowed handles without init we ignore any further life time
       // events this makes sense for a few tests in which we enable some hooks
       // later
       if (this._allowNoInit) {
-        const stub = { uid, type: 'Unknown' };
+        const stub = { uid, type: 'Unknown', handleIsObject: true };
         this._activities.set(uid, stub);
         return stub;
       } else {
@@ -163,7 +168,14 @@ class ActivityCollector {
   }
 
   _init(uid, type, triggerAsyncId, handle) {
-    const activity = { uid, type, triggerAsyncId };
+    const activity = {
+      uid,
+      type,
+      triggerAsyncId,
+      // In some cases (e.g. Timeout) the handle is a function, thus the usual
+      // `typeof handle === 'object' && handle !== null` check can't be used.
+      handleIsObject: handle instanceof Object
+    };
     this._stamp(activity, 'init');
     this._activities.set(uid, activity);
     this._maybeLog(uid, type, 'init');
diff --git a/test/async-hooks/test-emit-init.js b/test/async-hooks/test-emit-init.js
@@ -46,4 +46,4 @@ initHooks({
   })
 }).enable();
 
-async_hooks.emitInit(expectedId, expectedType, expectedResource);
+async_hooks.emitInit(expectedId, expectedType, null, expectedResource);
diff --git a/test/async-hooks/test-internal-nexttick-default-trigger.js b/test/async-hooks/test-internal-nexttick-default-trigger.js
@@ -0,0 +1,47 @@
+'use strict';
+// Flags: --expose-internals
+const common = require('../common');
+
+// This tests ensures that the triggerId of both the internal and external
+// nexTick function sets the triggerAsyncId correctly.
+
+const assert = require('assert');
+const async_hooks = require('async_hooks');
+const initHooks = require('./init-hooks');
+const { checkInvocations } = require('./hook-checks');
+const internal = require('internal/process/next_tick');
+
+const hooks = initHooks();
+hooks.enable();
+
+const rootAsyncId = async_hooks.executionAsyncId();
+
+// public
+process.nextTick(common.mustCall(function() {
+  assert.strictEqual(async_hooks.triggerAsyncId(), rootAsyncId);
+}));
+
+// internal default
+internal.nextTick(null, common.mustCall(function() {
+  assert.strictEqual(async_hooks.triggerAsyncId(), rootAsyncId);
+}));
+
+// internal
+internal.nextTick(rootAsyncId + 1, common.mustCall(function() {
+  assert.strictEqual(async_hooks.triggerAsyncId(), rootAsyncId + 1);
+}));
+
+process.on('exit', function() {
+  hooks.sanityCheck();
+
+  const as = hooks.activitiesOfTypes('TickObject');
+  checkInvocations(as[0], {
+      init: 1, before: 1, after: 1, destroy: 1
+  }, 'when process exits');
+  checkInvocations(as[1], {
+      init: 1, before: 1, after: 1, destroy: 1
+  }, 'when process exits');
+  checkInvocations(as[2], {
+      init: 1, before: 1, after: 1, destroy: 1
+  }, 'when process exits');
+});

Original file line number	Diff line number	Diff line change
`@@ -322,9 +322,7 @@ function emitInitS(asyncId, type, triggerAsyncId, resource) {`
`322`	`322`
`323`	`323`	`// This can run after the early return check b/c running this function`
`324`	`324`	`// manually means that the embedder must have used initTriggerId().`
`325`		`- if (!Number.isSafeInteger(triggerAsyncId)) {`
`326`		`- if (triggerAsyncId !== undefined)`
`327`		`- resource = triggerAsyncId;`
	`325`	`+ if (triggerAsyncId === null) {`
`328`	`326`	`triggerAsyncId = initTriggerId();`
`329`	`327`	`}`
`330`	`328`