lib: add observePromise helper by Qard · Pull Request #62391 · nodejs/node

Qard · 2026-03-22T12:21:50Z

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 tracePromise method in TracingChannel. Currently it wraps thenables in a Promise.resolve(...) and then calls the then(...) 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

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.

nodejs-github-bot · 2026-03-22T12:21:56Z

Review requested:

@nodejs/loaders
@nodejs/security-wg
@nodejs/v8-update

mcollina · 2026-03-22T15:25:51Z

What's the runtime overhead?

Qard · 2026-03-22T17:42:48Z

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.

Flarna · 2026-03-22T22:00:27Z

test/parallel/test-promise-observe.js

+  const err2 = new Error('test2');
+  const p2 = Promise.reject(err2);
+
+  observePromise(p2, null, common.mustCall(() => {}));


maybe assert on err2 like in test 1 and 3

Flarna · 2026-03-22T22:03:13Z

I like the idea.
But I think it is actually useful for userland - like you wrote "APM and diagnostics tools can use this...".
If it is not expose they can't use it. Only via tracePromise once it is added there but I think this is an unneeded restriction.

Qard · 2026-03-22T22:25:52Z

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. 😅

Flarna · 2026-03-23T07:04:02Z

The usecases are mostly the same as tracePromise in diagnostic channel.
tracePromise does not fit all possible cases so users/apm tools/... might want to implement a their own variant fitting to their concrete needed.

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.

mcollina · 2026-03-23T08:45:29Z

I like the idea, with the overhead being the biggest blocker.

Qard · 2026-03-23T11:49:14Z

Would people be averse to landing this basically as-is and separately working on a PR to make use of it in tracePromise where we can get some more real-world benchmark results? For either approach (exposing the setter for the handled marker in C++ or exposing it via a JS intrinsic) it will require V8 changes. The current approach is the simplest and possibly small enough it might be okay to float a patch for now? The intrinsic change would be a bit more involved and would likely be harder to get V8 folks to agree to it.

Alternatively, I could expand this PR to integrate the change into diagnostics_channel here and we can aim for landing the whole thing together. That way we could have some benchmark numbers before we land anything. I'm not too worried about the perf landing this in a disconnected state though.

addaleax · 2026-03-23T13:52:41Z

deps/v8/include/v8-promise.h

+  /**
+   * Marks this promise as unhandled, re-enabling unhandled rejection tracking.
+   */
+  void MarkAsUnhandled();


I'm personally okay with a patch like this, but I imagine it would make things a bit easier if it was in a separate commit

addaleax · 2026-03-23T13:55:12Z

src/node_task_queue.cc

+  // Set flag BEFORE .Then() so that if V8 fires kPromiseHandlerAddedAfterReject
+  // synchronously (because the promise is already rejected), PromiseRejectCallback
+  // suppresses it and the promise stays in pendingUnhandledRejections.
+  env->set_observing_promise(true);
+
+  Local<Promise> derived;
+  if (!promise->Then(env->context(), on_fulfilled, on_rejected)
+           .ToLocal(&derived)) {
+    env->set_observing_promise(false);
+    return;
+  }


Not substantially different, but imo this is a bit easier to read if it's clear that the observing_promise toggle is a scope:

Suggested change

// Set flag BEFORE .Then() so that if V8 fires kPromiseHandlerAddedAfterReject

// synchronously (because the promise is already rejected), PromiseRejectCallback

// suppresses it and the promise stays in pendingUnhandledRejections.

env->set_observing_promise(true);

Local<Promise> derived;

if (!promise->Then(env->context(), on_fulfilled, on_rejected)

.ToLocal(&derived)) {

env->set_observing_promise(false);

return;

}

Local<Promise> derived;

{

// Set flag BEFORE .Then() so that if V8 fires kPromiseHandlerAddedAfterReject

// synchronously (because the promise is already rejected), PromiseRejectCallback

// suppresses it and the promise stays in pendingUnhandledRejections.

env->set_observing_promise(true);

auto defer_reset = OnScopeLeave([&]() { env->set_observing_promise(false); });

if (!promise->Then(env->context(), on_fulfilled, on_rejected)

.ToLocal(&derived)) {

return;

}

}

jasnell · 2026-03-23T13:58:05Z

doc/api/packages.md

Nit.. unrelated edit?

Yeah, my git has been acting strangely with this file for some reason. It keeps insisting there's a line feed change even if I reset it to whatever is in main. 😕

jasnell · 2026-03-23T14:00:37Z

lib/internal/promise_observe.js

+ * Observes a promise's fulfillment or rejection without suppressing
+ * the `unhandledRejection` event. APM and diagnostics tools can use
+ * this to be notified of promise outcomes while still allowing
+ * `unhandledRejection` to fire if no real handler exists.


It would be helpful here to briefly explain the timing expectation. e.g. for "Called when the promise fulfills/rejects", does that follow the same timing as microtask queue? Does it happen synchronously when the promise hooks fire, etc. If the latter, I'd be very concerned about landing this.

It's also very not clear: would the onFulfilled/onRejected callbacks receive the settled value/error?

Qard · 2026-03-23T14:08:51Z

I thought of a better solution. Closing in favour of #62407 which simply branches rather than chains only for values which are not native promises, meaning a thenable. That preserves the unhandledRejection behaviour this was trying to resolve while still allowing thenables to work correctly which this PR actually doesn't resolve as it'd need to be converted to a native promise to have the handled state anyway. 🤦🏻

lib: add observePromise helper

a49a1b2

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.

Qard self-assigned this Mar 22, 2026

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Mar 22, 2026

Flarna reviewed Mar 22, 2026

View reviewed changes

addaleax approved these changes Mar 23, 2026

View reviewed changes

jasnell reviewed Mar 23, 2026

View reviewed changes

Qard closed this Mar 23, 2026

Qard deleted the observe-promise-without-handled-state branch March 23, 2026 14:09

Uh oh!

Conversation

Qard commented Mar 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Mar 22, 2026

Uh oh!

mcollina commented Mar 22, 2026

Uh oh!

Qard commented Mar 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Flarna Mar 22, 2026

Choose a reason for hiding this comment

Uh oh!

Flarna commented Mar 22, 2026

Uh oh!

Qard commented Mar 22, 2026

Uh oh!

Flarna commented Mar 23, 2026

Uh oh!

mcollina commented Mar 23, 2026

Uh oh!

Qard commented Mar 23, 2026

Uh oh!

addaleax Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

addaleax Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

jasnell Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

Qard Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

jasnell Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

Qard commented Mar 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Qard commented Mar 22, 2026 •

edited

Loading

Qard commented Mar 22, 2026 •

edited

Loading