Re: [whatwg/streams] Note about underlyingSink close() method is misleading (#617)

I just realized something else related... I'll keep it in this thread because it reminds me of @ricea's suggestion.

Right now, if `us.write()` fails, the stream will go in to the errored state---but no cleanup code will be called. That is, underlying sink writers need to take care to catch any errors writing to their underlying sink and then perform cleanup if so. This is not obvious: for example, https://streams.spec.whatwg.org/#example-ws-backpressure seems to miss that. It should instead be e.g.

```js
    async write(chunk) {
      try {
        await fs.write(fd, chunk, 0, chunk.length);
      } finally {
        await this.close();
      }
    }
```

(with the code even more complex than this if you don't use async functions and finally.)

This seems pretty bad and I'm sad we hadn't realized it before.

The guiding goal behind the current setup is roughly that you should only have to implement cleanup logic in one place. That's what guides the fallback in abort. I've never been super-happy with the implementation of that goal as-is, since it leads to strange situations like allowing close + abort, or apparently a bunch of interesting edge cases we're just now discovering.

I think @ricea's destroy + shutdown might be too much, because it seems geared toward allowing the interruption of ongoing writes, which as in my previous comment I think we should not really support. But I think it's indeed worth rethinking how to accomplish this goal.

One idea would be just `shutdown(why, reason) { }` which is always called any time the stream is transitioning from "writable" to "closed" or "errored". So, for: failed writes; writer.abort() (after the ongoing write finishes); writer.close() (after all queued writes finish).

-- 
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/617#issuecomment-262397827