stream: fix addAbortSignal() for web streams

[ikeyan]

This fixes stream.addAbortSignal() for web streams.

addAbortSignal() used kControllerErrorFunction, which mixed stream erroring
with abort handling.

For readable byte streams, ReadableByteStreamController left the default no-op
hook in place, so addAbortSignal() could not abort the stream.

For writable streams, WritableStreamDefaultController wired the hook to
controller.error(), so the stream became errored but
controller.signal.aborted stayed false.

This replaces kControllerErrorFunction with kControllerAbortFunction and
wires each controller to the internal operation that matches its abort path:

• ReadableStreamDefaultController uses
  readableStreamDefaultControllerError()
• ReadableByteStreamController uses
  readableByteStreamControllerError()
• WritableStreamDefaultController uses writableStreamAbort()

The updated test covers addAbortSignal() on readable byte streams, aborts
that race with pending pulls, writable abort signal state, and readable and
writable controllers with overridden public methods.

[nodejs-github-bot]

Review requested:

• @nodejs/streams

[MattiasBuelens]

Slightly off-topic: should we make this a class method instead? For WritableStream, we could do:

class WritableStream {
  [kControllerAbortFunction](reason) {
    setPromiseHandled(writableStreamAbort(this, reason));
  }
}

For ReadableStream, the method could check the type of this[kState].controller and then call either readableStreamDefaultControllerError or readableByteStreamControllerError.

[MattiasBuelens]

Out of curiosity: why are we erroring the stream instead of canceling it?

• A consumer can cancel the stream with readableStreamCancel, and the stream will call the source.cancel callback so it can clean up any resources.
• With controller.error, there is no so such callback, since the Streams standard assumes that the error was signaled by the source. If we allow a consumer to inject an error into the stream, this may leak resources in the source.

[ikeyan]

@MattiasBuelens
Should I include that fix here as well?

[MattiasBuelens]

Hmm, that's probably not a good idea. The addAbortSignal documentation currently states that it calls controller.error, so changing that will be a breaking change... 🤔

Let's stick with the current behavior for now. I'll put up a separate draft PR to discuss if we want to change it or not.

Actually, this PR is already introducing a breaking change for WritableStream... so maybe we should just fix both. 😅

[MattiasBuelens]

I believe this is a breaking change for WritableStreams.

I think this PR is a good change: using controller.error from a consumer API is fundamentally wrong, and we got this wrong from the start in #46273. But that does mean this PR is a semver-major change.

While we're at it, I think we should fix the behavior for ReadableStreams too to call readableStreamCancel instead.

@nodejs/streams Thoughts?

[MattiasBuelens]

This is a breaking change. From doc/api/stream.md:

Calling abort on the AbortController corresponding to the passed
AbortSignal will behave the same way as calling .destroy(new AbortError())
on the stream, and controller.error(new AbortError()) for webstreams.

We'll need to at least update the docs to document the new behavior.