quic: fix readable stream truncation on stop-sending, abort & timeout

[pimterry]

Currently in ~all cases where a QUIC stream is closed abruptly, we still exposed the data as a successfully completed stream on the iterable - in effect truncating it without any notice. This includes:

• Idle timeouts. When you hit the timeout, the readable stream just ends abruptly.
• Local destroy() or stopSending(). This is a no-error abort, but that doesn't mean we should expose the received data as a successfully received & complete stream.
• Remote reset (clean or error). Ditto.

This PR changes the iterable to throw in any scenario where you don't receive the full stream. If the remote peer doesn't send a happy FIN successfully for some reason, you haven't received the full data, and you should know that.

This matches the behaviour for Node streams in all our other APIs, including TCP sockets, which expose various PREMATURE_CLOSE and similar errors when you try to read a stream that is closed but hasn't actually received a clean end signal. All data up to the stream abort is delivered successfully, it's just the final read at the end that exposes the error.

This modifies blob.js generally, but only createBlobReaderIterable which is exclusively used by QUIC, and I think the behaviour is correct for an iterable reader in any scenarios.

This change doesn't touch closed, which I did consider for a while. The docs say:

A promise that is fulfilled when the stream is fully closed. It resolves
when the stream closes cleanly (including idle timeout). It rejects with
an ERR_QUIC_APPLICATION_ERROR or ERR_QUIC_TRANSPORT_ERROR when the
stream is closed due to a QUIC error (e.g., stream reset by the peer,
CONNECTION_CLOSE with a non-zero error code).

I'm treating that as "is there an actual definite error" as opposed to "the readable or the writable actually completed successfully". This diverges from our other stream semantics, but I think it's reasonable. Just might be worth considering if we want a separate way to wait for "readable ended OK" later on, currently those aren't part of the stream API so only the consumer itself knows this and there's no way to check the stream result externally. I'd rather leave that till later and just fix the clear bugs for now.

[nodejs-github-bot]

Review requested:

• @nodejs/quic

[jasnell]

hmm... "This PR changes the iterable to throw in any scenario where you don't receive the full stream"

I'm not sure this should be a blanket default behavior in every scenario. A closed stream in not really no-notice as the done: true is giving notice. In many cases with idle close, it's perfectly fine to just... stop providing data and exit clean.

I wonder if it is reasonable to make this a configuration option.

[pimterry]

A closed stream in not really no-notice as the done: true is giving notice.

You get notice that the stream is closed, but you're just not directly told that it's been truncated & failed to reach the end of the data, as opposed to cleanly completing. Both return done: true. The protocol has an explicit signal for "that's the end of my data".

I'm not sure this should be a blanket default behavior in every scenario.

Do you have any examples where the stream ends abruptly without the remote peer sending FIN, but a read should resolve successfully? I'd be interested in protocol flows where unclear shutdown like that is common.

For actual errors, I think this change is clear enough and hopefully unambiguous. I can see how the cleaner shutdown cases (idle timeout/remote & local cancel) are more debatable, I did think about the same thing for a good while. I've end up convincing myself that the iterator should always treat not reaching FIN as an error though. Some more detailed reasons:

• It matches our other APIs. For TCP, when you read a net.Socket, if it ends with anything except a clean FIN, it rejects as a failed read. The exact equivalent code reading a socket as an asynciterable rejects (and the equivalent happens if reading via for read(), or pipe(), or data/end events - none end cleanly). Mismatching these is going to create pain for us & users, and those semantics have existed for a long time and seem to work well for everybody (AFAICT).
• For idle timeout, it's actually often a serious error: if a peer crashes mid-stream, I think in a UDP world this is often how we find out. We shouldn't treat a stream of data as cleanly finished if the peer suddenly disconnects half way through writing a message.
• This means that other APIs like await text(stream) will error as well. Without this, they just return the truncated data as the complete body which seems even more clearly wrong than the iterator case.
• It affects HTTP/3 on top identically: right now, if an HTTP/3 server is reading a file upload request, and the client cancels it half way or disconnects, the server will see the body as completed (assuming no content-length header). Calls like await bytes(requestBody) will resolve successfully with half the body. Even if it's detectable elsewhere, it seems like that would catch a lot of people out (I found this because it caught me out!)

I'm not saying this should be an error emit or something similar that everybody needs to always carefully look for & handle. Just that if you're actively reading, and the stream ends without the remote peer sending a FIN, the read should fail (reject with an error). If you never read the stream, or if you've started but stopped now (not awaiting next()), or if you've already read to the end of the remote peer's FIN, then there's no error to handle.

As above, I haven't changed closed to error, which would indeed cause more issues. That only rejects for explicit transport/app errors (ignores idle timeout and local/remote cancellation).