Re: [whatwg/streams] Release reader immediately when shutting down a pipe (PR #1208)

@MattiasBuelens commented on this pull request.



> @@ -475,6 +483,20 @@ function WritableStreamDefaultWriterGetDesiredSize(writer) {
   return WritableStreamDefaultControllerGetDesiredSize(stream._controller);
 }
 
+function WritableStreamDefaultWriterIsOrBecomesErrored(writer, errorListener) {

> I suggest limiting the sync part to as small as possible to fix the issue.

The sync part is already minimal. We have to go from "if source becomes errored" all the way to "perform WritableStreamAbort" in order to avoid `ws.controller.error()` from affecting the result. Thus, the entirety of [step 3 in "shutdown with an action"](https://streams.spec.whatwg.org/commit-snapshots/5afb0e014c08be94e49db7c0305aa211545e3add/#rs-pipeTo-shutdown-with-action) must become synchronous (only if there are no pending writes that need to be awaited).

---

Anyway, I found another way to fix it. We keep track of how many `ReadableStreamDefaultReaderRead()` requests are still outstanding, and we *only* handle the `source.[[state]] == "closed"` transition after all those requests are settled. See https://github.com/MattiasBuelens/streams/commit/3c8b3c28f37c99bf49a58db89ff3baaff28aa503.


However, [this test](https://github.com/web-platform-tests/wpt/blob/99d74f9529e16ec0722ef11136ab29b9e80fff26/streams/piping/abort.any.js#L237) still fails. We do call `dest.abort()` and `source.cancel()` in the correct order, but it seems like `underlyingSink.abort()` and `underlyingSource.cancel()` are being called in the wrong order. When we call [WritableStreamStartErroring](https://github.com/whatwg/streams/blob/main/reference-implementation/lib/abstract-ops/writable-streams.js#L383), the writable controller is not yet started, so we postpone calling `sink.abort()` until after `sink.start()` resolves. Previously, the writable would already have been started while we were *asynchronously* [waiting for the writes to finish](https://github.com/whatwg/streams/blob/5afb0e014c08be94e49db7c0305aa211545e3add/reference-implementation/lib/abstract-ops/readable-streams.js#L294) (even if there were no pending writes).

Adding `await flushAsyncEvents()` before calling `pipeTo()` in that test restores the order and fixes the problem. Good enough? 🤷‍♂️

---

> We've tried to give latitude for implementations to optimise in their own way, but we're increasingly constraining their behaviour. Transparent thread offloading etc. may become impossible. I'm worried about it but I don't have an answer.

I agree, the reference implementation is becoming increasingly complicated in order to deal with these edge cases. 😞 

I'm wondering if it's even *worth* trying to spec these edge cases, or instead allow some wiggle room in how quickly `pipeTo()` must respond to these state transitions. But then it would become impossible to test the behavior, or we'd have to allow *multiple* valid outcomes... 😕

-- 
Reply to this email directly or view it on GitHub:
https://github.com/whatwg/streams/pull/1208#discussion_r793942484

You are receiving this because you are subscribed to this thread.

Message ID: <whatwg/streams/pull/1208/review/865349384@github.com>