[whatwg/streams] pipeTo(): Rationale for not making shutdown() no-op when shuttingDown is set (#557)

Forked from https://github.com/whatwg/streams/pull/512#issuecomment-255938194

This https://github.com/whatwg/streams/pull/512#issuecomment-255754552 approach sounds not so good to me.

I understand that it sometimes works well for simplifying code to omit logic to suppress harmless processing and just let it happen as no-op. But I think such approaches should be taken only when:
- it's very clear that the operations would result in no-op
- or, there's a clear win of performance compared to introduced complexity of the carefully implemented optimization

To ensure that the redundant finalize() is harmless, we need to ensure that it happens only after everything it affects is settled down. There are, a promise (waitForCurrentWrite()) involved, three possible actions happen in action(), 3 abstract operation (promise resolution) invocation in finalize(). It's too much.

Actually, I just found a real bug in the current code. An error of the WritableStream caused by `.abort()` invocation by the `pipeTo()` logic (during `shutdownWithAction()` assuming the `preventAbort` was set to falsy) in response to the ReadableStream's error would also invoke `shutdown()` (assuming `preventCancel` was set to truthy) for the abortion of the WritableStream. When the underlying sink's `abort()` takes time e.g. please try changing the underlying sink's `abort()` to be `delay(10).then(() => { throw error2; })`, `finalize()` call for the `shutdown()` would win. In this example, the `pipePromise` gets rejected with `new TypeError('Aborted')` while it's expected that `pipePromise` gets rejected by `error2`.

Even after fixing this bug, despite of the cost of reasoning, there should not be much performance/readability gain with this omission, I guess. We already have the `shuttingDown` defined for the async case. Additional if-clause wouldn't be a big pain for us and readers in terms of either readability or performance. With the if-clause, we can omit a microtask containing invocation of 3 meaningless function calls and make the solidness of the logic easier to reason about.

Additionally, the name `shuttingDown` sounds like a book keeping variable for controlling duplicated invocation of shutdown.*(). It strikes at least to me that it's confusing that only `shutdownWithAction()` has an if-clause to be no-op when `shuttingDown` is set.


-- 
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/issues/557