Abort followed by write on WritableStream may invoke a null strategy algorithm

[shannonbooth]

What is the issue with the Streams Standard?

Consider the following test (simplification of https://github.com/web-platform-tests/wpt/blob/8686b7a6d288d3b2c22b5ddb5a21773619b22b85/streams/writable-streams/aborting.any.js#L57 to remove any infrastructure)

<script>

function test() {
  const ws = new WritableStream();

  setTimeout(() => {
      const writer = ws.getWriter();
      const abortPromise = writer.abort("some error");
      writer.write(1);
  }, 0)
}

test();
</script>

The stack of calls from abort(reason) are:

1. WritableStreamDefaultWriterAbort
2. WritableStreamAbort
3. WritableStreamStartErroring
4. WritableStreamFinishErroring

Step 12 of WritableStreamFinishErroring will then perform:

12. Let promise be ! stream.[[controller]].[[AbortSteps]](abortRequest’s reason).

Where [AbortSteps](reason) will clear the write algorithms:

2. Perform ! WritableStreamDefaultControllerClearAlgorithms(this).

Which includes the strategySizeAlgorithm:

4. Set controller.[[strategySizeAlgorithm]] to undefined.

Once abort has finished, writer.write(chunk) is invoked.

In WritableStreamDefaultWriterWrite step 4 is to:

4. Let chunkSize be ! WritableStreamDefaultControllerGetChunkSize(controller, chunk).

Which will invoke the strategySizeAlgorithm:

1. Let returnValue be the result of performing controller.[[strategySizeAlgorithm]], passing in chunk, and interpreting the result as a completion record.

However, this has been nulled out by the call to WritableStreamDefaultControllerClearAlgorithms earlier, as part of the abort.

The stream is in an errored state, and should throw an exception, but this is only performed on step 9 of WritableStreamDefaultWriterWrite.

Speculatively, I think step 4. of WritableStreamDefaultWriterWrite could be moved to just after step 9 where the erroring status is checked, but I am not sure if that has unintended side effects / observable in some way we care about.

[shannonbooth]

Unfortunately reordering the step does cause unintended side effects - at least for my case it results in wpt.live/streams/writable-streams/bad-strategies.any.html assertion failure on WritableStreamAddWriteRequest for the stream state. https://streams.spec.whatwg.org/#writable-stream-add-write-request

Falling back to a size of 1 in the getter works for both cases, but doesn't seem ideal.

[domenic]

I'll try to look into this in more detail later, but have you looked at what the reference implementation does in this case? It passes all the tests, including the one you're simplifying, so either the spec is correct already in some way, or there's a bug in the reference implementation which maybe could inspire us on how to fix the spec.

[shannonbooth]

It looks like it will catch the null strategy size algorithm exception:

streams/reference-implementation/lib/abstract-ops/writable-streams.js

Line 667 in 8ebee1f

} catch (chunkSizeE) {

Which seems a bit, off. At least to me doesn't read like editorially that it could be null and it feels like there should be a cleaner way to order the steps, e.g by maybe clearing the algorithms at a different time.

[domenic]

Ah, yep, overzealous catching of exceptions is precisely one of the ways the reference implementation can be buggy compared to the spec. (In the spec, "performing" a null algorithm is undefined behavior, whereas in JS it's just a catchable exception.)

Thanks for investigating; one of the editors (possibly myself) will work on trying to find a good solution here soon.

[domenic]

My plan is to solve this the same way that Firefox and Chrome seem to: by falling back to 1.

I'll add an assert that we're in the erroring or errored state, since that should be the only case we end up here.