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: emitAfter correctly on fatalException #14914

Merged
merged 1 commit into from
Aug 30, 2017
Merged
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
5 changes: 2 additions & 3 deletions lib/internal/bootstrap_node.js
Original file line number Diff line number Diff line change
Expand Up @@ -383,7 +383,7 @@
// Arrays containing hook flags and ids for async_hook calls.
const { async_hook_fields, async_uid_fields } = async_wrap;
// Internal functions needed to manipulate the stack.
const { clearIdStack, popAsyncIds } = async_wrap;
const { clearIdStack, asyncIdStackSize } = async_wrap;
const { kAfter, kCurrentAsyncId, kInitTriggerId } = async_wrap.constants;

process._fatalException = function(er) {
Expand Down Expand Up @@ -420,8 +420,7 @@
do {
NativeModule.require('async_hooks').emitAfter(
async_uid_fields[kCurrentAsyncId]);
// popAsyncIds() returns true if there are more ids on the stack.
} while (popAsyncIds(async_uid_fields[kCurrentAsyncId]));
} while (asyncIdStackSize() > 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Few questions:

  1. why is this better? feels less abstract/more "internal implementation exposed"
  2. who actually pops the ids?
  3. cache emitAfter?
  const emitAfter = NativeModule.require('async_hooks').emitAfter;
  do {
    emitAfter(async_uid_fields[kCurrentAsyncId]);
  }...

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Are we sure asyncIdStackSize() > 0 at the first iteration?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. why is this better? feels less abstract/more "internal implementation exposed"

because it fixes the bug :) and it's actually the other way around. previously I was overriding implementation details in emitAfter().

  1. who actually pops the ids?

emitAfter() pops the id. that's why it was skipping every other id in the stack.

  1. cache emitAfter?

I'm doing it for the same reason as the require('timers') call below. Because if the script fails early in the startup process then the module might not have been loaded yet. And require() already caches the script into an object, doesn't it?

Copy link
Contributor

Choose a reason for hiding this comment

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

who actually pops the ids?

emitAfter() pops the id. that's why it was skipping every other id in the stack.

Ohh emitAfter is emitAfterScript not emitAfterNative

cache emitAfter?

I'm doing it for the same reason as the require('timers') call below. Because if the script fails early in the startup process then the module might not have been loaded yet. And require() already caches the script into an object, doesn't it?

Ack, but if you do it just before the loop it (1) looks cleaner [to me] (2) still lazy enough.

Copy link
Contributor Author

@trevnorris trevnorris Aug 22, 2017

Choose a reason for hiding this comment

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

Ack, but if you do it just before the loop it (1) looks cleaner [to me] (2) still lazy enough.

do you mean creating let async_hooks = null; in setupProcessFatal() then doing something like?:

if (async_hooks === null)
  async_hooks = NativeModule.require('async_hooks');

Or just const async_hooks = NativeModule.require('async_hooks'); before the loop every time?

// Or completely empty the id stack.
} else {
clearIdStack();
Expand Down
8 changes: 8 additions & 0 deletions src/async-wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -450,6 +450,13 @@ void AsyncWrap::PopAsyncIds(const FunctionCallbackInfo<Value>& args) {
}


void AsyncWrap::AsyncIdStackSize(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
args.GetReturnValue().Set(
static_cast<double>(env->async_hooks()->stack_size()));
}


void AsyncWrap::ClearIdStack(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
env->async_hooks()->clear_id_stack();
Expand Down Expand Up @@ -486,6 +493,7 @@ void AsyncWrap::Initialize(Local<Object> target,
env->SetMethod(target, "setupHooks", SetupHooks);
env->SetMethod(target, "pushAsyncIds", PushAsyncIds);
env->SetMethod(target, "popAsyncIds", PopAsyncIds);
env->SetMethod(target, "asyncIdStackSize", AsyncIdStackSize);
env->SetMethod(target, "clearIdStack", ClearIdStack);
env->SetMethod(target, "addIdToDestroyList", QueueDestroyId);
env->SetMethod(target, "enablePromiseHook", EnablePromiseHook);
Expand Down
1 change: 1 addition & 0 deletions src/async-wrap.h
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ class AsyncWrap : public BaseObject {
static void GetAsyncId(const v8::FunctionCallbackInfo<v8::Value>& args);
static void PushAsyncIds(const v8::FunctionCallbackInfo<v8::Value>& args);
static void PopAsyncIds(const v8::FunctionCallbackInfo<v8::Value>& args);
static void AsyncIdStackSize(const v8::FunctionCallbackInfo<v8::Value>& args);
static void ClearIdStack(const v8::FunctionCallbackInfo<v8::Value>& args);
static void AsyncReset(const v8::FunctionCallbackInfo<v8::Value>& args);
static void QueueDestroyId(const v8::FunctionCallbackInfo<v8::Value>& args);
Expand Down
4 changes: 4 additions & 0 deletions src/env-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,10 @@ inline bool Environment::AsyncHooks::pop_ids(double async_id) {
return !ids_stack_.empty();
}

inline size_t Environment::AsyncHooks::stack_size() {
return ids_stack_.size();
}

inline void Environment::AsyncHooks::clear_id_stack() {
while (!ids_stack_.empty())
ids_stack_.pop();
Expand Down
1 change: 1 addition & 0 deletions src/env.h
Original file line number Diff line number Diff line change
Expand Up @@ -400,6 +400,7 @@ class Environment {

inline void push_ids(double async_id, double trigger_id);
inline bool pop_ids(double async_id);
inline size_t stack_size();
inline void clear_id_stack(); // Used in fatal exceptions.

// Used to propagate the trigger_id to the constructor of any newly created
Expand Down
40 changes: 40 additions & 0 deletions test/parallel/test-emit-after-uncaught-exception.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
'use strict';

const common = require('../common');
const assert = require('assert');
const async_hooks = require('async_hooks');

const id_obj = {};
let collect = true;

const hook = async_hooks.createHook({
before(id) { if (collect) id_obj[id] = true; },
after(id) { delete id_obj[id]; },
}).enable();

process.once('uncaughtException', common.mustCall((er) => {
assert.strictEqual(er.message, 'bye');
collect = false;
}));

Copy link
Contributor

Choose a reason for hiding this comment

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

what happens is you throw here (where AFAICT the stack is still empty)?
What's the value of async_uid_fields[kCurrentAsyncId])?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The first pass of the script is the "bootstrap", which always has an id of 1. But as soon as it throws the stack is cleared and async_uid_fields[kCurrentAsyncId] === 0 until another stack is entered. That answer your question?

setImmediate(common.mustCall(() => {
process.nextTick(common.mustCall(() => {
assert.strictEqual(Object.keys(id_obj).length, 0);
hook.disable();
}));

// Create a stack of async ids that will need to be emitted in the case of
// an uncaught exception.
const ar1 = new async_hooks.AsyncResource('Mine');
ar1.emitBefore();

const ar2 = new async_hooks.AsyncResource('Mine');
ar2.emitBefore();

throw new Error('bye');

// TODO(trevnorris): This test shows that the after() hooks are always called
// correctly, but it doesn't solve where the emitDestroy() is missed because
// of the uncaught exception. Simple solution is to always call emitDestroy()
// before the emitAfter(), but how to codify this?
}));