Calling sink.abort() after a sink method rejection could lead to double cleanup

[ricea]

trafficstars

This was raised by @tyoshino in https://github.com/w3c/web-platform-tests/pull/5421#discussion_r111348353

Consider the following sink:

new WritableStream({
  write(chunk) {
    return handle.write(chunk)
        .catch(reason => {
          cleanup();
          return Promise.reject(reason);
        });
  }

  close() {
    handle.flush();
    cleanup();
  }

  abort() {
    cleanup();
  }

  cleanup() {
    handle.close();
  }
});

The author is trying to be diligent in closing the handle whatever happens. But since #721 if writer.abort() is called and then sink.write() fails, cleanup() will be called twice.

My feeling is that while this is unfortunate, the principle that sink.abort() is always called if writer.abort() is called before any other errors on the stream is a good one. I think adding finally() (#636) in V2 will provide a better way to implement the above sink, and until then we don't need to take any action.

I opened this issue to gather other opinions and to track this problem.

[domenic]

I agree with your feeling. I think in general without finally(), double-cleanup or not-enough-cleanup is a hazard with the current API, and you need to write code resilient to that. I guess we should prioritize finally() a decent bit.

[ricea]

I've been having second thoughts about this. But I really want to provide a stable target in the medium-term for browsers to implement. We could probably keep tweaking things forever if we let ourselves.

If/when browser vendors start complaining about this behaviour, that would be a good time to change it.

[tyoshino]

Thanks for the feedback, domenic, ricea. I'm fine with the plan.