stream/web internal function correct implementation leads to debuggers incorrectly detecting an unhandled rejection

[r-cyr]

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:

import { ReadableStream, WritableStream } from "stream/web";

await new ReadableStream({
  async start(controller) {
    controller.enqueue('Hello world');
    controller.close();
  },
}).pipeTo(
  new WritableStream({
    write(elem) {
      console.log(elem);
    },
  })
);

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:

TypeError [ERR_INVALID_STATE]: Invalid state: Writer has been released

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 cause

I 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 to writableStreamDefaultWriterEnsureReadyPromiseRejected and could, maybe, produce the same issue. (I didn't try to trigger it)

[r-cyr]

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
  }
}

[ljharb]

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?

[rotu]

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?

[rotu]

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

[r-cyr]

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)

[rotu]

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

[rotu]

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). 😞

[RedYetiDev]

May be fixed by #53800

[RedYetiDev]

May be fixed by #53800

Closing, as that PR has landed. If you disagree, feel free to reopen.

[rotu]

I still see "uncaught" rejections from writableStreamDefaultWriterEnsureReadyPromiseRejected when closing a TCP socket with wrapped in stream.Duplex.toWeb

node/lib/internal/webstreams/writablestream.js

Line 1044 in 7168295

promise: PromiseReject(error),

Any reason this wasn't included in #53800?