From 78a36e0dd143ce385848b73ca3740728d84d4dff Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Thu, 3 Aug 2017 17:03:41 -0600 Subject: [PATCH] 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 Reviewed-By: Anna Henningsen Reviewed-By: Refael Ackermann --- 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 8f513ba4524795..71b1e256592b08 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. @@ -696,18 +671,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 5bc48d7faf0c30..10d0150844a7ab 100644 --- a/src/async-wrap.h +++ b/src/async-wrap.h @@ -113,8 +113,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; @@ -148,6 +148,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 3c9a6e6ce65743..34785693c8a7ad 100644 --- a/src/node.cc +++ b/src/node.cc @@ -1334,8 +1334,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); @@ -1348,8 +1349,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); } }