Repository navigation
asyncId for unhandled rejected Promise #37244
Description
Activity
- changed the title
[-]asyncId for rejected Promise[/-][+]asyncId for unhandled rejected Promise[/+]on Feb 6, 2021 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
unhandledRejectionevent?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.
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.setwe 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
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. :)
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
unhandledRejectionwork with domains at #36082Please 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?
Alright. Thanks, @benjamingr. Will take a look.
Reacted by Benjamin GruenbaumA 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 ?
+ 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.
asyncHooksis 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_symbolfrom thepromisewe receive inunhandledRejectionfunction. But that seems to beundefined. Looks like I am missing something.
UPDATE: Perhaps you meant
AsyncResource.bind?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.jsattaching theasyncIds to thePromiseobject, and I extracted that. This might not be the ideal thing to do. For one, this only worksdestroyasync_hook is not set.UPDATE: Perhaps you meant AsyncResource.bind?
Yeah
@benjamingr But doesn't that lead to a new
AsyncResourcebeing created? I thought the intention is that the execution context (at least theasync_hooks.executionAsyncId()) is of the rejectedPromisewhenprocess.on('unhandledRejection')'s callback is called.At least with this draft change I am able to get the correct
executionAsyncIdfor theunhandledRejectionmethod. However, I am not sure of how to preserve this context over the hop when ultimatelyprocess.on('unhandledRejection')'s callback is called.UPDATE:
Actually my bad. Even without the change, the correctexecutionAsyncIdinunhandledRejectionmethod.@sajal50 I think this looks good, honestly the only thing missing here is a test :]
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?
Reacted by Benjamin Gruenbaum@sajal50 probably just add to
test-promises-unhandled-rejections.jsinside test/parallel? Alternatively what you suggested - adding a new file to test/parallel (or test/async-hooks) should be fine.Reacted by Sajal KhandelwalFixed in #37281 (comment) - thank you for contributing a fix :]
Is your feature request related to a problem? Please describe.
I am trying to track the lifecycle events of a
Promise. Withasync_hooksI am able to get those and eachPromiseis assigned anasyncId. However, I wish to know theasyncIdof aPromisethat was rejected and didn't have acatchhandler attached to it. I see that with there isprocess.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
asyncIdfor an unhandledPromise.