asyncId for unhandled rejected Promise

[sajal50]

Is your feature request related to a problem? Please describe.
I am trying to track the lifecycle events of a Promise. With async_hooks I am able to get those and each Promise is assigned an asyncId. However, I wish to know the asyncId of a Promise that was rejected and didn't have a catch handler attached to it. I see that with there is process.on('unhandledRejection'), but I don't believe that can give that info.

Describe the solution you'd like
A way to be informed of the asyncId for an unhandled Promise.

[benjamingr]

Hmm, I vaguely recall fixing this for domains at some point so the REPL doesn't crash. This shouldn't be too hard to address.

@sajal50 if I may ask - whay are you using the asyncId for in the unhandledRejection event?

[sajal50]

Effectively the idea is that there our different "entities" running in a single process creating Promises as they see fit. If one of those entities has an unhanded promise rejection I wish to know which entity caused it and take appropriate action like cleaning up the entity. I don't wish the other entities to be affected and terminate the process.

[benjamingr]

@sajal50

I don't think that's safe since the entities can (and probably do) leak things that are "global" in the Node.js process, we should do a better job documenting that.

That said - this sounds like a bug that should be fixed regardless.

The fix should be pretty simple: we have a pool and when we do maybeUnhandledPromises.set we should store the asyncId with it. Let me know if you want to work on this otherwise someone from the async hooks team will likely fix this (eventually) :]

cc @nodejs/async_hooks

Also cc @Linkgoron in case you want to work on this

[sajal50]

I don't think that's safe since the entities can (and probably do) leak things that are "global" in the Node.js process, we should do a better job documenting that.

I agree. My example perhaps was not the best. Just think it'd still be useful.

me know if you want to work on this otherwise someone from the async hooks team will likely fix this (eventually) :]

I would love to take a crack at this. Can try building this weekend and get back.

Thanks for the guidance. :)

[benjamingr]

Assigned to you in the meantime :] Please check out the docs part where it talks about resource pools (I think worker pools are the example used) and the code for making unhandledRejection work with domains at #36082

[sajal50]

Please check out the docs part where it talks about resource pools (I think worker pools are the example used)

Sorry, where is this doc exactly?

[benjamingr]

@sajal50 https://nodejs.org/api/async_hooks.html#async_hooks_using_asyncresource_for_a_worker_thread_pool

[sajal50]

Alright. Thanks, @benjamingr. Will take a look.

[benjamingr]

