streams icon indicating copy to clipboard operation
streams copied to clipboard

Should the abort result reflect the close result?

Open yutakahirano opened this issue 3 years ago • 3 comments
trafficstars

Let's think about the following code.

const e = Error();
const ws = new WritableStream({close() { return Promise.reject(e); }});
const writer = ws.getWriter();
writer.close().catch(() => {});
await writer.abort();

This throws an exception because https://streams.spec.whatwg.org/#writable-stream-finish-in-flight-close-with-error rejects the promise. I want to understand the reason behind this - why is it bad to just fulfill the promise returned by abort(), given close() returns its own promise?

Note that the following code doesn't throw.

const e = Error();
const ws = new WritableStream({close() { return Promise.reject(e); }});
const writer = ws.getWriter();
await writer.close().catch(() => {});
await writer.abort();

cc: @nidhijaju

yutakahirano avatar Jan 06 '22 07:01 yutakahirano

Some of the discussions in https://github.com/whatwg/streams/pulls?q=is%3Apr+pendingAbortRequest+is%3Aclosed , in particular https://github.com/whatwg/streams/pull/655 and https://github.com/whatwg/streams/pull/634, seem related.

Note that the following code doesn't throw.

This is a bit surprising and indeed seems inconsistent.

In general it seems like this is an area we've spent a lot of time on involving some complicated structures like [[pendingAbortRequest]]. So that kind of inconsistency is surprising. What were we trying to accomplish? 😕

The for-web-developers documentation for abort() doesn't explain what happens in such scenarios, and we don't seem to have documented the intent anywhere else.

Some relevant tests starting here:

  • https://github.com/web-platform-tests/wpt/blob/305cf97e20c9b5ab08fcf28a62c153f54536fbbf/streams/writable-streams/aborting.any.js#L231
  • https://github.com/web-platform-tests/wpt/blob/305cf97e20c9b5ab08fcf28a62c153f54536fbbf/streams/writable-streams/aborting.any.js#L442 : this block includes the scenario here
  • https://github.com/web-platform-tests/wpt/blob/305cf97e20c9b5ab08fcf28a62c153f54536fbbf/streams/writable-streams/aborting.any.js#L649 : these indicate that if you controller.error() during a close, that will cause abortPromise to fulfill
  • https://github.com/web-platform-tests/wpt/blob/305cf97e20c9b5ab08fcf28a62c153f54536fbbf/streams/writable-streams/aborting.any.js#L1182 the scenario here again, maybe redundantly

Blame on the test file indicates maybe https://github.com/whatwg/streams/pull/672 is the cause.

So my best guess is that this is a historical accident and we should fix it to fulfill with undefined. I wonder if that would allow simplifications (e.g. if some of the [[pendingAbortRequest]] machinery becomes less necessary?) I'd love more perspectives though.

domenic avatar Jan 06 '22 22:01 domenic

One of the design principles we tried to follow was "users shouldn't be punished for race conditions outside of their control". We appear to have failed in this case.

I don't have a good sense for whether it would be safe to change this behaviour. Could there be a non-trivial number of scripts depending on it?

ricea avatar Mar 29 '22 08:03 ricea

In general changing things from not-throwing to throwing is seen as likely to be safe. From rejecting to fulfilling should be symmetric in theory, but in practice I think people have found it a bit more dangerous. Still, this is specifically about abort() in an edge case, so I suspect it would be safe.

domenic avatar Mar 29 '22 14:03 domenic