Re: [whatwg/streams] ReadableStream reference implementation: remove unnecessary code (#992)

MattiasBuelens commented on this pull request.



> @@ -460,10 +460,7 @@ function ReadableStreamPipeTo(source, dest, preventClose, preventAbort, preventC
       }
     }
 
-    pipeLoop().catch(err => {
-      currentWrite = Promise.resolve();

This change looks very similar to #884, where a pipe could end too soon without waiting for pending writes. Except this time, I can't come up with a failing test case to show that keeping this line would cause a bug. 😛 

In order to cause a bug, we would need to hit this line *before* we call `waitForWritesToFinish()`. We can't error the writable end, because then we don't need to wait for any pending writes anyway. For the same reason, we can't have `preventAbort = false`. We would need to error the readable end *in some way* that would lead to the following chain of events:
1. `ReadableStreamDefaultReaderRead` rejects, causing `pipeStep` and subsequently `pipeLoop` to reject.
1. `pipeLoop().catch` handler resets `currentWrite`, forgetting about the pending write.
1. `reader._closedPromise` rejects, causing `isOrBecomesErrored` to call `shutdown` and subsequently `waitForWritesToFinish`.
1. `waitForWritesToFinish` waits for `currentWrite`, but it has already been replaced by `pipeLoop().catch` so it resolves immediately.
1. `shutdown` calls `finalize`, causing the pipe to incorrectly complete without waiting for the pending write.

Whatever I tried, I couldn't get `pipeLoop().catch` to run before `shutdown`. The closed promise always seems to reject before the read promise reject, so we never end up in the "bad" scenario where we clear `currentWrite` too early. Therefore, no bug... 😕 

TL;DR: Although I can't find a test case to show that removing this line is **necessary** for correctness, I don't think the line served any purpose and I'm happy to see it removed. 🙂

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/whatwg/streams/pull/992#pullrequestreview-205482631