From 700d576962f21b6f65e66b2016dd1a005e76ccc4 Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Thu, 17 Aug 2017 16:44:27 -0600 Subject: [PATCH] async_hooks: emitAfter correctly on fatalException Previously calling emitAfter() from _fatalException would skipt the first asyncId. Instead use the size() of the std::stack to determine how many times to loop and call emitAfter(). PR-URL: https://github.com/nodejs/node/pull/14914 Reviewed-By: James M Snell Reviewed-By: Refael Ackermann --- lib/internal/bootstrap_node.js | 5 +-- src/async-wrap.cc | 8 ++++ src/async-wrap.h | 1 + src/env-inl.h | 4 ++ src/env.h | 1 + .../test-emit-after-uncaught-exception.js | 40 +++++++++++++++++++ 6 files changed, 56 insertions(+), 3 deletions(-) create mode 100644 test/parallel/test-emit-after-uncaught-exception.js diff --git a/lib/internal/bootstrap_node.js b/lib/internal/bootstrap_node.js index 9e26080987183e..e11df46e7f8053 100644 --- a/lib/internal/bootstrap_node.js +++ b/lib/internal/bootstrap_node.js @@ -347,7 +347,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) { @@ -384,8 +384,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); // Or completely empty the id stack. } else { clearIdStack(); diff --git a/src/async-wrap.cc b/src/async-wrap.cc index b6588a20ad4071..3228299233e468 100644 --- a/src/async-wrap.cc +++ b/src/async-wrap.cc @@ -474,6 +474,13 @@ void AsyncWrap::PopAsyncIds(const FunctionCallbackInfo& args) { } +void AsyncWrap::AsyncIdStackSize(const FunctionCallbackInfo& args) { + Environment* env = Environment::GetCurrent(args); + args.GetReturnValue().Set( + static_cast(env->async_hooks()->stack_size())); +} + + void AsyncWrap::ClearIdStack(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); env->async_hooks()->clear_id_stack(); @@ -503,6 +510,7 @@ void AsyncWrap::Initialize(Local 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); diff --git a/src/async-wrap.h b/src/async-wrap.h index ffdf8358747f12..5bc48d7faf0c30 100644 --- a/src/async-wrap.h +++ b/src/async-wrap.h @@ -102,6 +102,7 @@ class AsyncWrap : public BaseObject { static void GetAsyncId(const v8::FunctionCallbackInfo& args); static void PushAsyncIds(const v8::FunctionCallbackInfo& args); static void PopAsyncIds(const v8::FunctionCallbackInfo& args); + static void AsyncIdStackSize(const v8::FunctionCallbackInfo& args); static void ClearIdStack(const v8::FunctionCallbackInfo& args); static void AsyncReset(const v8::FunctionCallbackInfo& args); static void QueueDestroyId(const v8::FunctionCallbackInfo& args); diff --git a/src/env-inl.h b/src/env-inl.h index 888dd807c11e15..b651d969a3d7e8 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -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(); diff --git a/src/env.h b/src/env.h index 80ef32b896d98b..47a6487b77dbe6 100644 --- a/src/env.h +++ b/src/env.h @@ -396,6 +396,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 diff --git a/test/parallel/test-emit-after-uncaught-exception.js b/test/parallel/test-emit-after-uncaught-exception.js new file mode 100644 index 00000000000000..368099d48355be --- /dev/null +++ b/test/parallel/test-emit-after-uncaught-exception.js @@ -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; +})); + +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? +}));