Skip to content

Commit

Permalink
async_wrap: unroll unnecessarily DRY code
Browse files Browse the repository at this point in the history 
* 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: #14722
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
  • Loading branch information
@trevnorris @MylesBorins
trevnorris authored and MylesBorins committed Sep 12, 2017
1 parent 3e73ea8 commit 78a36e0
Show file tree
Hide file tree
Showing 3 changed files with 50 additions and 65 deletions.
102 changes: 43 additions & 59 deletions src/async-wrap.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down Expand Up @@ -218,69 +219,43 @@ bool DomainExit(Environment* env, v8::Local<v8::Object> 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<Value> uid = Number::New(env->isolate(), async_id);
Local<Function> fn = env->async_hooks_before_function();
TryCatch try_catch(env->isolate());
MaybeLocal<Value> 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<Value> uid = Number::New(env->isolate(), async_id);
Local<Function> fn = env->async_hooks_before_function();
TryCatch try_catch(env->isolate());
MaybeLocal<Value> 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<Value> uid = Number::New(env->isolate(), async_id);
Local<Function> fn = env->async_hooks_after_function();
TryCatch try_catch(env->isolate());
MaybeLocal<Value> 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<Value> uid = Number::New(env->isolate(), async_id);
Local<Function> fn = env->async_hooks_after_function();
TryCatch try_catch(env->isolate());
MaybeLocal<Value> 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 {
Expand Down Expand Up @@ -373,9 +348,9 @@ static void PromiseHook(PromiseHookType type, Local<Promise> 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.
Expand Down Expand Up @@ -696,18 +671,27 @@ MaybeLocal<Value> AsyncWrap::MakeCallback(const Local<Function> 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<Value> 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());
}

Expand Down
5 changes: 3 additions & 2 deletions src/async-wrap.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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<v8::Object> object);
bool DomainExit(Environment* env, v8::Local<v8::Object> object);

Expand Down
8 changes: 4 additions & 4 deletions src/node.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1334,8 +1334,9 @@ MaybeLocal<Value> MakeCallback(Environment* env,
asyncContext.trigger_async_id);

if (asyncContext.async_id != 0) {
if (!AsyncWrap::EmitBefore(env, asyncContext.async_id))
return Local<Value>();
// 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);
Expand All @@ -1348,8 +1349,7 @@ MaybeLocal<Value> MakeCallback(Environment* env,
}

if (asyncContext.async_id != 0) {
if (!AsyncWrap::EmitAfter(env, asyncContext.async_id))
return Local<Value>();
AsyncWrap::EmitAfter(env, asyncContext.async_id);
}
}

Expand Down

0 comments on commit 78a36e0

Please sign in to comment.