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 rename #14152

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
57 changes: 23 additions & 34 deletions lib/async_hooks.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ const emitDestroyNative = emitHookFactory(destroy_symbol, 'emitDestroyNative');
// process. They use the same functions as the JS embedder API. These callbacks
// are setup immediately to prevent async_wrap.setupHooks() from being hijacked
// and the cost of doing so is negligible.
async_wrap.setupHooks({ init,
async_wrap.setupHooks({ init: emitInitNative,
before: emitBeforeNative,
after: emitAfterNative,
destroy: emitDestroyNative });
Expand Down Expand Up @@ -207,42 +207,30 @@ function triggerAsyncId() {
// Embedder API //

class AsyncResource {
constructor(type, triggerAsyncId) {
this[async_id_symbol] = ++async_uid_fields[kAsyncUidCntr];
// Read and reset the current kInitTriggerId so that when the constructor
// finishes the kInitTriggerId field is always 0.
if (triggerAsyncId === undefined) {
triggerAsyncId = initTriggerId();
// If a triggerAsyncId was passed, any kInitTriggerId still must be null'd.
} else {
async_uid_fields[kInitTriggerId] = 0;
}
this[trigger_id_symbol] = triggerAsyncId;

if (typeof type !== 'string' || type.length <= 0)
throw new TypeError('type must be a string with length > 0');
constructor(type, triggerAsyncId = initTriggerId()) {
Copy link
Contributor

@refack refack Jul 10, 2017

Choose a reason for hiding this comment

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

So just an FYI since this effectively changes the signature of the ctor (now AsyncResource.length == 1 it would have been some do consider it semver-major, but since this congruent with the docs, I'd call this a bug fix.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also this is still officially experimental, which should give us more flexibility.

// Unlike emitInitScript, AsyncResource doesn't supports null as the
// triggerAsyncId.
if (!Number.isSafeInteger(triggerAsyncId) || triggerAsyncId < 0)
throw new RangeError('triggerAsyncId must be an unsigned integer');

// Return immediately if there's nothing to do.
if (async_hook_fields[kInit] === 0)
return;
this[async_id_symbol] = ++async_uid_fields[kAsyncUidCntr];
this[trigger_id_symbol] = triggerAsyncId;

init(this[async_id_symbol], type, triggerAsyncId, this);
emitInitScript(this[async_id_symbol], type, this[trigger_id_symbol], this);
}

emitBefore() {
emitBeforeS(this[async_id_symbol], this[trigger_id_symbol]);
emitBeforeScript(this[async_id_symbol], this[trigger_id_symbol]);
return this;
}

emitAfter() {
emitAfterS(this[async_id_symbol]);
emitAfterScript(this[async_id_symbol]);
return this;
}

emitDestroy() {
emitDestroyS(this[async_id_symbol]);
emitDestroyScript(this[async_id_symbol]);
return this;
}

Expand Down Expand Up @@ -311,7 +299,7 @@ function setInitTriggerId(triggerAsyncId) {
}


function emitInitS(asyncId, type, triggerAsyncId, resource) {
function emitInitScript(asyncId, type, triggerAsyncId, resource) {
// Short circuit all checks for the common case. Which is that no hooks have
// been set. Do this to remove performance impact for embedders (and core).
// Even though it bypasses all the argument checks. The performance savings
Expand All @@ -323,6 +311,9 @@ function emitInitS(asyncId, type, triggerAsyncId, resource) {
// manually means that the embedder must have used initTriggerId().
if (triggerAsyncId === null) {
triggerAsyncId = initTriggerId();
} else {
// If a triggerAsyncId was passed, any kInitTriggerId still must be null'd.
async_uid_fields[kInitTriggerId] = 0;
}

// TODO(trevnorris): I'd prefer allowing these checks to not exist, or only
Expand All @@ -334,7 +325,7 @@ function emitInitS(asyncId, type, triggerAsyncId, resource) {
if (!Number.isSafeInteger(triggerAsyncId) || triggerAsyncId < 0)
throw new RangeError('triggerAsyncId must be an unsigned integer');

init(asyncId, type, triggerAsyncId, resource);
emitInitNative(asyncId, type, triggerAsyncId, resource);
}

function emitHookFactory(symbol, name) {
Expand Down Expand Up @@ -370,9 +361,7 @@ function emitHookFactory(symbol, name) {
}


// Usage: emitBeforeS(asyncId[, triggerAsyncId]). If triggerAsyncId is omitted
// then asyncId will be used instead.
function emitBeforeS(asyncId, triggerAsyncId) {
function emitBeforeScript(asyncId, triggerAsyncId) {
// CHECK(Number.isSafeInteger(asyncId) && asyncId > 0)
// CHECK(Number.isSafeInteger(triggerAsyncId) && triggerAsyncId > 0)

Expand All @@ -392,15 +381,15 @@ function emitBeforeS(asyncId, triggerAsyncId) {
// TODO(trevnorris): Calling emitBefore/emitAfter from native can't adjust the
// kIdStackIndex. But what happens if the user doesn't have both before and
// after callbacks.
function emitAfterS(asyncId) {
function emitAfterScript(asyncId) {
if (async_hook_fields[kAfter] > 0)
emitAfterNative(asyncId);

popAsyncIds(asyncId);
}


function emitDestroyS(asyncId) {
function emitDestroyScript(asyncId) {
// Return early if there are no destroy callbacks, or on attempt to emit
// destroy on the void.
if (async_hook_fields[kDestroy] === 0 || asyncId === 0)
Expand All @@ -422,7 +411,7 @@ function emitDestroyS(asyncId) {
// change in the future depending on whether it can be determined if there's a
// slim chance of the application remaining stable after handling one of these
// exceptions.
function init(asyncId, type, triggerAsyncId, resource) {
function emitInitNative(asyncId, type, triggerAsyncId, resource) {
processing_hook += 1;
// Use a single try/catch for all hook to avoid setting up one per iteration.
try {
Expand Down Expand Up @@ -467,10 +456,10 @@ module.exports = {
newUid,
initTriggerId,
setInitTriggerId,
emitInit: emitInitS,
emitBefore: emitBeforeS,
emitAfter: emitAfterS,
emitDestroy: emitDestroyS,
emitInit: emitInitScript,
emitBefore: emitBeforeScript,
emitAfter: emitAfterScript,
emitDestroy: emitDestroyScript,
};

// currentId was renamed to executionAsyncId. This was in 8.2.0 during the
Expand Down
4 changes: 2 additions & 2 deletions test/async-hooks/test-embedder.api.async-resource.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,8 @@ assert.throws(() => new AsyncResource('invalid_trigger_id', null),
/^RangeError: triggerAsyncId must be an unsigned integer$/);

assert.strictEqual(
typeof new AsyncResource('default_trigger_id').triggerAsyncId(),
Copy link
Member Author

Choose a reason for hiding this comment

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

This change wasn't needed. But when I looked for the AsyncResource('default_trigger_id') test, I found that it could be made more strict.

'number'
new AsyncResource('default_trigger_id').triggerAsyncId(),
async_hooks.executionAsyncId()
);

Copy link
Contributor

@refack refack Jul 10, 2017

Choose a reason for hiding this comment

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

Could you add assert.strinctEqual(AsyncResource.length, 1);
Retracted

// create first custom event 'alcazares' with triggerAsyncId derived
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,13 @@ require('../common');
// This tests that AsyncResource throws an error if bad parameters are passed

const assert = require('assert');
const AsyncResource = require('async_hooks').AsyncResource;
const async_hooks = require('async_hooks');
const { AsyncResource } = async_hooks;

// Setup init hook such parameters are validated
async_hooks.createHook({
init() {}
}).enable();

assert.throws(() => {
return new AsyncResource();
Expand Down