From 74329ca64807c35f751383498e298fdd89e19b68 Mon Sep 17 00:00:00 2001 From: Andreas Madsen Date: Wed, 5 Jul 2017 15:01:18 +0200 Subject: [PATCH] async_hooks: fix default nextTick triggerAsyncId 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: https://github.com/nodejs/node/issues/13548#issuecomment-310985270 PR-URL: https://github.com/nodejs/node/pull/14026 Reviewed-By: Refael Ackermann --- lib/async_hooks.js | 4 +- lib/internal/process/next_tick.js | 9 ++-- test/async-hooks/init-hooks.js | 26 +++++++--- test/async-hooks/test-emit-init.js | 2 +- .../test-internal-nexttick-default-trigger.js | 47 +++++++++++++++++++ 5 files changed, 73 insertions(+), 15 deletions(-) create mode 100644 test/async-hooks/test-internal-nexttick-default-trigger.js diff --git a/lib/async_hooks.js b/lib/async_hooks.js index d5c7c116434c0b..02cfce0262ca64 100644 --- 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 index 418f4ff8816d3f..5b41c4ba09ebcf 100644 --- 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 index 69656fdb2c4255..045639945c564d 100644 --- 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 index e0a28d50c930d3..feee3d944b8afb 100644 --- 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 new file mode 100644 index 00000000000000..90e566b7063c46 --- /dev/null +++ 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'); +});