A simple solution could look something like (I haven't tested):

diff --git a/lib/internal/process/promises.js b/lib/internal/process/promises.js
index 023f7df036..8fed595bb9 100644
--- a/lib/internal/process/promises.js
+++ b/lib/internal/process/promises.js
@@ -24,6 +24,8 @@ const {
   triggerUncaughtException
 } = internalBinding('errors');
 
+const asyncHooks = require('async_hooks');
+
 // *Must* match Environment::TickInfo::Fields in src/env.h.
 const kHasRejectionToWarn = 1;
 
@@ -116,11 +118,18 @@ function resolveError(type, promise, reason) {
 }
 
 function unhandledRejection(promise, reason) {
+  const emit = asyncHooks.bind((reason, promise, promiseInfo) => {
+    if (promiseInfo.domain) {
+      return promiseInfo.domain.emit('error', reason);
+    }
+    return process.emit('unhandledRejection', reason, promise);
+  });
   maybeUnhandledPromises.set(promise, {
     reason,
     uid: ++lastPromiseId,
     warned: false,
-    domain: process.domain
+    domain: process.domain,
+    emit
   });
   // This causes the promise to be referenced at least for one tick.
   ArrayPrototypePush(pendingUnhandledRejections, promise);
@@ -194,13 +203,7 @@ function processPromiseRejections() {
       continue;
     }
     promiseInfo.warned = true;
-    const { reason, uid } = promiseInfo;
-    function emit(reason, promise, promiseInfo) {
-      if (promiseInfo.domain) {
-        return promiseInfo.domain.emit('error', reason);
-      }
-      return process.emit('unhandledRejection', reason, promise);
-    }
+    const { reason, uid, emit } = promiseInfo;
     switch (unhandledRejectionsMode) {
       case kStrictUnhandledRejections: {
         const err = reason instanceof Error ?

[sajal50]

+  const emit = asyncHooks.bind((reason, promise, promiseInfo) => {
+    if (promiseInfo.domain) {
+      return promiseInfo.domain.emit('error', reason);
+    }
+    return process.emit('unhandledRejection', reason, promise);
+  });

Hmm, this doesn't work. asyncHooks is not a function.

we have a pool and when we do maybeUnhandledPromises.set we should store the asyncId with it.

I was trying to follow this. I thought I could extract async_id_symbol from the promise we receive in unhandledRejection function. But that seems to be undefined. Looks like I am missing something.

---

UPDATE: Perhaps you meant AsyncResource.bind?

[sajal50]

diff --git a/lib/internal/process/promises.js b/lib/internal/process/promises.js
index 023f7df036..0e5d1181cb 100644
--- a/lib/internal/process/promises.js
+++ b/lib/internal/process/promises.js
@@ -24,6 +24,9 @@ const {
   triggerUncaughtException
 } = internalBinding('errors');
 
+const { async_id_symbol,
+  trigger_async_id_symbol } = internalBinding('symbols');
+
 // *Must* match Environment::TickInfo::Fields in src/env.h.
 const kHasRejectionToWarn = 1;
 
@@ -120,7 +123,8 @@ function unhandledRejection(promise, reason) {
     reason,
     uid: ++lastPromiseId,
     warned: false,
-    domain: process.domain
+    domain: process.domain,
+    asyncId: promise[async_id_symbol]
   });
   // This causes the promise to be referenced at least for one tick.
   ArrayPrototypePush(pendingUnhandledRejections, promise);
@@ -194,12 +198,12 @@ function processPromiseRejections() {
       continue;
     }
     promiseInfo.warned = true;
-    const { reason, uid } = promiseInfo;
+    const { reason, uid, asyncId } = promiseInfo;
     function emit(reason, promise, promiseInfo) {
       if (promiseInfo.domain) {
         return promiseInfo.domain.emit('error', reason);
       }
-      return process.emit('unhandledRejection', reason, promise);
+      return process.emit('unhandledRejection', reason, promise, asyncId);
     }
     switch (unhandledRejectionsMode) {
       case kStrictUnhandledRejections: {

I had this working. Basically, I see lib/internal/async_hooks.js attaching the asyncIds to the Promise object, and I extracted that. This might not be the ideal thing to do. For one, this only works destroy async_hook is not set.

[benjamingr]

UPDATE: Perhaps you meant AsyncResource.bind?

Yeah

[sajal50]

@benjamingr But doesn't that lead to a new AsyncResource being created? I thought the intention is that the execution context (at least the async_hooks.executionAsyncId()) is of the rejected Promise when process.on('unhandledRejection')'s callback is called.

At least with this draft change I am able to get the correct executionAsyncId for the unhandledRejection method. However, I am not sure of how to preserve this context over the hop when ultimately process.on('unhandledRejection')'s callback is called.

UPDATE:
Actually my bad. Even without the change, the correct executionAsyncId in unhandledRejection method.

[sajal50]

So, I was able to make this commit work. I get the correct executionAsyncId() in process.on('unhandledRejection') callback.

I can raise a draft PR if this a decent starting point to discuss further.

[benjamingr]

@sajal50 I think this looks good, honestly the only thing missing here is a test :]

[sajal50]

Oh, that's awesome. 😃

I am not super familiar with node's code organization. Do I just add the test case as a new file in https://github.com/nodejs/node/tree/master/test/async-hooks?

[benjamingr]

@sajal50 probably just add to test-promises-unhandled-rejections.js inside test/parallel? Alternatively what you suggested - adding a new file to test/parallel (or test/async-hooks) should be fine.

[benjamingr]

Fixed in #37281 (comment) - thank you for contributing a fix :]