Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resolve BYOB reads immediately on cancel #1129

Merged
merged 4 commits into from
Jun 2, 2021

Conversation

MattiasBuelens
Copy link
Collaborator

@MattiasBuelens MattiasBuelens commented May 26, 2021

Right now, if you cancel the stream while a BYOB read is pending, the stream has to wait for the underlying byte source to call respond(0) before it can return the BYOB request's buffer to the caller. Effectively, every single underlying byte source would need to look like this:

const readable = new ReadableStream({
  type: 'bytes',
  autoAllocateChunkSize: 1024,
  start(controller) {
    this._controller = controller;
  },
  async pull(controller) {
    const bytesWritten = await readBytesIntoViewSomehow(controller.byobRequest.view);
    controller.byobRequest.respond(bytesWritten);
  },
  cancel(reason) {
    this._controller.byobRequest?.respond(0); // must not forget this!
  }
});

This is a footgun: we have to rely on the underlying byte source to do this correctly. We are unable to do this automatically, since we don't know if or how the source might be using the BYOB request (for example, it might have transferred the buffer). If the source fails to call respond(0), then the BYOB read never resolves and the reader gets stuck. If you forget to implement cancel() altogether, then the default implementation will not help either, since it is specified to do nothing.

In #1114 (comment) and #1123 (comment), @domenic suggested that if you cancel the stream while a BYOB read is pending, the stream should simply not give you back your buffer. This means that cancel() can immediately resolve all pending BYOB reads with the classic { done: true, value: undefined }, without having to wait for the underlying byte source. This solves the problem, and would make it easier to implement an underlying byte source.

This PR implements this suggestion. To make it work, I also had to change one other thing: when the stream is cancelled, any pending BYOB request is now immediately invalidated, so the underlying byte source doesn't erroneously think that it still needs to provide a response. (This aligns with the behavior of controller.enqueue(), which throws if the stream is already closed.)

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


Preview | Diff

@MattiasBuelens
Copy link
Collaborator Author

I initially thought that we'd need to refactor large parts of the specification to make this work, but surprisingly there aren't that many changes needed. So that's good... assuming I didn't miss anything important. 😅

Copy link
Collaborator

@ricea ricea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@MattiasBuelens MattiasBuelens marked this pull request as ready for review May 27, 2021 21:41
domenic pushed a commit to web-platform-tests/wpt that referenced this pull request Jun 1, 2021
@domenic domenic merged commit 8a7d92b into whatwg:main Jun 2, 2021
@MattiasBuelens MattiasBuelens deleted the resolve-byob-reads-on-cancel branch June 2, 2021 20:35
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Jun 8, 2021
…on cancel, a=testonly

Automatic update from web-platform-tests
Streams: resolve BYOB reads immediately on cancel

See whatwg/streams#1129 for the accompanying spec change.
--

wpt-commits: b869e60df1b8d3840e09b41c5e987c7e23f6856c
wpt-pr: 29128
jamienicol pushed a commit to jamienicol/gecko that referenced this pull request Jun 9, 2021
…on cancel, a=testonly

Automatic update from web-platform-tests
Streams: resolve BYOB reads immediately on cancel

See whatwg/streams#1129 for the accompanying spec change.
--

wpt-commits: b869e60df1b8d3840e09b41c5e987c7e23f6856c
wpt-pr: 29128
blueboxd pushed a commit to blueboxd/chromium-legacy that referenced this pull request Oct 13, 2021
This CL is based on spec changes made in whatwg/streams#1129.

Summary/context of changes:
If you cancel a stream while a BYOB read is pending, the stream
should simply not give back the buffer. This means that cancel()
can immediately resolve all pending BYOB reads with the classic
{ done: true, value: undefined }, without having to wait for the
underlying byte source. Also, when the stream is cancelled, any
pending BYOB request is now immediately invalidated, so the
underlying byte source doesn't erroneously think that it still
needs to provide a response.

Bug: 1223565
Change-Id: I17d1d5cfaee80c02c51bc682447b381ade4363bf
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3216778
Commit-Queue: Nidhi Jaju <nidhijaju@chromium.org>
Reviewed-by: Adam Rice <ricea@chromium.org>
Cr-Commit-Position: refs/heads/main@{#930987}
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this pull request Oct 14, 2022
This CL is based on spec changes made in whatwg/streams#1129.

Summary/context of changes:
If you cancel a stream while a BYOB read is pending, the stream
should simply not give back the buffer. This means that cancel()
can immediately resolve all pending BYOB reads with the classic
{ done: true, value: undefined }, without having to wait for the
underlying byte source. Also, when the stream is cancelled, any
pending BYOB request is now immediately invalidated, so the
underlying byte source doesn't erroneously think that it still
needs to provide a response.

Bug: 1223565
Change-Id: I17d1d5cfaee80c02c51bc682447b381ade4363bf
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3216778
Commit-Queue: Nidhi Jaju <nidhijaju@chromium.org>
Reviewed-by: Adam Rice <ricea@chromium.org>
Cr-Commit-Position: refs/heads/main@{#930987}
NOKEYCHECK=True
GitOrigin-RevId: e8c4592de3851305dd1f3a021d528c84f8d98658
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants