Conversation
This is helpful to observe promise resolution without considering the promise as handled. This allows the unhandledRejection event to still be produced if no other resolution handling occurs.
|
Review requested:
|
|
What's the runtime overhead? |
|
No idea. Haven't measured it yet. Just wanted to validate the approach seemed reasonable before pursuing it too far. I could always write an intrinsic for setting the handled marker tough, as I did for AsyncLocalStorage with the ContinuationPreservedEmbedderData interface, if the perf impact turns out to be enough to matter. Just a bit more work. |
| const err2 = new Error('test2'); | ||
| const p2 = Promise.reject(err2); | ||
|
|
||
| observePromise(p2, null, common.mustCall(() => {})); |
There was a problem hiding this comment.
maybe assert on err2 like in test 1 and 3
|
I like the idea. |
|
I'm unclear what use case there would be to make it public though. If you have any ideas, I'd be happy to entertain the idea. My plan was to just try it out internally, see if it works okay, and we can always decide to make it public later if we're happy with it. A lot harder to go the other direction. 😅 |
|
The usecases are mostly the same as If this API is internal only it would imply that such very specific cases would require DC enhancements. As you wrote in the comment "APM and diagnostics tools can use this" - but node.js itself is no APM nor a diagnostics tool - but there are quite a lot such tools using node.js as runtime. But I agree, there is no need to make it public on first iteration. |
|
I like the idea, with the overhead being the biggest blocker. |
I'm looking for feedback on this experiment. This is helpful to observe promise resolution without considering the promise as handled. This allows the unhandledRejection event to still be produced if no other resolution handling occurs. This includes a very small V8 change. Assuming people find the intent of this change reasonable, I would try to get that upstreamed to V8. I just don't want to put in all the effort to make that happen if the thing itself is not likely to be accepted.
The particular use case I have in mind for this is the
tracePromisemethod inTracingChannel. Currently it wraps thenables in aPromise.resolve(...)and then calls thethen(...)on it. This is problematic though as the thenable could have other methods which disappear as the return value becomes a native promise instead. Rather than using chaining there which produces a new and crucially different promise, I would like to return the original promise. The issue is that if we just branch internally for our observation and then return the original that original is considered handled and so would no longer trigger unhandledRejection if no other continuation was attached. This change would provide an interface such that we can observe that result but opt-out of the handled transition.I imagine there might be other use cases for this in other parts of the codebase. This is intended purely as an internal utility, I don't expect we'd actually expose it in the public API anywhere.
What do folks think? Is this a reasonable solution we should be applying?
cc @nodejs/diagnostics @nodejs/v8