Address feedback

nodejs · Feb 9, 2021 · 361d583 · 361d583
1 parent 9e10007
commit 361d583
Show file tree

Hide file tree

Showing 2 changed files with 29 additions and 27 deletions.
diff --git a/src/node_task_queue.cc b/src/node_task_queue.cc
@@ -25,7 +25,6 @@ using v8::kPromiseResolveAfterResolved;
 using v8::Local;
 using v8::Maybe;
 using v8::Number;
-using v8::Nothing;
 using v8::Object;
 using v8::Promise;
 using v8::PromiseRejectEvent;
@@ -37,7 +36,7 @@ static Maybe<double> GetAssignedPromiseAsyncId(Environment* env,
                                                Local<Value> id_symbol) {
   Local<Value> maybe_async_id;
   if (!promise->Get(env->context(), id_symbol).ToLocal(&maybe_async_id)) {
-    return Nothing<double>();
+    return v8::Just(AsyncWrap::kInvalidAsyncId);
   }
   return maybe_async_id->IsNumber()
       ? maybe_async_id->NumberValue(env->context())
@@ -54,15 +53,15 @@ static Maybe<double> GetAssignedPromiseWrapAsyncId(Environment* env,
   Local<Value> promiseWrap = promise->GetInternalField(0);
   if (promiseWrap->IsObject()) {
         Local<Value> maybe_async_id;
-    if (!promiseWrap.As<Object>()->Get(env->context(), id_symbol).ToLocal(&maybe_async_id)) {
-      return Nothing<double>();
+    if (!promiseWrap.As<Object>()->Get(env->context(), id_symbol)
+        .ToLocal(&maybe_async_id)) {
+      return v8::Just(AsyncWrap::kInvalidAsyncId);
     }
     return maybe_async_id->IsNumber()
         ? maybe_async_id->NumberValue(env->context())
         : v8::Just(AsyncWrap::kInvalidAsyncId);
-  }
-  else {
-      return Nothing<double>();
+  } else {
+      return v8::Just(AsyncWrap::kInvalidAsyncId);
   }
 }
 
@@ -116,28 +115,40 @@ void PromiseRejectCallback(PromiseRejectMessage message) {
 
   double async_id = AsyncWrap::kInvalidAsyncId;
   double trigger_async_id = AsyncWrap::kInvalidAsyncId;
-  GetAssignedPromiseAsyncId(env, promise, env->async_id_symbol())
-          .To(&async_id);
-  GetAssignedPromiseAsyncId(env, promise, env->trigger_async_id_symbol())
-          .To(&trigger_async_id);
-
-  GetAssignedPromiseWrapAsyncId(env, promise, env->async_id_symbol())
-            .To(&async_id);
-  GetAssignedPromiseWrapAsyncId(env, promise, env->trigger_async_id_symbol())
-          .To(&trigger_async_id);
-
+  TryCatchScope try_catch_async_id(env);
+
+  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) {
+    // That means that promise might be a PromiseWrap, so we'll
+    // check there as well.
+    if (!GetAssignedPromiseWrapAsyncId(env, promise, env->async_id_symbol())
+              .To(&async_id)) return;
+    if (!GetAssignedPromiseWrapAsyncId(
+          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);
   }
 
+  if (try_catch_async_id.HasCaught()) {
+    // What must be done here?
+  }
+
   TryCatchScope try_catch(env);
   USE(callback->Call(
       env->context(), Undefined(isolate), arraysize(args), args));
 
   if (async_id != AsyncWrap::kInvalidAsyncId &&
-      trigger_async_id != AsyncWrap::kInvalidAsyncId && 
+      trigger_async_id != AsyncWrap::kInvalidAsyncId &&
       env->execution_async_id() == async_id) {
     // This condition might not be true if async_hooks was enabled during
     // the promise callback execution.

diff --git a/test/async-hooks/test-unhandled-rejection-context.js b/test/async-hooks/test-unhandled-rejection-context.js
@@ -19,18 +19,9 @@ const hooks = initHooks({
 });
 
 hooks.enable();
-
-// Promise.resolve()
-// .then(() => {
-//   throw new Error('Throwing on purpose');
-// });
-
 Promise.reject();
 
 process.on('unhandledRejection', common.mustCall(() => {
-  // assert.strictEqual(promiseAsyncIds.length, 2);
-  // assert.strictEqual(async_hooks.executionAsyncId(), promiseAsyncIds[1]);
-
   assert.strictEqual(promiseAsyncIds.length, 1);
   assert.strictEqual(async_hooks.executionAsyncId(), promiseAsyncIds[0]);
 }));