From f5ef678bd896b07e4009a785c85e15f5fde7d2d0 Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Wed, 9 Aug 2017 00:02:20 -0600 Subject: [PATCH 1/4] async_wrap: return undefined if domain is disposed v8::MaybeLocal::ToLocalChecked() will abort on an empty handle. AsyncWrap::MakeCallback returns an empty handle if the domain is disposed, but this should not cause the process to abort. So instead return v8::Undefined() and allow the error to be handled normally. PR-URL: https://github.com/nodejs/node/pull/14722 --- src/async-wrap.cc | 5 ++-- .../test-domain-abort-when-disposed.js | 25 +++++++++++++++++++ 2 files changed, 28 insertions(+), 2 deletions(-) create mode 100644 test/parallel/test-domain-abort-when-disposed.js diff --git a/src/async-wrap.cc b/src/async-wrap.cc index 86759bc62c63b4..8132aa40063f3c 100644 --- a/src/async-wrap.cc +++ b/src/async-wrap.cc @@ -54,6 +54,7 @@ using v8::String; using v8::Symbol; using v8::TryCatch; using v8::Uint32Array; +using v8::Undefined; using v8::Value; using AsyncHooks = node::Environment::AsyncHooks; @@ -687,7 +688,7 @@ MaybeLocal AsyncWrap::MakeCallback(const Local cb, get_trigger_id()); if (!PreCallbackExecution(this, true)) { - return MaybeLocal(); + return Undefined(env()->isolate()); } // Finally... Get to running the user's callback. @@ -698,7 +699,7 @@ MaybeLocal AsyncWrap::MakeCallback(const Local cb, } if (!PostCallbackExecution(this, true)) { - return Local(); + return Undefined(env()->isolate()); } exec_scope.Dispose(); diff --git a/test/parallel/test-domain-abort-when-disposed.js b/test/parallel/test-domain-abort-when-disposed.js new file mode 100644 index 00000000000000..3a02b1e94c1b11 --- /dev/null +++ b/test/parallel/test-domain-abort-when-disposed.js @@ -0,0 +1,25 @@ +'use strict'; + +const common = require('../common'); +const assert = require('assert'); +const domain = require('domain'); + +// These were picked arbitrarily and are only used to trigger arync_hooks. +const JSStream = process.binding('js_stream').JSStream; +const Socket = require('net').Socket; + +const handle = new JSStream(); +handle.domain = domain.create(); +handle.domain.dispose(); + +handle.close = () => {}; +handle.isAlive = () => { throw new Error(); }; + +const s = new Socket({ handle }); + +// When AsyncWrap::MakeCallback() returned an empty handle the +// MaybeLocal::ToLocalChecked() call caused the process to abort. By returning +// v8::Undefined() it allows the error to propagate to the 'error' event. +s.on('error', common.mustCall((e) => { + assert.strictEqual(e.code, 'EINVAL'); +})); From 0f863d0ec22789d2570edf793233ec69ea9ab627 Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Wed, 2 Aug 2017 01:34:59 -0600 Subject: [PATCH 2/4] async_hooks: improve comments and function names * Reword several of the comments to be more descriptive. * Rename functions to better indicate what they are doing. * Change AsyncHooks::uid_fields_ to be a fixed array instead of a pointer. * Define regex early so line ends before 80 columns. * Remove obsolete comments. * Rename AsyncHooks::InitScope::uid_fields_ to uid_fields_ptr_ because using the same name as AsyncHooks::uid_fields_ was confusing. * Place variables that are used to store the new set of hooks if another hook is enabled/disabled during hook execution into an object to act as a namespace. PR-URL: https://github.com/nodejs/node/pull/14722 --- lib/async_hooks.js | 190 ++++++++++++++++++++++++--------------------- src/env-inl.h | 6 +- src/env.h | 10 +-- 3 files changed, 109 insertions(+), 97 deletions(-) diff --git a/lib/async_hooks.js b/lib/async_hooks.js index cb07938039052b..41810271a42b70 100644 --- a/lib/async_hooks.js +++ b/lib/async_hooks.js @@ -1,39 +1,58 @@ 'use strict'; const async_wrap = process.binding('async_wrap'); -/* Both these arrays are used to communicate between JS and C++ with as little - * overhead as possible. +/* async_hook_fields is a Uint32Array wrapping the uint32_t array of + * Environment::AsyncHooks::fields_[]. Each index tracks the number of active + * hooks for each type. * - * async_hook_fields is a Uint32Array() that communicates the number of each - * type of active hooks of each type and wraps the uin32_t array of - * node::Environment::AsyncHooks::fields_. - * - * async_uid_fields is a Float64Array() that contains the async/trigger ids for - * several operations. These fields are as follows: - * kCurrentAsyncId: The async id of the current execution stack. - * kCurrentTriggerId: The trigger id of the current execution stack. - * kAsyncUidCntr: Counter that tracks the unique ids given to new resources. - * kInitTriggerId: Written to just before creating a new resource, so the - * constructor knows what other resource is responsible for its init(). - * Used this way so the trigger id doesn't need to be passed to every - * resource's constructor. + * async_uid_fields is a Float64Array wrapping the double array of + * Environment::AsyncHooks::uid_fields_[]. Each index contains the ids for the + * various asynchronous states of the application. These are: + * kCurrentAsyncId: The async_id assigned to the resource responsible for the + * current execution stack. + * kCurrentTriggerId: The trigger_async_id of the resource responsible for the + * current execution stack. + * kAsyncUidCntr: Incremental counter tracking the next assigned async_id. + * kInitTriggerId: Written immediately before a resource's constructor that + * sets the value of the init()'s triggerAsyncId. The order of retrieving + * the triggerAsyncId value is passing directly to the constructor -> value + * set in kInitTriggerId -> executionAsyncId of the current resource. */ const { async_hook_fields, async_uid_fields } = async_wrap; -// Used to change the state of the async id stack. +// Store the pair executionAsyncId and triggerAsyncId in a std::stack on +// Environment::AsyncHooks::ids_stack_ tracks the resource responsible for the +// current execution stack. This is unwound as each resource exits. In the case +// of a fatal exception this stack is emptied after calling each hook's after() +// callback. const { pushAsyncIds, popAsyncIds } = async_wrap; -// Array of all AsyncHooks that will be iterated whenever an async event fires. -// Using var instead of (preferably const) in order to assign -// tmp_active_hooks_array if a hook is enabled/disabled during hook execution. -var active_hooks_array = []; -// Use a counter to track whether a hook callback is currently being processed. -// Used to make sure active_hooks_array isn't altered in mid execution if -// another hook is added or removed. A counter is used to track nested calls. -var processing_hook = 0; -// Use to temporarily store and updated active_hooks_array if the user enables -// or disables a hook while hooks are being processed. -var tmp_active_hooks_array = null; -// Keep track of the field counts held in tmp_active_hooks_array. -var tmp_async_hook_fields = null; +// For performance reasons, only track Proimses when a hook is enabled. +const { enablePromiseHook, disablePromiseHook } = async_wrap; +// Properties in active_hooks are used to keep track of the set of hooks being +// executed in case another hook is enabled/disabled. The new set of hooks is +// then restored once the active set of hooks is finished executing. +const active_hooks = { + // Array of all AsyncHooks that will be iterated whenever an async event + // fires. Using var instead of (preferably const) in order to assign + // active_hooks.tmp_array if a hook is enabled/disabled during hook + // execution. + array: [], + // Use a counter to track nested calls of async hook callbacks and make sure + // the active_hooks.array isn't altered mid execution. + call_depth: 0, + // Use to temporarily store and updated active_hooks.array if the user + // enables or disables a hook while hooks are being processed. If a hook is + // enabled() or disabled() during hook execution then the current set of + // active hooks is duplicated and set equal to active_hooks.tmp_array. Any + // subsequent changes are on the duplicated array. When all hooks have + // completed executing active_hooks.tmp_array is assigned to + // active_hooks.array. + tmp_array: null, + // Keep track of the field counts held in active_hooks.tmp_array. Because the + // async_hook_fields can't be reassigned, store each uint32 in an array that + // is written back to async_hook_fields when active_hooks.array is restored. + tmp_fields: null +}; + // Each constant tracks how many callbacks there are for any given step of // async execution. These are tracked so if the user didn't include callbacks @@ -42,6 +61,9 @@ const { kInit, kBefore, kAfter, kDestroy, kTotals, kCurrentAsyncId, kCurrentTriggerId, kAsyncUidCntr, kInitTriggerId } = async_wrap.constants; +// Symbols used to store the respective ids on both AsyncResource instances and +// internal resources. They will also be assigned to arbitrary objects passed +// in by the user that take place of internally constructed objects. const { async_id_symbol, trigger_id_symbol } = async_wrap; // Used in AsyncHook and AsyncResource. @@ -53,6 +75,9 @@ const emitBeforeNative = emitHookFactory(before_symbol, 'emitBeforeNative'); const emitAfterNative = emitHookFactory(after_symbol, 'emitAfterNative'); const emitDestroyNative = emitHookFactory(destroy_symbol, 'emitDestroyNative'); +// TODO(refack): move to node-config.cc +const abort_regex = /^--abort[_-]on[_-]uncaught[_-]exception$/; + // Setup the callbacks that node::AsyncWrap will call when there are hooks to // process. They use the same functions as the JS embedder API. These callbacks // are setup immediately to prevent async_wrap.setupHooks() from being hijacked @@ -71,7 +96,7 @@ function fatalError(e) { Error.captureStackTrace(o, fatalError); process._rawDebug(o.stack); } - if (process.execArgv.some((e) => /^--abort[_-]on[_-]uncaught[_-]exception$/.test(e))) { + if (process.execArgv.some((e) => abort_regex.test(e))) { process.abort(); } process.exit(1); @@ -121,7 +146,7 @@ class AsyncHook { hooks_array.push(this); if (prev_kTotals === 0 && hook_fields[kTotals] > 0) - async_wrap.enablePromiseHook(); + enablePromiseHook(); return this; } @@ -143,7 +168,7 @@ class AsyncHook { hooks_array.splice(index, 1); if (prev_kTotals > 0 && hook_fields[kTotals] === 0) - async_wrap.disablePromiseHook(); + disablePromiseHook(); return this; } @@ -151,41 +176,41 @@ class AsyncHook { function getHookArrays() { - if (processing_hook === 0) - return [active_hooks_array, async_hook_fields]; + if (active_hooks.call_depth === 0) + return [active_hooks.array, async_hook_fields]; // If this hook is being enabled while in the middle of processing the array // of currently active hooks then duplicate the current set of active hooks // and store this there. This shouldn't fire until the next time hooks are // processed. - if (tmp_active_hooks_array === null) + if (active_hooks.tmp_array === null) storeActiveHooks(); - return [tmp_active_hooks_array, tmp_async_hook_fields]; + return [active_hooks.tmp_array, active_hooks.tmp_fields]; } function storeActiveHooks() { - tmp_active_hooks_array = active_hooks_array.slice(); + active_hooks.tmp_array = active_hooks.array.slice(); // Don't want to make the assumption that kInit to kDestroy are indexes 0 to // 4. So do this the long way. - tmp_async_hook_fields = []; - tmp_async_hook_fields[kInit] = async_hook_fields[kInit]; - tmp_async_hook_fields[kBefore] = async_hook_fields[kBefore]; - tmp_async_hook_fields[kAfter] = async_hook_fields[kAfter]; - tmp_async_hook_fields[kDestroy] = async_hook_fields[kDestroy]; + active_hooks.tmp_fields = []; + active_hooks.tmp_fields[kInit] = async_hook_fields[kInit]; + active_hooks.tmp_fields[kBefore] = async_hook_fields[kBefore]; + active_hooks.tmp_fields[kAfter] = async_hook_fields[kAfter]; + active_hooks.tmp_fields[kDestroy] = async_hook_fields[kDestroy]; } // Then restore the correct hooks array in case any hooks were added/removed // during hook callback execution. -function restoreTmpHooks() { - active_hooks_array = tmp_active_hooks_array; - async_hook_fields[kInit] = tmp_async_hook_fields[kInit]; - async_hook_fields[kBefore] = tmp_async_hook_fields[kBefore]; - async_hook_fields[kAfter] = tmp_async_hook_fields[kAfter]; - async_hook_fields[kDestroy] = tmp_async_hook_fields[kDestroy]; - - tmp_active_hooks_array = null; - tmp_async_hook_fields = null; +function restoreActiveHooks() { + active_hooks.array = active_hooks.tmp_array; + async_hook_fields[kInit] = active_hooks.tmp_fields[kInit]; + async_hook_fields[kBefore] = active_hooks.tmp_fields[kBefore]; + async_hook_fields[kAfter] = active_hooks.tmp_fields[kAfter]; + async_hook_fields[kDestroy] = active_hooks.tmp_fields[kDestroy]; + + active_hooks.tmp_array = null; + active_hooks.tmp_fields = null; } @@ -322,25 +347,30 @@ function emitHookFactory(symbol, name) { // before this is called. // eslint-disable-next-line func-style const fn = function(asyncId) { - processing_hook += 1; + active_hooks.call_depth += 1; // 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][symbol] === 'function') { - active_hooks_array[i][symbol](asyncId); + for (var i = 0; i < active_hooks.array.length; i++) { + if (typeof active_hooks.array[i][symbol] === 'function') { + active_hooks.array[i][symbol](asyncId); } } } catch (e) { fatalError(e); } finally { - processing_hook -= 1; + active_hooks.call_depth -= 1; } - if (processing_hook === 0 && tmp_active_hooks_array !== null) { - restoreTmpHooks(); + // Hooks can only be restored if there have been no recursive hook calls. + // Also the active hooks do not need to be restored if enable()/disable() + // weren't called during hook execution, in which case + // active_hooks.tmp_array will be null. + if (active_hooks.call_depth === 0 && active_hooks.tmp_array !== null) { + restoreActiveHooks(); } }; + // Set the name property of the anonymous function as it looks good in the // stack trace. Object.defineProperty(fn, 'name', { @@ -367,9 +397,6 @@ function emitBeforeScript(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 emitAfterScript(asyncId) { if (async_hook_fields[kAfter] > 0) emitAfterNative(asyncId); @@ -387,26 +414,16 @@ function emitDestroyScript(asyncId) { } -// Emit callbacks for native calls. Since some state can be setup directly from -// C++ there's no need to perform all the work here. - -// This should only be called if hooks_array has kInit > 0. There are no global -// values to setup. Though hooks_array will be cloned if C++ needed to call -// 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. - -// 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. +// Used by C++ to call all init() callbacks. Because some state can be setup +// from C++ there's no need to perform all the same operations as in +// emitInitScript. function emitInitNative(asyncId, type, triggerAsyncId, resource) { - processing_hook += 1; + active_hooks.call_depth += 1; // 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][init_symbol] === 'function') { - active_hooks_array[i][init_symbol]( + 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, triggerAsyncId, resource ); @@ -415,18 +432,15 @@ function emitInitNative(asyncId, type, triggerAsyncId, resource) { } catch (e) { fatalError(e); } finally { - processing_hook -= 1; + active_hooks.call_depth -= 1; } - // * `tmp_active_hooks_array` is null if no hooks were added/removed while - // the hooks were running. In that case no restoration is needed. - // * In the case where another hook was added/removed while the hooks were - // running and a handle was created causing the `init` hooks to fire again, - // then `restoreTmpHooks` should not be called for the nested `hooks`. - // Otherwise `active_hooks_array` can change during execution of the - // `hooks`. - if (processing_hook === 0 && tmp_active_hooks_array !== null) { - restoreTmpHooks(); + // Hooks can only be restored if there have been no recursive hook calls. + // Also the active hooks do not need to be restored if enable()/disable() + // weren't called during hook execution, in which case active_hooks.tmp_array + // will be null. + if (active_hooks.call_depth === 0 && active_hooks.tmp_array !== null) { + restoreActiveHooks(); } } diff --git a/src/env-inl.h b/src/env-inl.h index f3ca8882b033cb..024a91f2aa50ca 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -176,13 +176,13 @@ inline void Environment::AsyncHooks::clear_id_stack() { inline Environment::AsyncHooks::InitScope::InitScope( Environment* env, double init_trigger_id) : env_(env), - uid_fields_(env->async_hooks()->uid_fields()) { - env->async_hooks()->push_ids(uid_fields_[AsyncHooks::kCurrentAsyncId], + uid_fields_ref_(env->async_hooks()->uid_fields()) { + env->async_hooks()->push_ids(uid_fields_ref_[AsyncHooks::kCurrentAsyncId], init_trigger_id); } inline Environment::AsyncHooks::InitScope::~InitScope() { - env_->async_hooks()->pop_ids(uid_fields_[AsyncHooks::kCurrentAsyncId]); + env_->async_hooks()->pop_ids(uid_fields_ref_[AsyncHooks::kCurrentAsyncId]); } inline Environment::AsyncHooks::ExecScope::ExecScope( diff --git a/src/env.h b/src/env.h index f0774e61a65d08..9fdf9cdced493c 100644 --- a/src/env.h +++ b/src/env.h @@ -401,7 +401,7 @@ class Environment { private: Environment* env_; - double* uid_fields_; + double* uid_fields_ref_; DISALLOW_COPY_AND_ASSIGN(InitScope); }; @@ -434,12 +434,10 @@ class Environment { v8::Isolate* isolate_; // Stores the ids of the current execution context stack. std::stack ids_stack_; - // Used to communicate state between C++ and JS cheaply. Is placed in an - // Uint32Array() and attached to the async_wrap object. + // Attached to a Uint32Array that tracks the number of active hooks for + // each type. uint32_t fields_[kFieldsCount]; - // Used to communicate ids between C++ and JS cheaply. Placed in a - // Float64Array and attached to the async_wrap object. Using a double only - // gives us 2^53-1 unique ids, but that should be sufficient. + // Attached to a Float64Array that tracks the state of async resources. double uid_fields_[kUidFieldsCount]; DISALLOW_COPY_AND_ASSIGN(AsyncHooks); From f2c1638cbc4d19aa6f8c29e6967e867b2a949c2f Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Thu, 3 Aug 2017 17:03:41 -0600 Subject: [PATCH 3/4] async_wrap: unroll unnecessarily DRY code * Because Emit{Before,After}() will always exit the process if there's an exception there's no need to check a return value. This simplifies the conditions and makes {Pre,Post}CallbackExecution() unnecessary. They have been removed and relevant code has been moved to the respective call sites. Which are: * PromiseHook() never needs to run domains, and without a return value to check place the calls to Emit{Before,After}() on location. * The logic in MakeCallback() is simplified by moving the single calls to Emit{Before,After}() then doing a simpler conditional to check if the domain has been disposed. * Change Domain{Enter,Exit}() to return true if the instance has been disposed. Makes the conditional check in MakeCallback() simpler to reason about. * Add UNREACHABLE() after FatalException() to make sure all error handlers have been cleared and the process really does exit. PR-URL: https://github.com/nodejs/node/pull/14722 --- src/async-wrap.cc | 102 +++++++++++++++++++--------------------------- src/async-wrap.h | 5 ++- src/node.cc | 8 ++-- 3 files changed, 50 insertions(+), 65 deletions(-) diff --git a/src/async-wrap.cc b/src/async-wrap.cc index 8132aa40063f3c..7630de3694df99 100644 --- a/src/async-wrap.cc +++ b/src/async-wrap.cc @@ -165,6 +165,7 @@ static void DestroyIdsCb(uv_timer_t* handle) { if (ret.IsEmpty()) { ClearFatalExceptionHandlers(env); FatalException(env->isolate(), try_catch); + UNREACHABLE(); } } } while (!env->destroy_ids_list()->empty()); @@ -218,69 +219,43 @@ bool DomainExit(Environment* env, v8::Local object) { } -static bool PreCallbackExecution(AsyncWrap* wrap, bool run_domain_cbs) { - if (wrap->env()->using_domains() && run_domain_cbs) { - bool is_disposed = DomainEnter(wrap->env(), wrap->object()); - if (is_disposed) - return false; - } - - return AsyncWrap::EmitBefore(wrap->env(), wrap->get_id()); -} - - -bool AsyncWrap::EmitBefore(Environment* env, double async_id) { +void AsyncWrap::EmitBefore(Environment* env, double async_id) { AsyncHooks* async_hooks = env->async_hooks(); - if (async_hooks->fields()[AsyncHooks::kBefore] > 0) { - Local uid = Number::New(env->isolate(), async_id); - Local fn = env->async_hooks_before_function(); - TryCatch try_catch(env->isolate()); - MaybeLocal ar = fn->Call( - env->context(), Undefined(env->isolate()), 1, &uid); - if (ar.IsEmpty()) { - ClearFatalExceptionHandlers(env); - FatalException(env->isolate(), try_catch); - return false; - } - } - - return true; -} - - -static bool PostCallbackExecution(AsyncWrap* wrap, bool run_domain_cbs) { - if (!AsyncWrap::EmitAfter(wrap->env(), wrap->get_id())) - return false; + if (async_hooks->fields()[AsyncHooks::kBefore] == 0) + return; - if (wrap->env()->using_domains() && run_domain_cbs) { - bool is_disposed = DomainExit(wrap->env(), wrap->object()); - if (is_disposed) - return false; + Local uid = Number::New(env->isolate(), async_id); + Local fn = env->async_hooks_before_function(); + TryCatch try_catch(env->isolate()); + MaybeLocal ar = fn->Call( + env->context(), Undefined(env->isolate()), 1, &uid); + if (ar.IsEmpty()) { + ClearFatalExceptionHandlers(env); + FatalException(env->isolate(), try_catch); + UNREACHABLE(); } - - return true; } -bool AsyncWrap::EmitAfter(Environment* env, double async_id) { + +void AsyncWrap::EmitAfter(Environment* env, double async_id) { AsyncHooks* async_hooks = env->async_hooks(); - // If the callback failed then the after() hooks will be called at the end - // of _fatalException(). - if (async_hooks->fields()[AsyncHooks::kAfter] > 0) { - Local uid = Number::New(env->isolate(), async_id); - Local fn = env->async_hooks_after_function(); - TryCatch try_catch(env->isolate()); - MaybeLocal ar = fn->Call( - env->context(), Undefined(env->isolate()), 1, &uid); - if (ar.IsEmpty()) { - ClearFatalExceptionHandlers(env); - FatalException(env->isolate(), try_catch); - return false; - } - } + if (async_hooks->fields()[AsyncHooks::kAfter] == 0) + return; - return true; + // If the user's callback failed then the after() hooks will be called at the + // end of _fatalException(). + Local uid = Number::New(env->isolate(), async_id); + Local fn = env->async_hooks_after_function(); + TryCatch try_catch(env->isolate()); + MaybeLocal ar = fn->Call( + env->context(), Undefined(env->isolate()), 1, &uid); + if (ar.IsEmpty()) { + ClearFatalExceptionHandlers(env); + FatalException(env->isolate(), try_catch); + UNREACHABLE(); + } } class PromiseWrap : public AsyncWrap { @@ -373,9 +348,9 @@ static void PromiseHook(PromiseHookType type, Local promise, CHECK_NE(wrap, nullptr); if (type == PromiseHookType::kBefore) { env->async_hooks()->push_ids(wrap->get_id(), wrap->get_trigger_id()); - PreCallbackExecution(wrap, false); + AsyncWrap::EmitBefore(wrap->env(), wrap->get_id()); } else if (type == PromiseHookType::kAfter) { - PostCallbackExecution(wrap, false); + AsyncWrap::EmitAfter(wrap->env(), wrap->get_id()); if (env->current_async_id() == wrap->get_id()) { // This condition might not be true if async_hooks was enabled during // the promise callback execution. @@ -687,18 +662,27 @@ MaybeLocal AsyncWrap::MakeCallback(const Local cb, get_id(), get_trigger_id()); - if (!PreCallbackExecution(this, true)) { + // Return v8::Undefined() because returning an empty handle will cause + // ToLocalChecked() to abort. + if (env()->using_domains() && DomainEnter(env(), object())) { return Undefined(env()->isolate()); } - // Finally... Get to running the user's callback. + // No need to check a return value because the application will exit if an + // exception occurs. + AsyncWrap::EmitBefore(env(), get_id()); + MaybeLocal ret = cb->Call(env()->context(), object(), argc, argv); if (ret.IsEmpty()) { return ret; } - if (!PostCallbackExecution(this, true)) { + AsyncWrap::EmitAfter(env(), get_id()); + + // Return v8::Undefined() because returning an empty handle will cause + // ToLocalChecked() to abort. + if (env()->using_domains() && DomainExit(env(), object())) { return Undefined(env()->isolate()); } diff --git a/src/async-wrap.h b/src/async-wrap.h index a02356e84547be..f2fa8f4334f34d 100644 --- a/src/async-wrap.h +++ b/src/async-wrap.h @@ -111,8 +111,8 @@ class AsyncWrap : public BaseObject { double id, double trigger_id); - static bool EmitBefore(Environment* env, double id); - static bool EmitAfter(Environment* env, double id); + static void EmitBefore(Environment* env, double id); + static void EmitAfter(Environment* env, double id); inline ProviderType provider_type() const; @@ -146,6 +146,7 @@ class AsyncWrap : public BaseObject { void LoadAsyncWrapperInfo(Environment* env); +// Return value is an indicator whether the domain was disposed. bool DomainEnter(Environment* env, v8::Local object); bool DomainExit(Environment* env, v8::Local object); diff --git a/src/node.cc b/src/node.cc index bfd1eeb954dd54..be2a96c1ed6191 100644 --- a/src/node.cc +++ b/src/node.cc @@ -1328,8 +1328,9 @@ MaybeLocal MakeCallback(Environment* env, asyncContext.trigger_async_id); if (asyncContext.async_id != 0) { - if (!AsyncWrap::EmitBefore(env, asyncContext.async_id)) - return Local(); + // No need to check a return value because the application will exit if + // an exception occurs. + AsyncWrap::EmitBefore(env, asyncContext.async_id); } ret = callback->Call(env->context(), recv, argc, argv); @@ -1342,8 +1343,7 @@ MaybeLocal MakeCallback(Environment* env, } if (asyncContext.async_id != 0) { - if (!AsyncWrap::EmitAfter(env, asyncContext.async_id)) - return Local(); + AsyncWrap::EmitAfter(env, asyncContext.async_id); } } From 14c3a5ef314b1698b935c679b16e4b33ba5eed13 Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Thu, 3 Aug 2017 14:43:41 -0600 Subject: [PATCH 4/4] async_hooks: don't abort unnecessarily * id values of -1 are allowed. They indicate that the id was never correctly assigned to the async resource. These will appear in any call graph, and will only be apparent to those using the async_hooks module, then reported in an issue. * ids < -1 are still not allowed and will cause the application to exit the process; because there is no scenario where this should ever happen. * Add asyncId range checks to emitAfterScript(). * Fix emitBeforeScript() range checks which should have been || not &&. * Replace errors with entries in internal/errors. * Fix async_hooks tests that check for exceptions to match new internal/errors entries. NOTE: emit{Before,After,Destroy}() must continue to exit the process because in the case of an exception during hook execution the state of the application is unknowable. For example, an exception could cause a memory leak: const id_map = new Map(); before(id) { id_map.set(id, /* data object or similar */); }, after(id) { throw new Error('id never dies!'); id_map.delete(id); } Allowing a recoverable exception may also cause an abort because of a stack check in Environment::AsyncHooks::pop_ids() that verifies the async id and pop'd ids match. This case would be more difficult to debug than if fatalError() (lib/async_hooks.js) was called immediately. try { async_hooks.emitBefore(null, NaN); } catch (e) { } // do something async_hooks.emitAfter(5); It also allows an edge case where emitBefore() could be called twice and not have the pop_ids() CHECK fail: try { async_hooks.emitBefore(5, NaN); } catch (e) { } async_hooks.emitBefore(5); // do something async_hooks.emitAfter(5); There is the option of allowing mismatches in the stack and ignoring the check if no async hooks are enabled, but I don't believe going this far is necessary. PR-URL: https://github.com/nodejs/node/pull/14722 --- lib/async_hooks.js | 68 ++++++++++++------- lib/internal/errors.js | 3 + src/env-inl.h | 7 +- .../test-embedder.api.async-resource.js | 16 +++-- test/async-hooks/test-emit-before-after.js | 16 ++--- test/async-hooks/test-emit-init.js | 24 +++++-- ...t-async-hooks-asyncresource-constructor.js | 22 ++++-- test/parallel/test-async-wrap-constructor.js | 7 +- test/parallel/test-internal-errors.js | 12 ++++ 9 files changed, 124 insertions(+), 51 deletions(-) diff --git a/lib/async_hooks.js b/lib/async_hooks.js index 41810271a42b70..61b9d51e8ab000 100644 --- a/lib/async_hooks.js +++ b/lib/async_hooks.js @@ -1,6 +1,7 @@ 'use strict'; const async_wrap = process.binding('async_wrap'); +const errors = require('internal/errors'); /* async_hook_fields is a Uint32Array wrapping the uint32_t array of * Environment::AsyncHooks::fields_[]. Each index tracks the number of active * hooks for each type. @@ -108,13 +109,13 @@ function fatalError(e) { class AsyncHook { constructor({ init, before, after, destroy }) { if (init !== undefined && typeof init !== 'function') - throw new TypeError('init must be a function'); + throw new errors.TypeError('ERR_ASYNC_CALLBACK', 'init'); if (before !== undefined && typeof before !== 'function') - throw new TypeError('before must be a function'); + throw new errors.TypeError('ERR_ASYNC_CALLBACK', 'before'); if (after !== undefined && typeof after !== 'function') - throw new TypeError('after must be a function'); + throw new errors.TypeError('ERR_ASYNC_CALLBACK', 'before'); if (destroy !== undefined && typeof destroy !== 'function') - throw new TypeError('destroy must be a function'); + throw new errors.TypeError('ERR_ASYNC_CALLBACK', 'before'); this[init_symbol] = init; this[before_symbol] = before; @@ -235,8 +236,11 @@ class AsyncResource { constructor(type, triggerAsyncId = initTriggerId()) { // 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'); + if (!Number.isSafeInteger(triggerAsyncId) || triggerAsyncId < -1) { + throw new errors.RangeError('ERR_INVALID_ASYNC_ID', + 'triggerAsyncId', + triggerAsyncId); + } this[async_id_symbol] = ++async_uid_fields[kAsyncUidCntr]; this[trigger_id_symbol] = triggerAsyncId; @@ -330,14 +334,17 @@ function emitInitScript(asyncId, type, triggerAsyncId, resource) { async_uid_fields[kInitTriggerId] = 0; } - // TODO(trevnorris): I'd prefer allowing these checks to not exist, or only - // throw in a debug build, in order to improve performance. - if (!Number.isSafeInteger(asyncId) || asyncId < 0) - throw new RangeError('asyncId must be an unsigned integer'); - if (typeof type !== 'string' || type.length <= 0) - throw new TypeError('type must be a string with length > 0'); - if (!Number.isSafeInteger(triggerAsyncId) || triggerAsyncId < 0) - throw new RangeError('triggerAsyncId must be an unsigned integer'); + if (!Number.isSafeInteger(asyncId) || asyncId < -1) { + throw new errors.RangeError('ERR_INVALID_ASYNC_ID', 'asyncId', asyncId); + } + if (!Number.isSafeInteger(triggerAsyncId) || triggerAsyncId < -1) { + throw new errors.RangeError('ERR_INVALID_ASYNC_ID', + 'triggerAsyncId', + triggerAsyncId); + } + if (typeof type !== 'string' || type.length <= 0) { + throw new errors.TypeError('ERR_ASYNC_TYPE', type); + } emitInitNative(asyncId, type, triggerAsyncId, resource); } @@ -381,13 +388,17 @@ function emitHookFactory(symbol, name) { function emitBeforeScript(asyncId, triggerAsyncId) { - // CHECK(Number.isSafeInteger(asyncId) && asyncId > 0) - // CHECK(Number.isSafeInteger(triggerAsyncId) && triggerAsyncId > 0) - - // Validate the ids. - if (asyncId < 0 || triggerAsyncId < 0) { - fatalError('before(): asyncId or triggerAsyncId is less than zero ' + - `(asyncId: ${asyncId}, triggerAsyncId: ${triggerAsyncId})`); + // Validate the ids. An id of -1 means it was never set and is visible on the + // call graph. An id < -1 should never happen in any circumstance. Throw + // on user calls because async state should still be recoverable. + if (!Number.isSafeInteger(asyncId) || asyncId < -1) { + fatalError( + new errors.RangeError('ERR_INVALID_ASYNC_ID', 'asyncId', asyncId)); + } + if (!Number.isSafeInteger(triggerAsyncId) || triggerAsyncId < -1) { + fatalError(new errors.RangeError('ERR_INVALID_ASYNC_ID', + 'triggerAsyncId', + triggerAsyncId)); } pushAsyncIds(asyncId, triggerAsyncId); @@ -398,6 +409,11 @@ function emitBeforeScript(asyncId, triggerAsyncId) { function emitAfterScript(asyncId) { + if (!Number.isSafeInteger(asyncId) || asyncId < -1) { + fatalError( + new errors.RangeError('ERR_INVALID_ASYNC_ID', 'asyncId', asyncId)); + } + if (async_hook_fields[kAfter] > 0) emitAfterNative(asyncId); @@ -406,9 +422,13 @@ function emitAfterScript(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) + if (!Number.isSafeInteger(asyncId) || asyncId < -1) { + fatalError( + new errors.RangeError('ERR_INVALID_ASYNC_ID', 'asyncId', asyncId)); + } + + // Return early if there are no destroy callbacks, or invalid asyncId. + if (async_hook_fields[kDestroy] === 0 || asyncId <= 0) return; async_wrap.addIdToDestroyList(asyncId); } diff --git a/lib/internal/errors.js b/lib/internal/errors.js index f83e6c6b212682..263a1243f55624 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -101,6 +101,8 @@ module.exports = exports = { // Note: Please try to keep these in alphabetical order E('ERR_ARG_NOT_ITERABLE', '%s must be iterable'); E('ERR_ASSERTION', '%s'); +E('ERR_ASYNC_CALLBACK', (name) => `${name} must be a function`); +E('ERR_ASYNC_TYPE', (s) => `Invalid name for async "type": ${s}`); E('ERR_BUFFER_OUT_OF_BOUNDS', bufferOutOfBounds); E('ERR_CHILD_CLOSED_BEFORE_REPLY', 'Child closed before reply received'); E('ERR_CONSOLE_WRITABLE_STREAM', @@ -188,6 +190,7 @@ E('ERR_INVALID_ARRAY_LENGTH', assert.strictEqual(typeof actual, 'number'); return `The array "${name}" (length ${actual}) must be of length ${len}.`; }); +E('ERR_INVALID_ASYNC_ID', (type, id) => `Invalid ${type} value: ${id}`); E('ERR_INVALID_BUFFER_SIZE', 'Buffer size must be a multiple of %s'); E('ERR_INVALID_CALLBACK', 'Callback must be a function'); E('ERR_INVALID_CHAR', 'Invalid character in %s'); diff --git a/src/env-inl.h b/src/env-inl.h index 024a91f2aa50ca..af77f3756bf506 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -127,8 +127,8 @@ inline v8::Local Environment::AsyncHooks::provider_string(int idx) { inline void Environment::AsyncHooks::push_ids(double async_id, double trigger_id) { - CHECK_GE(async_id, 0); - CHECK_GE(trigger_id, 0); + CHECK_GE(async_id, -1); + CHECK_GE(trigger_id, -1); ids_stack_.push({ uid_fields_[kCurrentAsyncId], uid_fields_[kCurrentTriggerId] }); @@ -177,6 +177,7 @@ inline Environment::AsyncHooks::InitScope::InitScope( Environment* env, double init_trigger_id) : env_(env), uid_fields_ref_(env->async_hooks()->uid_fields()) { + CHECK_GE(init_trigger_id, -1); env->async_hooks()->push_ids(uid_fields_ref_[AsyncHooks::kCurrentAsyncId], init_trigger_id); } @@ -190,6 +191,8 @@ inline Environment::AsyncHooks::ExecScope::ExecScope( : env_(env), async_id_(async_id), disposed_(false) { + CHECK_GE(async_id, -1); + CHECK_GE(trigger_id, -1); env->async_hooks()->push_ids(async_id, trigger_id); } diff --git a/test/async-hooks/test-embedder.api.async-resource.js b/test/async-hooks/test-embedder.api.async-resource.js index 78985b955ad683..533df6a5c84549 100644 --- a/test/async-hooks/test-embedder.api.async-resource.js +++ b/test/async-hooks/test-embedder.api.async-resource.js @@ -12,10 +12,18 @@ const { checkInvocations } = require('./hook-checks'); const hooks = initHooks(); hooks.enable(); -assert.throws(() => new AsyncResource(), - /^TypeError: type must be a string with length > 0$/); -assert.throws(() => new AsyncResource('invalid_trigger_id', null), - /^RangeError: triggerAsyncId must be an unsigned integer$/); +assert.throws(() => { + new AsyncResource(); +}, common.expectsError({ + code: 'ERR_ASYNC_TYPE', + type: TypeError, +})); +assert.throws(() => { + new AsyncResource('invalid_trigger_id', null); +}, common.expectsError({ + code: 'ERR_INVALID_ASYNC_ID', + type: RangeError, +})); assert.strictEqual( new AsyncResource('default_trigger_id').triggerAsyncId(), diff --git a/test/async-hooks/test-emit-before-after.js b/test/async-hooks/test-emit-before-after.js index 1334767c766a25..2b22739fa9478d 100644 --- a/test/async-hooks/test-emit-before-after.js +++ b/test/async-hooks/test-emit-before-after.js @@ -8,25 +8,25 @@ const initHooks = require('./init-hooks'); switch (process.argv[2]) { case 'test_invalid_async_id': - async_hooks.emitBefore(-1, 1); + async_hooks.emitBefore(-2, 1); return; case 'test_invalid_trigger_id': - async_hooks.emitBefore(1, -1); + async_hooks.emitBefore(1, -2); return; } assert.ok(!process.argv[2]); const c1 = spawnSync(process.execPath, [__filename, 'test_invalid_async_id']); -assert.strictEqual(c1.stderr.toString().split(/[\r\n]+/g)[0], - 'Error: before(): asyncId or triggerAsyncId is less than ' + - 'zero (asyncId: -1, triggerAsyncId: 1)'); +assert.strictEqual( + c1.stderr.toString().split(/[\r\n]+/g)[0], + 'RangeError [ERR_INVALID_ASYNC_ID]: Invalid asyncId value: -2'); assert.strictEqual(c1.status, 1); const c2 = spawnSync(process.execPath, [__filename, 'test_invalid_trigger_id']); -assert.strictEqual(c2.stderr.toString().split(/[\r\n]+/g)[0], - 'Error: before(): asyncId or triggerAsyncId is less than ' + - 'zero (asyncId: 1, triggerAsyncId: -1)'); +assert.strictEqual( + c2.stderr.toString().split(/[\r\n]+/g)[0], + 'RangeError [ERR_INVALID_ASYNC_ID]: Invalid triggerAsyncId value: -2'); assert.strictEqual(c2.status, 1); const expectedId = async_hooks.newUid(); diff --git a/test/async-hooks/test-emit-init.js b/test/async-hooks/test-emit-init.js index feee3d944b8afb..631dcd759968cb 100644 --- a/test/async-hooks/test-emit-init.js +++ b/test/async-hooks/test-emit-init.js @@ -25,12 +25,24 @@ const hooks1 = initHooks({ hooks1.enable(); -assert.throws(() => async_hooks.emitInit(), - /^RangeError: asyncId must be an unsigned integer$/); -assert.throws(() => async_hooks.emitInit(expectedId), - /^TypeError: type must be a string with length > 0$/); -assert.throws(() => async_hooks.emitInit(expectedId, expectedType, -1), - /^RangeError: triggerAsyncId must be an unsigned integer$/); +assert.throws(() => { + async_hooks.emitInit(); +}, common.expectsError({ + code: 'ERR_INVALID_ASYNC_ID', + type: RangeError, +})); +assert.throws(() => { + async_hooks.emitInit(expectedId); +}, common.expectsError({ + code: 'ERR_INVALID_ASYNC_ID', + type: RangeError, +})); +assert.throws(() => { + async_hooks.emitInit(expectedId, expectedType, -2); +}, common.expectsError({ + code: 'ERR_INVALID_ASYNC_ID', + type: RangeError, +})); async_hooks.emitInit(expectedId, expectedType, expectedTriggerId, expectedResource); diff --git a/test/parallel/test-async-hooks-asyncresource-constructor.js b/test/parallel/test-async-hooks-asyncresource-constructor.js index 2fb0bb14ccf64e..2ab5f067ca6c76 100644 --- a/test/parallel/test-async-hooks-asyncresource-constructor.js +++ b/test/parallel/test-async-hooks-asyncresource-constructor.js @@ -1,8 +1,8 @@ 'use strict'; -require('../common'); // This tests that AsyncResource throws an error if bad parameters are passed +const common = require('../common'); const assert = require('assert'); const async_hooks = require('async_hooks'); const { AsyncResource } = async_hooks; @@ -14,16 +14,28 @@ async_hooks.createHook({ assert.throws(() => { return new AsyncResource(); -}, /^TypeError: type must be a string with length > 0$/); +}, common.expectsError({ + code: 'ERR_ASYNC_TYPE', + type: TypeError, +})); assert.throws(() => { new AsyncResource(''); -}, /^TypeError: type must be a string with length > 0$/); +}, common.expectsError({ + code: 'ERR_ASYNC_TYPE', + type: TypeError, +})); assert.throws(() => { new AsyncResource('type', -4); -}, /^RangeError: triggerAsyncId must be an unsigned integer$/); +}, common.expectsError({ + code: 'ERR_INVALID_ASYNC_ID', + type: RangeError, +})); assert.throws(() => { new AsyncResource('type', Math.PI); -}, /^RangeError: triggerAsyncId must be an unsigned integer$/); +}, common.expectsError({ + code: 'ERR_INVALID_ASYNC_ID', + type: RangeError, +})); diff --git a/test/parallel/test-async-wrap-constructor.js b/test/parallel/test-async-wrap-constructor.js index 4f344fd99bcff4..86fce0e3f39e68 100644 --- a/test/parallel/test-async-wrap-constructor.js +++ b/test/parallel/test-async-wrap-constructor.js @@ -1,8 +1,8 @@ 'use strict'; -require('../common'); // This tests that using falsy values in createHook throws an error. +const common = require('../common'); const assert = require('assert'); const async_hooks = require('async_hooks'); @@ -10,6 +10,9 @@ for (const badArg of [0, 1, false, true, null, 'hello']) { for (const field of ['init', 'before', 'after', 'destroy']) { assert.throws(() => { async_hooks.createHook({ [field]: badArg }); - }, new RegExp(`^TypeError: ${field} must be a function$`)); + }, common.expectsError({ + code: 'ERR_ASYNC_CALLBACK', + type: TypeError, + })); } } diff --git a/test/parallel/test-internal-errors.js b/test/parallel/test-internal-errors.js index 8df0780e04055c..81b9bac5704e37 100644 --- a/test/parallel/test-internal-errors.js +++ b/test/parallel/test-internal-errors.js @@ -260,3 +260,15 @@ assert.strictEqual( errors.message('ERR_UNESCAPED_CHARACTERS', ['Request path']), 'Request path contains unescaped characters' ); + + +// Test error messages for async_hooks +assert.strictEqual( + errors.message('ERR_ASYNC_CALLBACK', ['init']), + 'init must be a function'); +assert.strictEqual( + errors.message('ERR_ASYNC_TYPE', [{}]), + 'Invalid name for async "type": [object Object]'); +assert.strictEqual( + errors.message('ERR_INVALID_ASYNC_ID', ['asyncId', undefined]), + 'Invalid asyncId value: undefined');