Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

async_hooks: fix default nextTick triggerAsyncId #14026

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions lib/async_hooks.js
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}

Expand Down
9 changes: 5 additions & 4 deletions lib/internal/process/next_tick.js
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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;
Expand All @@ -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;
}
}
26 changes: 19 additions & 7 deletions test/async-hooks/init-hooks.js
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down Expand Up @@ -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`);
}
}

Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: If you're already here, could you turn this comment to a proper paragraph (capitalization, some punctuation, etc)

// later
if (this._allowNoInit) {
const stub = { uid, type: 'Unknown' };
const stub = { uid, type: 'Unknown', handleIsObject: true };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: maybe just isObject? (here and everywhere, obviusly)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would much rather be explicit, so many things could be an object.

this._activities.set(uid, stub);
return stub;
} else {
Expand All @@ -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');
Expand Down
2 changes: 1 addition & 1 deletion test/async-hooks/test-emit-init.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,4 +46,4 @@ initHooks({
})
}).enable();

async_hooks.emitInit(expectedId, expectedType, expectedResource);
async_hooks.emitInit(expectedId, expectedType, null, expectedResource);
47 changes: 47 additions & 0 deletions test/async-hooks/test-internal-nexttick-default-trigger.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
'use strict';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: style as in https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure (and but the //Flags on the first line 🤷‍♂️

// 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');
});