Closed r-cyr closed 1 week ago
I decided to dive in the code and found that if I replace the PromiseReject
implementation from the default one in primordials
to the following hack (shown in the chromium bug tracker link in my previous post), then it works as expected. I don't know anything about the NodeJS codebase so I have no idea if using the hack has possible unintended side-effects.
The hack being simply:
function PromiseReject(e) {
try {
return primordials.PromiseReject(e);
} catch {
// Empty on purpose
}
}
By spec, Promise.reject
should never, ever, ever synchronously throw under any circumstances, and that hack implies it does, so that sounds like a pretty bad scenario; https://bugs.chromium.org/p/chromium/issues/detail?id=465666&q=uncaught%20promise%20pause&can=2 is closed as wontfix. I do see that it's just about doing "catch detection", so it's not that it actually throws.
Another alternative could be async function PromiseReject(e) { throw e; }
but i think that'd mess with node's callsite detection; and another might be creating the rejected promise in C++ so that the callsite info is preserved?
This is an incredibly annoying error. I'm managing to work around it by adding this condition to the uncaught exception breakpoint:
!error.stack.match('node:internal/webstreams/writablestream:101') &&
!error.stack.match('node:internal/webstreams/adapters:110') &&
!error.stack.match('node:internal/webstreams/readablestream:158')
Are there any better userland workarounds?
The hack being simply:
function PromiseReject(e) { try { return primordials.PromiseReject(e); } catch { // Empty on purpose } }
By spec,
Promise.reject
should never, ever, ever synchronously throw under any circumstances, and that hack implies it does, so that sounds like a pretty bad scenario; https://bugs.chromium.org/p/chromium/issues/detail?id=465666&q=uncaught%20promise%20pause&can=2 is closed as wontfix. I do see that it's just about doing "catch detection", so it's not that it actually throws.
It looks like this hack "worked" because of this catch detection issue in particular (now fixed): https://issues.chromium.org/issues/40283987
According to @rotu comment above, the problem has been fixed in V8 version 12.3. I'm going to close this issue as the "fix" is to upgrade to Node 22 (with V8 12.4) which is scheduled to become the next LTS version this October
Misunderstood what the fix was doing, the original problem is unfortunately, not fixed. (Reopening)
@r-cyr sorry I was not clear. The catch{…}
should no longer affect debugger behavior. But I don’t know if the actual issue (the debugger spuriously thinking a rejection is unhandled) is fixed.
I just retried your original repro code.
It is NOT treated as an uncaught exception by the VSCode debugger (VSCode 1.91.0-insider). 🎉 It is still show a problem in Chrome (Version 128.0.6544.0 (Official Build) canary (64-bit). 😞
May be fixed by https://github.com/nodejs/node/pull/53800
May be fixed by https://github.com/nodejs/node/pull/53800
Closing, as that PR has landed. If you disagree, feel free to reopen.
I still see "uncaught" rejections from writableStreamDefaultWriterEnsureReadyPromiseRejected
when closing a TCP socket with wrapped in stream.Duplex.toWeb
https://github.com/nodejs/node/blob/7168295e7a94e6cfef69e43d26d7d5b57d3e1cf9/lib/internal/webstreams/writablestream.js#L1044
Any reason this wasn't included in #53800?
Version
v20.10.0, v22.3.0
Platform
MacOS/WSL
Subsystem
No response
What steps will reproduce the bug?
Run the following code into a debugger (Inside VSCode or in Chrome) with
Pause on uncaught exceptions
checked/enabled:How often does it reproduce? Is there a required condition?
Always
What is the expected behavior? Why is that the expected behavior?
When run in a debugger with
Pause on uncaught exceptions
checked, program should print "Hello world" then quit.What do you see instead?
The program will execute as expected then when the stream gets destroyed at the end, the debugger will pause after a second and show the following error as an unhandled rejection:
The program runs as expected if
Pause on uncaught exceptions
is unchecked/disabled.Additional information
The call stack shows that the error comes from https://github.com/nodejs/node/blob/main/lib/internal/webstreams/writablestream.js#L1033 but we can see that
setPromiseHandled
gets called a few lines after so I don't believe that error to be correct. While looking for possible reasons, I ended up finding this which may be the root causeI also tried listening to
process
events 'uncaughtException' and 'unhandledrejection' but those were never triggered.If it's really caused by the debugger, then I don't know if it's possible to refactor the function in a way that would make debuggers happy... (and me too since we started using webstreams a lot at work... and getting that error all the time when debugging is painful)
Worth noting that the function below it, named
writableStreamDefaultWriterEnsureClosedPromiseRejected
is written similarly towritableStreamDefaultWriterEnsureReadyPromiseRejected
and could, maybe, produce the same issue. (I didn't try to trigger it)