search

whatwg / streams

Streams Standard
https://streams.spec.whatwg.org/
Other
1.35k stars 160 forks source link

Release reader immediately when shutting down a pipe #1208

Open MattiasBuelens opened 2 years ago

MattiasBuelens commented 2 years ago

In #1207, we raised concern about whether it's currently possible for pipeTo() to drop chunks when the pipe shuts down while it still has a pending read request.

It turns out that this is indeed possible. The new WPT tests demonstrate at least one way this can happen: by aborting a pipe with an AbortSignal, and then enqueuing a chunk immediately after aborting the signal.

This PR fixes this by releasing the pipe's reader immediately when starting the shutdown process, rather than when it finalizes the shutdown. This ensures that any pending reads that were started are immediately rejected, and the chunks stay in the source's queue.

~This does have the side effect of releasing the reader before the pipe promise resolves. I don't know if this is acceptable.~ Fixed in https://github.com/whatwg/streams/pull/1208#issuecomment-1015911238.

(See WHATWG Working Mode: Changes for more details.)

Preview | Diff

MattiasBuelens commented 2 years ago

This does have the side effect of releasing the reader before the pipe promise resolves. I don't know if this is acceptable.

Hmm, this means that the pipe could still cancel the readable after it has released its reader lock but before it settles the pipe promise. That might be quite unexpected: the user might have already acquired a new reader, and only that new reader should have the ability to cancel the stream.

Possible solutions:

  1. We could try to split up ReadableStream(Default|BYOB)Release, such that the read requests are already rejected but the reader is still locked to the stream. We would then actually release the reader as part of the "finalize" steps (as in the current spec).
  2. We could make the pipe releases its reader, and immediately acquire a new reader. It would no longer use that reader to request new chunks (since shuttingDown is already true), it would only be used to keep the stream locked (in case we still want to cancel it).

Option 2 is probably the easiest to implement, but it may require a note to explain why this weird "release + acquire" dance is necessary. Then again, we already mention that:

the exact manner in which this happens is not observable to author code, and so there is flexibility in how this is done

so user agents are free to implement these steps differently if they want. 🤷‍♂️

MattiasBuelens commented 2 years ago

Yes, they're equivalent. I'll go with option 2. 👍

MattiasBuelens commented 2 years ago

I've generalized the implementation a bit. I added two new helpers:

The pipeTo() reference implementation now uses these helpers instead of reader._closedPromise and writer._closedPromise to detect state changes. A checkState() helper checks the shutdown conditions when the pipe starts, and when the state of one of the streams changes.

This should handle all possible synchronous state transitions, and check the shutdown conditions in the correct order as specified by the standard's text.

(Perhaps we want to also use these helpers for tee(), and possibly eliminate the extra microtask from readRequest's chunk steps there? I'll leave that for another time though. 😛)