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 2 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
6 changes: 3 additions & 3 deletions lib/internal/process/next_tick.js
Original file line number Diff line number Diff line change
Expand Up @@ -315,13 +315,13 @@ function setupNextTick() {
}

const asyncId = ++async_uid_fields[kAsyncUidCntr];
if (triggerAsyncId === null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Since it's argument overload handling, could you move it up, probably just after the if (process._exiting) return

triggerAsyncId = async_hooks.initTriggerId();
}
const obj = new TickObject(callback, args, asyncId, triggerAsyncId);
nextTickQueue.push(obj);
++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;
}
}
20 changes: 16 additions & 4 deletions test/async-hooks/init-hooks.js
Original file line number Diff line number Diff line change
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 @@ -147,7 +152,7 @@ class ActivityCollector {
// 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 (Timeout) the handle is a function, thus the usual
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: (I don't really mind but) start the sentence with a capital I. Also (e.g. `Timeout`)

// `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);
37 changes: 37 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,37 @@
'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');
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');
});