From a5aacec70c15605bf2909296e8b189297a5cad79 Mon Sep 17 00:00:00 2001 From: Andreas Madsen Date: Sat, 1 Jul 2017 14:07:59 +0200 Subject: [PATCH 1/4] 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. --- lib/async_hooks.js | 4 +- lib/internal/process/next_tick.js | 3 ++ test/async-hooks/init-hooks.js | 20 ++++++++-- test/async-hooks/test-emit-init.js | 2 +- .../test-internal-nexttick-default-trigger.js | 37 +++++++++++++++++++ 5 files changed, 58 insertions(+), 8 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 e1029c97a57eec..fe2751b8b58bbb 100644 --- a/lib/async_hooks.js +++ b/lib/async_hooks.js @@ -319,9 +319,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 e41064104e0c3e..e943da7a970b45 100644 --- a/lib/internal/process/next_tick.js +++ b/lib/internal/process/next_tick.js @@ -315,6 +315,9 @@ function setupNextTick() { } const asyncId = ++async_uid_fields[kAsyncUidCntr]; + if (triggerAsyncId === null) { + triggerAsyncId = async_hooks.initTriggerId(); + } const obj = new TickObject(callback, args, asyncId, triggerAsyncId); nextTickQueue.push(obj); ++tickInfo[kLength]; diff --git a/test/async-hooks/init-hooks.js b/test/async-hooks/init-hooks.js index 69656fdb2c4255..012fa7f7501527 100644 --- a/test/async-hooks/init-hooks.js +++ b/test/async-hooks/init-hooks.js @@ -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`); } } @@ -147,7 +152,7 @@ class ActivityCollector { // 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 (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..f70aff2465107b --- /dev/null +++ b/test/async-hooks/test-internal-nexttick-default-trigger.js @@ -0,0 +1,37 @@ +'use strict'; + +// Flags: --expose-internals + +const common = require('../common'); +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 +internal.nextTick(null, common.mustCall(function() { + assert.strictEqual(async_hooks.triggerAsyncId(), rootAsyncId); +})); + +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'); +}); From 836cc683e85ce054ee54065d0cbd1e190d8d353d Mon Sep 17 00:00:00 2001 From: Andreas Madsen Date: Sun, 2 Jul 2017 16:00:12 +0200 Subject: [PATCH 2/4] [squash] remove redundant initTriggerId reset --- lib/internal/process/next_tick.js | 3 --- 1 file changed, 3 deletions(-) diff --git a/lib/internal/process/next_tick.js b/lib/internal/process/next_tick.js index e943da7a970b45..3c1bd147708efc 100644 --- a/lib/internal/process/next_tick.js +++ b/lib/internal/process/next_tick.js @@ -323,8 +323,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; } } From 619b98ae8892b045ae2fe340b43c33db57c7b88f Mon Sep 17 00:00:00 2001 From: Andreas Madsen Date: Mon, 3 Jul 2017 09:32:53 +0200 Subject: [PATCH 3/4] [squash] refack suggestions --- lib/internal/process/next_tick.js | 7 ++++--- test/async-hooks/init-hooks.js | 8 ++++---- .../test-internal-nexttick-default-trigger.js | 16 +++++++++++++--- 3 files changed, 21 insertions(+), 10 deletions(-) diff --git a/lib/internal/process/next_tick.js b/lib/internal/process/next_tick.js index 3c1bd147708efc..e740030f4e5e8f 100644 --- a/lib/internal/process/next_tick.js +++ b/lib/internal/process/next_tick.js @@ -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; @@ -315,9 +319,6 @@ function setupNextTick() { } const asyncId = ++async_uid_fields[kAsyncUidCntr]; - if (triggerAsyncId === null) { - triggerAsyncId = async_hooks.initTriggerId(); - } const obj = new TickObject(callback, args, asyncId, triggerAsyncId); nextTickQueue.push(obj); ++tickInfo[kLength]; diff --git a/test/async-hooks/init-hooks.js b/test/async-hooks/init-hooks.js index 012fa7f7501527..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), @@ -148,7 +148,7 @@ 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) { @@ -172,7 +172,7 @@ class ActivityCollector { uid, type, triggerAsyncId, - // in some cases (Timeout) the handle is a function, thus the usual + // 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 }; diff --git a/test/async-hooks/test-internal-nexttick-default-trigger.js b/test/async-hooks/test-internal-nexttick-default-trigger.js index f70aff2465107b..90e566b7063c46 100644 --- a/test/async-hooks/test-internal-nexttick-default-trigger.js +++ b/test/async-hooks/test-internal-nexttick-default-trigger.js @@ -1,8 +1,10 @@ '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'); @@ -19,11 +21,16 @@ process.nextTick(common.mustCall(function() { assert.strictEqual(async_hooks.triggerAsyncId(), rootAsyncId); })); -// internal +// 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(); @@ -34,4 +41,7 @@ process.on('exit', function() { 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'); }); From 41b63e6595005f1974e9d7bd7e842b5ed03d6d26 Mon Sep 17 00:00:00 2001 From: Andreas Madsen Date: Mon, 3 Jul 2017 12:08:44 +0200 Subject: [PATCH 4/4] [squash] remove unused variable --- lib/internal/process/next_tick.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/internal/process/next_tick.js b/lib/internal/process/next_tick.js index e740030f4e5e8f..96a70e5c310d33 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();