From b7a23295e84479843c4975bd6ed876392a9d7602 Mon Sep 17 00:00:00 2001
From: Andrey Pechkurov <apechkurov@gmail.com>
Date: Tue, 28 Jul 2020 22:36:40 +0300
Subject: async_hooks: fix id assignment in fast-path promise hook
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Native side of fast-path promise hook was not calling JS
fastPromiseHook function when there were no async ids
previously assigned to the promise. Because of that already
created promises could not get id assigned in situations
when an async hook without a before listener function is
enabled after their creation. As the result executionAsyncId
could return wrong id when called within promise's .then().

Refs: https://github.com/nodejs/node/pull/34512

PR-URL: https://github.com/nodejs/node/pull/34548
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
---
 src/async_wrap.cc | 82 +++++++++++++++++++++++++++++--------------------------
 1 file changed, 44 insertions(+), 38 deletions(-)

(limited to 'src/async_wrap.cc')

diff --git a/src/async_wrap.cc b/src/async_wrap.cc
index 05c63cbc2db..b03c6600873 100644
--- a/src/async_wrap.cc
+++ b/src/async_wrap.cc
@@ -39,9 +39,12 @@ using v8::Global;
 using v8::HandleScope;
 using v8::Integer;
 using v8::Isolate;
+using v8::Just;
 using v8::Local;
+using v8::Maybe;
 using v8::MaybeLocal;
 using v8::Name;
+using v8::Nothing;
 using v8::Number;
 using v8::Object;
 using v8::ObjectTemplate;
@@ -189,6 +192,21 @@ void AsyncWrap::EmitAfter(Environment* env, double async_id) {
        env->async_hooks_after_function());
 }
 
+// TODO(addaleax): Remove once we're on C++17.
+constexpr double AsyncWrap::kInvalidAsyncId;
+
+static Maybe<double> GetAssignedPromiseAsyncId(Environment* env,
+                                               Local<Promise> promise,
+                                               Local<Value> id_symbol) {
+  Local<Value> maybe_async_id;
+  if (!promise->Get(env->context(), id_symbol).ToLocal(&maybe_async_id)) {
+    return Nothing<double>();
+  }
+  return maybe_async_id->IsNumber()
+      ? maybe_async_id->NumberValue(env->context())
+      : Just(AsyncWrap::kInvalidAsyncId);
+}
+
 class PromiseWrap : public AsyncWrap {
  public:
   PromiseWrap(Environment* env, Local<Object> object, bool silent)
@@ -231,18 +249,17 @@ PromiseWrap* PromiseWrap::New(Environment* env,
 
   // Skip for init events
   if (silent) {
-    Local<Value> maybe_async_id = promise
-        ->Get(context, env->async_id_symbol())
-        .ToLocalChecked();
-
-    Local<Value> maybe_trigger_async_id = promise
-        ->Get(context, env->trigger_async_id_symbol())
-        .ToLocalChecked();
-
-    if (maybe_async_id->IsNumber() && maybe_trigger_async_id->IsNumber()) {
-      double async_id = maybe_async_id.As<Number>()->Value();
-      double trigger_async_id = maybe_trigger_async_id.As<Number>()->Value();
-      return new PromiseWrap(env, obj, async_id, trigger_async_id);
+    double async_id;
+    double trigger_async_id;
+    if (!GetAssignedPromiseAsyncId(env, promise, env->async_id_symbol())
+            .To(&async_id)) return nullptr;
+    if (!GetAssignedPromiseAsyncId(env, promise, env->trigger_async_id_symbol())
+            .To(&trigger_async_id)) return nullptr;
+
+    if (async_id != AsyncWrap::kInvalidAsyncId &&
+        trigger_async_id != AsyncWrap::kInvalidAsyncId) {
+      return new PromiseWrap(
+          env, obj, async_id, trigger_async_id);
     }
   }
 
@@ -321,46 +338,35 @@ static void FastPromiseHook(PromiseHookType type, Local<Promise> promise,
 
   if (type == PromiseHookType::kBefore &&
       env->async_hooks()->fields()[AsyncHooks::kBefore] == 0) {
-    Local<Value> maybe_async_id;
-    if (!promise->Get(context, env->async_id_symbol())
-          .ToLocal(&maybe_async_id)) {
-      return;
-    }
-
-    Local<Value> maybe_trigger_async_id;
-    if (!promise->Get(context, env->trigger_async_id_symbol())
-          .ToLocal(&maybe_trigger_async_id)) {
-      return;
-    }
-
-    if (maybe_async_id->IsNumber() && maybe_trigger_async_id->IsNumber()) {
-      double async_id = maybe_async_id.As<Number>()->Value();
-      double trigger_async_id = maybe_trigger_async_id.As<Number>()->Value();
+    double async_id;
+    double trigger_async_id;
+    if (!GetAssignedPromiseAsyncId(env, promise, env->async_id_symbol())
+            .To(&async_id)) return;
+    if (!GetAssignedPromiseAsyncId(env, promise, env->trigger_async_id_symbol())
+            .To(&trigger_async_id)) return;
+
+    if (async_id != AsyncWrap::kInvalidAsyncId &&
+        trigger_async_id != AsyncWrap::kInvalidAsyncId) {
       env->async_hooks()->push_async_context(
           async_id, trigger_async_id, promise);
+      return;
     }
-
-    return;
   }
 
   if (type == PromiseHookType::kAfter &&
       env->async_hooks()->fields()[AsyncHooks::kAfter] == 0) {
-    Local<Value> maybe_async_id;
-    if (!promise->Get(context, env->async_id_symbol())
-          .ToLocal(&maybe_async_id)) {
-      return;
-    }
+    double async_id;
+    if (!GetAssignedPromiseAsyncId(env, promise, env->async_id_symbol())
+            .To(&async_id)) return;
 
-    if (maybe_async_id->IsNumber()) {
-      double async_id = maybe_async_id.As<Number>()->Value();
+    if (async_id != AsyncWrap::kInvalidAsyncId) {
       if (env->execution_async_id() == async_id) {
         // This condition might not be true if async_hooks was enabled during
         // the promise callback execution.
         env->async_hooks()->pop_async_context(async_id);
       }
+      return;
     }
-
-    return;
   }
 
   if (type == PromiseHookType::kResolve &&
-- 
cgit v1.2.3