diff --git a/lib/async_hooks.js b/lib/async_hooks.js index cb07938039052b..61b9d51e8ab000 100644 --- a/lib/async_hooks.js +++ b/lib/async_hooks.js @@ -1,39 +1,59 @@ '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. +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. * - * 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 +62,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 +76,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 +97,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); @@ -83,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; @@ -121,7 +147,7 @@ class AsyncHook { hooks_array.push(this); if (prev_kTotals === 0 && hook_fields[kTotals] > 0) - async_wrap.enablePromiseHook(); + enablePromiseHook(); return this; } @@ -143,7 +169,7 @@ class AsyncHook { hooks_array.splice(index, 1); if (prev_kTotals > 0 && hook_fields[kTotals] === 0) - async_wrap.disablePromiseHook(); + disablePromiseHook(); return this; } @@ -151,41 +177,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; } @@ -210,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; @@ -305,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); } @@ -322,25 +354,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', { @@ -351,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); @@ -367,10 +408,12 @@ 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 (!Number.isSafeInteger(asyncId) || asyncId < -1) { + fatalError( + new errors.RangeError('ERR_INVALID_ASYNC_ID', 'asyncId', asyncId)); + } + if (async_hook_fields[kAfter] > 0) emitAfterNative(asyncId); @@ -379,34 +422,28 @@ 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); } -// 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 +452,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/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/async-wrap.cc b/src/async-wrap.cc index 86759bc62c63b4..7630de3694df99 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; @@ -164,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()); @@ -217,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 { @@ -372,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. @@ -686,19 +662,28 @@ MaybeLocal AsyncWrap::MakeCallback(const Local cb, get_id(), get_trigger_id()); - if (!PreCallbackExecution(this, true)) { - return MaybeLocal(); + // 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)) { - return Local(); + 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()); } exec_scope.Dispose(); 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/env-inl.h b/src/env-inl.h index f3ca8882b033cb..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] }); @@ -176,13 +176,14 @@ 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()) { + CHECK_GE(init_trigger_id, -1); + 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( @@ -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/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); 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); } } 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-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'); +})); 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');