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

BYOB with "fill at least" semantics #1175

Closed
jasnell opened this issue Oct 13, 2021 · 2 comments
Closed

BYOB with "fill at least" semantics #1175

jasnell opened this issue Oct 13, 2021 · 2 comments

Comments

@jasnell
Copy link

jasnell commented Oct 13, 2021

The BYOB reader can be a bit inefficient if the controller does not make an effort to fill the given view as much as possible. For the Cloudflare implementation, we're looking at options for a stream consumer to instruct the stream to fill a view with a specified minimum number of bytes before returning. For instance, "here's a 100 byte view, don't resolve the read until you've got at least 50 bytes in it".

In the current API, there are several ways we could achieve this:

  1. We could add a new non-standard readAtLeast() method to the existing ReadableStreamBYOBReader interface, e.g. const reader = stream.getReader({ mode: 'byob' }); await reader.readAtLeast(50, new Uint8Array(100));. This is obviously not ideal unless the method became part of the standard interface.
  2. We could create a new kind of reader such that, const reader = new ReadableStreamAtLeastReader(stream); await reader.read(50, new Uint8Array(100));. This avoids stepping on the standard but creates additional complexities on the implementation side.
  3. Coupled with 2, we could introduce a new mode, const reader = stream.getReader({ mode: 'at-least' }) that would return the new reader type from 2.
  4. We could add an optional options to the existing ReadableStreamBYOBReader's read method, e.g. const reader = stream.getReader({ mode: 'byob'}); await reader.read(new Uint8Array(100), { atLeast: 50 });, but again, this steps on the standard interface and isn't portable.

A key part of this is that we want the stream consumer to be able to indicate the minimum number of bytes they wish filled, and most of the time those consumers will not have any visibility to the underlying source that is filling the view (that is, there won't be any other way to independently tell the underlying source to fill the minimal number of bytes).

Before we choose a direction, I'd like to get a sense from the spec authors which direction they feel is most appropriate.

@jasnell
Copy link
Author

jasnell commented Oct 13, 2021

This is closely similar to #1143 ...

Given the choice between readFully() and readAtLeast() ... we definitely have a preference for readAtLeast() ...

@vlovich
Copy link

vlovich commented Oct 13, 2021

Technically readAtLeast could be more cleanly added as an optional dictionary to ReadableStreamBYOBReader's read rather than as a standalone function. I.e.

({done, value} = await read(buffer, {minBytes: 100});

One could easily imagine extending this with additional options like a timeout that ensures that we read minBytes bytes first OR the timeout hits returning whatever has been buffered so far. Without minBytes, then we read however much we can within that time interval. From a practical implementation standpoint that might be difficult to plumb through & there may not be a motivating use-case yet, but allowing an optional dictionary for customizing minBytes doesn't sound like a terrible idea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants