Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Promise rejection in timeout (versus module-level) treated as unhandled by debugger #53732

Open
rotu opened this issue Jul 4, 2024 · 5 comments
Labels
debugger Issues and PRs related to the debugger subsystem. promises Issues and PRs related to ECMAScript promises. v8 engine Issues and PRs related to the V8 dependency.

Comments

@rotu
Copy link

rotu commented Jul 4, 2024

Version

v22.4.0

Platform

Darwin Hypothesis.local 24.0.0 Darwin Kernel Version 24.0.0: Thu Jun 20 20:36:19 PDT 2024; root:xnu-11215.0.115.501.3~1/RELEASE_ARM64_T8103 arm64

Subsystem

No response

What steps will reproduce the bug?

Create this script:

setTimeout(() => {
  Promise.reject().catch(() => { })
}, 1)

And run it in the debugger with break on Uncaught enabled.

How often does it reproduce? Is there a required condition?

Every time. Note this does not happen if Promise.reject() is at top level instead of wrapped in setTimeout().

What is the expected behavior? Why is that the expected behavior?

It is expected that the debugger does (1) not pause on such a promise or (2) only pauses on such a promise if breakOnException is enabled.

The HTML spec guarantees that promise rejections are not considered unhandled if a handler is then synchronously attached.

What do you see instead?

Node breaks synchronously on the creation of the rejected promise. Similar issues happen when using the VSCode debugger and Chrome debugger.

node inspect promisetest.js
< Debugger listening on ws://127.0.0.1:9229/d18cff1f-6886-4ff6-9a4f-404128bd150a
< For help, see: https://nodejs.org/en/docs/inspector
< 
< Debugger attached.
< 
 ok
Break on start in promisetest.js:1
> 1 setTimeout(() => {
  2   Promise.reject().catch(() => { })
  3 }, 1)
debug> breakOnUncaught
debug> c
promiseRejection in promisetest.js:2
  1 setTimeout(() => {
> 2   Promise.reject().catch(() => { })
  3 }, 1)
  4 

Additional information

A downstream issue where this interferes with usage of the web streams API: #51093

I too have been incredibly confused by this, as it makes it seem like even correct usage of promises is developer error.

VSCode reports this as "Exception has occurred" instead of "promiseRejection".

@RedYetiDev RedYetiDev added promises Issues and PRs related to ECMAScript promises. debugger Issues and PRs related to the debugger subsystem. repro-exists labels Jul 4, 2024
@RedYetiDev
Copy link
Member

RedYetiDev commented Jul 4, 2024

It's reproducible via the VSCode debugger.
image

The debugger treats it as both a caught exception, and an uncaught exception.


It appears to be a debugger specific issue, as it does not emit an uncaughtException (or unhandledRejection) event on the process:

process.on("uncaughtException", () => console.log("uncaughtException")) // This is never called
process.on("unhandledRejection", () => console.log("unhandledRejection")) // This is never called

setTimeout(() => {
    Promise
        .reject("This is a promise rejection")
        .catch(() => console.log("Caught promise rejection")) // This is called
}, 1)

@rotu
Copy link
Author

rotu commented Jul 4, 2024

It appears to be a debugger specific issue, as it does not emit an uncaughtException (or unhandledRejection) event on the process:

Yes, it is a debugger-specific issue caused by V8 "catch prediction". The debugger breaks synchronously when the promise is rejected.

It would be okay if the debugger paused here when breakOnException was specified, but it's inappropriate for breakOnUncaught.

Related discussion:
https://issues.chromium.org/issues/41161875

@RedYetiDev RedYetiDev added the v8 engine Issues and PRs related to the V8 dependency. label Jul 4, 2024
@RedYetiDev
Copy link
Member

@nodejs/v8

@rotu
Copy link
Author

rotu commented Jul 5, 2024

Relatedly, the below code does not trip the debugger (with breakOnUncaught enabled) and does not print anything. I think this situation is dual to the originally reported issue:

  • the rejected promise never has a handler attached
  • but catch prediction incorrectly expects the surrounding try statement to apply to the rejection (it doesn't because the promise is never awaited).
setTimeout(async () => {
  try {
    Promise.reject()
  } catch (e) { }
})

@rotu
Copy link
Author

rotu commented Jul 22, 2024

I think I figured out the setTimeout part of this mystery. When running a module as the main entry point, the code is within in a try block in an async function (in module_job.js).

async run(isEntryPoint = false) {
await this.instantiate();
if (isEntryPoint) {
globalThis[entry_point_module_private_symbol] = this.module;
}
const timeout = -1;
const breakOnSigint = false;
setHasStartedUserESMExecution();
try {
await this.module.evaluate(timeout, breakOnSigint);
} catch (e) {

Because of this try/catch block, NO promise rejection at module level is regarded by the inspector as an "uncaught exception", REGARDLESS OF WHETHER OR NOT IT HAS A HANDLER ATTACHED.

I raised an issue for this catch prediction false positive but it was regarded as "infeasible" here: https://issues.chromium.org/issues/352455689

When code escapes this call stack by running asynchronously, the inspector no longer regards it as handled by that try/catch so it may or may not be considered an "uncaught exception" (based on V8's designed behavior).

Here's a demonstration:

// assume this is in a .mjs file

// this function creates a Promise which is rejected and handled
// This promise is NOT regarded as "caught" by the V8's catch prediction
const makeRejectedPromise = (reason) => {
   Promise.reject(reason).catch(() => {});
};

// rejection regarded as handled created synchronously:
makeRejectedPromise(1);
(async () => {
   makeRejectedPromise(2);
})();
await null;
// or asynchronously after a module-level await
makeRejectedPromise(3);

// rejection regarded as unhandled when created asynchronously:
(async () => {
   await null;
   makeRejectedPromise(4);
})();
queueMicrotask(() => makeRejectedPromise(5));
process.nextTick(() => makeRejectedPromise(6));
setTimeout(() => makeRejectedPromise(7));

@rotu rotu changed the title Promise rejection created in timeout treated as unhandled by debugger Promise rejection created in timeout treated as unhandled by debugger (versus module-level) Jul 22, 2024
@rotu rotu changed the title Promise rejection created in timeout treated as unhandled by debugger (versus module-level) Promise rejection in timeout (versus module-level) treated as unhandled by debugger Jul 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
debugger Issues and PRs related to the debugger subsystem. promises Issues and PRs related to ECMAScript promises. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

No branches or pull requests

3 participants