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 <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>

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,21 +219,12 @@ bool DomainExit(Environment* env, v8::Local<v8::Object> object) {
 }
 
 
-static bool PreCallbackExecution(AsyncWrap* wrap, bool run_domain_cbs) {
+void AsyncWrap::EmitBefore(Environment* env, double async_id) {
-  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) {
   AsyncHooks* async_hooks = env->async_hooks();
 
-  if (async_hooks->fields()[AsyncHooks::kBefore] > 0) {
+  if (async_hooks->fields()[AsyncHooks::kBefore] == 0)
+    return;
+
   Local<Value> uid = Number::New(env->isolate(), async_id);
   Local<Function> fn = env->async_hooks_before_function();
   TryCatch try_catch(env->isolate());
@@ -241,33 +233,19 @@ bool AsyncWrap::EmitBefore(Environment* env, double async_id) {
   if (ar.IsEmpty()) {
     ClearFatalExceptionHandlers(env);
     FatalException(env->isolate(), try_catch);
-      return false;
+    UNREACHABLE();
   }
-  }
-
-  return true;
 }
 
 
-static bool PostCallbackExecution(AsyncWrap* wrap, bool run_domain_cbs) {
+void AsyncWrap::EmitAfter(Environment* env, double async_id) {
-  if (!AsyncWrap::EmitAfter(wrap->env(), wrap->get_id()))
-    return false;
-
-  if (wrap->env()->using_domains() && run_domain_cbs) {
-    bool is_disposed = DomainExit(wrap->env(), wrap->object());
-    if (is_disposed)
-      return false;
-  }
-
-  return true;
-}
-
-bool 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
+  if (async_hooks->fields()[AsyncHooks::kAfter] == 0)
-  // of _fatalException().
+    return;
-  if (async_hooks->fields()[AsyncHooks::kAfter] > 0) {
+
+  // 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());
@@ -276,11 +254,8 @@ bool AsyncWrap::EmitAfter(Environment* env, double async_id) {
   if (ar.IsEmpty()) {
     ClearFatalExceptionHandlers(env);
     FatalException(env->isolate(), try_catch);
-      return false;
+    UNREACHABLE();
   }
-  }
-
-  return true;
 }
 
 class PromiseWrap : public AsyncWrap {
@@ -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.
@@ -687,18 +662,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());
   }
 

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 void EmitBefore(Environment* env, double id);
-  static bool EmitAfter(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<v8::Object> object);
 bool DomainExit(Environment* env, v8::Local<v8::Object> object);
 

src/node.cc

@@ -1328,8 +1328,9 @@ MaybeLocal<Value> MakeCallback(Environment* env,
                                      asyncContext.trigger_async_id);
 
     if (asyncContext.async_id != 0) {
-      if (!AsyncWrap::EmitBefore(env, asyncContext.async_id))
+      // No need to check a return value because the application will exit if
-        return Local<Value>();
+      // an exception occurs.
+      AsyncWrap::EmitBefore(env, asyncContext.async_id);
     }
 
     ret = callback->Call(env->context(), recv, argc, argv);
@@ -1342,8 +1343,7 @@ MaybeLocal<Value> MakeCallback(Environment* env,
     }
 
     if (asyncContext.async_id != 0) {
-      if (!AsyncWrap::EmitAfter(env, asyncContext.async_id))
+      AsyncWrap::EmitAfter(env, asyncContext.async_id);
-        return Local<Value>();
     }
   }