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.

Sign up for GitHub

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: minor refactor to callback invocation #13419

Closed
wants to merge 1 commit into from
Closed

async_hooks: minor refactor to callback invocation #13419

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
103 changes: 41 additions & 62 deletions lib/async_hooks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,17 +214,7 @@ class AsyncResource {
if (async_hook_fields[kInit] === 0)
return;

processing_hook = true;
for (var i = 0; i < active_hooks_array.length; i++) {
if (typeof active_hooks_array[i][init_symbol] === 'function') {
runInitCallback(active_hooks_array[i][init_symbol],
this[async_id_symbol],
type,
triggerId,
this);
}
}
processing_hook = false;
init(this[async_id_symbol], type, triggerId, this);
}

emitBefore() {
Expand Down Expand Up @@ -321,14 +311,7 @@ function emitInitS(asyncId, type, triggerId, resource) {
if (!Number.isSafeInteger(triggerId) || triggerId < 0)
throw new RangeError('triggerId must be an unsigned integer');

processing_hook = true;
for (var i = 0; i < active_hooks_array.length; i++) {
if (typeof active_hooks_array[i][init_symbol] === 'function') {
runInitCallback(
active_hooks_array[i][init_symbol], asyncId, type, triggerId, resource);
}
}
processing_hook = false;
init(asyncId, type, triggerId, resource);

// Isn't null if hooks were added/removed while the hooks were running.
if (tmp_active_hooks_array !== null) {
Expand All @@ -339,10 +322,15 @@ function emitInitS(asyncId, type, triggerId, resource) {

function emitBeforeN(asyncId) {
processing_hook = true;
for (var i = 0; i < active_hooks_array.length; i++) {
if (typeof active_hooks_array[i][before_symbol] === 'function') {
runCallback(active_hooks_array[i][before_symbol], asyncId);
// Use a single try/catch for all hook to avoid setting up one per iteration.
try {
for (var i = 0; i < active_hooks_array.length; i++) {
if (typeof active_hooks_array[i][before_symbol] === 'function') {
active_hooks_array[i][before_symbol](asyncId);
}
}
} catch (e) {
fatalError(e);
}
processing_hook = false;

Expand All @@ -366,26 +354,27 @@ function emitBeforeS(asyncId, triggerId = asyncId) {

pushAsyncIds(asyncId, triggerId);

if (async_hook_fields[kBefore] === 0) {
if (async_hook_fields[kBefore] === 0)
return;
}

emitBeforeN(asyncId);
}


// Called from native. The asyncId stack handling is taken care of there before
// this is called.
function emitAfterN(asyncId) {
if (async_hook_fields[kAfter] > 0) {
processing_hook = true;
processing_hook = true;
Copy link
Member

Choose a reason for hiding this comment

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

Why was the if (async_hook_fields[kAfter] > 0) short circuit removed?

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’ve added it to emitAfterS now; The idea is that the native code doesn’t bother calling emitAfterN if the field is 0, so checking it here is pointless for the common case of a call from C++.

Copy link
Contributor

Choose a reason for hiding this comment

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

Note for future change: I'd like to include a macros.py similar to deps/v8/src/js/macros.py for Debug builds. except it replaces comments instead of normal expressions.

In the code you'll find // CHECK() comments that were meant as reminders of important bits to check, but not important enough to check in a Release build. Though the difference would be that

// CHECK(expr)

is replaced with something like

assert.ok(expr);

But only for debug builds.

Point here would be to include the following here:

// CHECK(async_hook_fields[kAfter] > 0)

as a sanity check for the future in case a change is made that invalidates your comment of

The idea is that the native code doesn’t bother calling emitAfterN if the field is 0, so checking it here is pointless for the common case of a call from C++.

// Use a single try/catch for all hook to avoid setting up one per iteration.
try {
for (var i = 0; i < active_hooks_array.length; i++) {
if (typeof active_hooks_array[i][after_symbol] === 'function') {
runCallback(active_hooks_array[i][after_symbol], asyncId);
active_hooks_array[i][after_symbol](asyncId);
}
}
processing_hook = false;
} catch (e) {
fatalError(e);
}
processing_hook = false;

if (tmp_active_hooks_array !== null) {
restoreTmpHooks();
Expand All @@ -397,7 +386,9 @@ function emitAfterN(asyncId) {
// kIdStackIndex. But what happens if the user doesn't have both before and
// after callbacks.
function emitAfterS(asyncId) {
emitAfterN(asyncId);
if (async_hook_fields[kAfter] > 0)
emitAfterN(asyncId);

popAsyncIds(asyncId);
}

Expand All @@ -413,10 +404,15 @@ function emitDestroyS(asyncId) {

function emitDestroyN(asyncId) {
processing_hook = true;
for (var i = 0; i < active_hooks_array.length; i++) {
if (typeof active_hooks_array[i][destroy_symbol] === 'function') {
runCallback(active_hooks_array[i][destroy_symbol], asyncId);
// Use a single try/catch for all hook to avoid setting up one per iteration.
try {
for (var i = 0; i < active_hooks_array.length; i++) {
if (typeof active_hooks_array[i][destroy_symbol] === 'function') {
active_hooks_array[i][destroy_symbol](asyncId);
}
}
} catch (e) {
fatalError(e);
}
processing_hook = false;

Expand All @@ -434,41 +430,24 @@ function emitDestroyN(asyncId) {
// init().
// TODO(trevnorris): Perhaps have MakeCallback call a single JS function that
// does the before/callback/after calls to remove two additional calls to JS.
function init(asyncId, type, resource, triggerId) {
processing_hook = true;
for (var i = 0; i < active_hooks_array.length; i++) {
if (typeof active_hooks_array[i][init_symbol] === 'function') {
runInitCallback(
active_hooks_array[i][init_symbol], asyncId, type, triggerId, resource);
}
}
processing_hook = false;
}


// Generalized callers for all callbacks that handles error handling.

// If either runInitCallback() or runCallback() throw then force the
// application to shutdown if one of the callbacks throws. This may 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
// Force the application to shutdown if one of the callbacks throws. This may
// 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 runInitCallback(cb, asyncId, type, triggerId, resource) {
try {
cb(asyncId, type, triggerId, resource);
} catch (e) {
fatalError(e);
}
}


function runCallback(cb, asyncId) {
function init(asyncId, type, triggerId, resource) {
processing_hook = true;
// Use a single try/catch for all hook to avoid setting up one per iteration.
try {
cb(asyncId);
for (var i = 0; i < active_hooks_array.length; i++) {
if (typeof active_hooks_array[i][init_symbol] === 'function') {
active_hooks_array[i][init_symbol](asyncId, type, triggerId, resource);
}
}
} catch (e) {
fatalError(e);
}
processing_hook = false;
}


Expand Down
2 changes: 1 addition & 1 deletion src/async-wrap.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -560,8 +560,8 @@ void AsyncWrap::EmitAsyncInit(Environment* env,
Local<Value> argv[] = {
Number::New(env->isolate(), async_id),
type,
object,
Number::New(env->isolate(), trigger_id),
object,
};

TryCatch try_catch(env->isolate());
Expand Down