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) {

Oh boy, this is a can of worms... 😅

Making `waitForWritesToFinish()` possibly synchronous (https://github.com/MattiasBuelens/streams/commit/7eef3d2ccd447f674b2b2f82b4651201a91cd6d0) allows weird things to happen:
1. `pipeTo()` calls `ReadableStreamDefaultReaderRead()`, reading the *last* chunk of the stream.
2. `ReadableStreamDefaultController[[PullSteps]]` dequeues the chunk, and notices that the queue is now empty *and* close was requested. So it calls `ReadableStreamClose()`.
3. `ReadableStreamClose` changes `[[state]]` to `"closed"`, which fires `pipeTo()`'s state change listener.
4. `pipeTo()` starts the shutdown process. There's no pending write, so it immediately finalizes the pipe and releases both the reader and the writer.
5. The call stack unwinds up to `ReadableStreamDefaultController[[PullSteps]]`. We continue with `readRequest.chunkSteps()`.
6. `chunkSteps()` brings us back inside `pipeTo()`. It now wants to start a new write for this chunk, *but we've already released the writer.* 💥

This wasn't a problem before, since even if we never actually started a write, `currentWrite` would still be initialized to `Promise.resolve(undefined)`. `waitForWritesToFinish()` would always have to wait at least one microtask to detect that this is indeed the last write, which is enough time for `chunkSteps()` to *actually* start a new write on time.

The root of the problem is that the state change listener is called *in the middle* of `ReadableStreamDefaultReaderRead()`, before we call `chunkSteps()`.
* We could try to only call the state change listeners *at the end* of all "top-level" abstract ops. That is, instead of calling them within the internal `ReadableStreamClose()`, we call them at the end of `ReadableStreamDefaultReaderRead()` etc. However, this could quickly become unmaintainable. 😕 
* We could have `pipeTo()` "remember" that it still has a pending read, and *wait* until either its `chunkSteps`, `closeSteps` or `errorSteps` have been called before finalizing the pipe. But then I'm not sure if `waitForWritesToFinish()` would still be synchronous in the cases where it matters... 🤔

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

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

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