Skip to content

Add an abstraction to pull from buffer into ReadableStream #1263

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

Merged
merged 7 commits into from
Apr 5, 2023

Conversation

saschanaz
Copy link
Member

@saschanaz saschanaz commented Mar 30, 2023

Intended to fix the Fetch spec issue whatwg/fetch#1610.

  • At least two implementers are interested (and none opposed): N/A, just moving existing steps from Fetch to Streams
  • Tests are written and can be reviewed and commented upon at: Same
  • Implementation bugs are filed: N/A
  • MDN issue is filed: N/A

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


Preview | Diff

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

So one issue here is that this "buffer" type, and the operations on it, are undefined. The operations seem to be:

  • "extracting from" which returns a byte sequence while presumably modifying the buffer;
  • "size" which returns an integer; and
  • "append" in fetch

I was OK with this as a one-off over in fetch, but since we're encountering this a couple times, we should think a bit harder about whether it's the route we want to go down. I.e., how should we represent underlying platform byte buffers in specs.

Options I can see:

  • Leave it as-is in this PR. It's fine, probably people know what it means.
  • Define a "platform byte buffer" type, probably in Streams, with a couple sentences describing what it generally corresponds to and explaining the two operations you can apply to one of these platform byte buffers.
  • Use byte sequences instead. Then specs like Fetch would say something like "Let |buffer| be an empty [=byte sequence=]. NOTE: this represents the internal buffer inside the network layer of the user agent".

I think using byte sequences would be most reasonable. WDYT?

Co-authored-by: Domenic Denicola <[email protected]>
@saschanaz
Copy link
Member Author

Good point! Using byte sequence sounds good to me. But we still have no definition for the operations, right?

@domenic
Copy link
Member

domenic commented Mar 31, 2023

Well, yes, but we have length. And I feel less bad about using common-sense definitions if the base type is well-defined. So:

  • We can use length instead of size
  • We can use "Append" because it's straightforwardly clear
  • For "Extract", I'd feel better if we broke it down into "Let x be the first Y bytes of Z", "Remove the first Y bytes from Z". Then the common-sense definitions are very clear.

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

Thanks, this algorithm looks great now! I pushed a commit to move it around and integrate it with the rest of that section; if those changes look good to you, then I think we can merge.

Copy link
Member Author

@saschanaz saschanaz left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the cleanup!

index.bs Outdated
@@ -6808,10 +6784,38 @@ They should only [=ArrayBufferView/create=] a new {{ArrayBufferView}} to pass to
[=ReadableStream/enqueue=] when the [=ReadableStream/current BYOB request view=] is null, or when
they have more bytes on hand than the [=ReadableStream/current BYOB request view=]'s
Copy link
Member Author

@saschanaz saschanaz Apr 3, 2023

Choose a reason for hiding this comment

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

Should we explicitly say the below algorithm doesn't enqueue "when they have more bytes on hand than ..."?

Also, w3c/webtransport#487 throws in that case, should it enqueue instead? Not sure it's fine to implicitly ignore the byob request view. Oh okay, it doesn't match what it wants in that case.

Copy link
Member

Choose a reason for hiding this comment

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

Added a note, thanks!

@domenic domenic merged commit 2942e89 into whatwg:main Apr 5, 2023
@saschanaz saschanaz deleted the byob-read-abstraction branch April 5, 2023 06:10
annevk pushed a commit to whatwg/fetch that referenced this pull request Apr 28, 2023
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.

2 participants