Skip to content

Set body with byte reading support #1593

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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Merged
annevk merged 11 commits into from
Feb 20, 2023
Merged

Set body with byte reading support #1593

Changes from 2 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
ea9d056
Set body with byte reading support
saschanaz Jan 11, 2023
8f98591
Implement proper BYOB enqueue
saschanaz Jan 19, 2023
e07ef8d
Sleep on enough buffer and wake up on empty buffer
saschanaz Jan 23, 2023
c6130c0
Prevent reentrance
saschanaz Jan 26, 2023
8441245
Link "a new promise"
saschanaz Jan 26, 2023
09ce040
nested steps, a lower limit, and fetch task
saschanaz Jan 27, 2023
4f22afb
Merge branch 'main' into byte-stream
saschanaz Feb 6, 2023
3c85e1f
Run in parallel and then return promise
saschanaz Feb 6, 2023
23f9912
Update fetch.bs
saschanaz Feb 15, 2023
053d28b
Reformat
saschanaz Feb 15, 2023
5473365
Reformat 2
saschanaz Feb 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 45 additions & 22 deletions fetch.bs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5914,25 +5914,55 @@ optional boolean <var>forceNewConnection</var> (default false), run these steps:
<li><p>Return the <a for=/>appropriate network error</a> for <var>fetchParams</var>.
</ol>

<li><p>Let <var>pullAlgorithm</var> be an algorithm that <a lt=resumed for=fetch>resumes</a> the
ongoing fetch if it is <a lt=suspend for=fetch>suspended</a>.
<li>
<p>Let |buffer| be an empty buffer that can have bytes appended to it.

<p class="note">This represents an internal buffer inside the network layer of the user agent.

<li><p>Let |pullAlgorithm| be the followings steps:
Comment thread
annevk marked this conversation as resolved.
Outdated

<ol>
<li>[=fetch/resumed|Resume=] the ongoing fetch if it is [=fetch/suspend|suspended=].

<li>Wait until |buffer| is not empty.

<li>Let |available| be the size of |buffer|.

<li>Let |desiredSize| be |available|.

<li>If |stream|'s [=ReadableStream/current BYOB request view=] is non-null, then set

@domenic domenic Jan 27, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The current BYOB request view should not be accessed in parallel; nor should you create Uint8Arrays. You could probably fix this by posting a fetch task back after the waiting is done.

However, I'm not sure this is worth fixing, given the general problems with Streams being very JSey but being used even for no-JS-involved fetches (e.g. those who use a parallel queue). Thoughts from @annevk appreciated.

@saschanaz saschanaz Jan 27, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

For now I fixed it to queue a task as it was simple enough, I can revert it if Anne disagrees.

@annevk annevk Feb 6, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think it makes sense to do the right thing where we can, but I also don't mind if we take shortcuts when it cannot be observed. Although at some point we'll have to clean it all up.

|desiredSize| to |stream|'s [=ReadableStream/current BYOB request view=]'s [=BufferSource/byte
length=].

<li>Let |extractSize| be the smaller value of |available| and |desiredSize|.

<li>Let <var>bytes</var> be the result of extracting |extractSize| of bytes from
<var>buffer</var>.

<li>If |stream|s [=ReadableStream/current BYOB request view=] is non-null, then
[=ArrayBufferView/write=] |bytes| into |stream|'s [=ReadableStream/current BYOB request view=],
and set |view| to |stream|'s [=ReadableStream/current BYOB request view=]. <li>Otherwise, set
|view| to the result of [=ArrayBufferView/create|creating=] a {{Uint8Array}} from |bytes| in
|stream|'s [=relevant Realm=].

<li>[=ReadableStream/Enqueue=] |view| into |stream|.
Comment thread
domenic marked this conversation as resolved.
Outdated

<li><p>If |stream| is [=ReadableStream/errored=], then [=fetch controller/terminate=]
|fetchParams|'s [=fetch params/controller=].

@domenic domenic Jan 23, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think this can happen inside pull. I think it needs to be separate, something like, if the network blows up, terminate the controller and error the stream.

I guess below there is already a line that does that, but it doesn't error the stream? Is there some way in which terminating the controller errors the stream, which I haven't seen?

@saschanaz saschanaz Jan 23, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm not quite sure either and honestly just copied this from the previous steps which was also right after the enqueueing step.

Looking at the Gecko call diagram, it seems ReadableByteStreamControllerRespond can lead to ReadableByteStreamControllerEnqueueClonedChunkToQueue which can error the stream on a buffer clone failure. It's complex enough, I'd be happy if the spec can list the possible error reasons.

I think it needs to be separate, something like, if the network blows up, terminate the controller and error the stream.

Yes, but I think that's a lil bit out of scope here as I see no relevant existing step, right?

@domenic domenic Jan 27, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I guess if the lack of proper erroring step is a preexisting problem then we don't need to fix it here.

I do think this step makes very little sense, so I'd prefer to remove it.

@saschanaz saschanaz Jan 27, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Copypasting my question from Matrix: https://matrix.to/#/!AGetWbsMpFPdSgUrbs:matrix.org/$1y0cHr7B913RupwXNW93uLA06GT5Uls6nV8Ds8khB80?via=matrix.org&via=mozilla.org&via=igalia.com

r = new ReadableStream({
  async pull(c) {
    await new Promise(r => setTimeout(r, 100));
    c.byobRequest.respond(512);
  },
  type: "bytes"
});
reader = r.getReader({mode: "byob"});
reader.read(new Uint16Array(1024));
setTimeout(() => reader.releaseLock(), 5);

This eventually hits https://streams.spec.whatwg.org/#abstract-opdef-readablebytestreamcontrollerenqueueclonedchunktoqueue which theoretically can error the stream. Can Fetch really ignore this?


<li><p>If |stream| doesn't [=ReadableStream/need more data=], ask the user agent to
[=fetch/suspend=] the ongoing fetch.

@domenic domenic Jan 23, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In this model, we don't consult "need more data" at all. We should instead have something vague near the definition of buffer about how we expect that the buffer getting too full / too empty will suspend/resume.

@saschanaz saschanaz Jan 23, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Hmm, I was lazy and hoping I could just reuse the existing things... but you're right, since the HWM is now zero the desired size cannot be a positive number. I'll try adding some notes below buffer.

@saschanaz saschanaz Jan 23, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Actually I instead moved this step back to the fetching algorithm and made it to sleep if the buffer becomes larger than a user-agent defined limit. I think that works too?

(It's me who just don't want to describe about the network layer which is not exactly in my area)

</ol>

<li><p>Let <var>cancelAlgorithm</var> be an algorithm that <a for="fetch controller">aborts</a>
<var>fetchParams</var>'s <a for="fetch params">controller</a> with <var>reason</var>, given
<var>reason</var>.

<li><p>Let <var>highWaterMark</var> be a non-negative, non-NaN number, chosen by the user agent.

<li><p>Let <var>sizeAlgorithm</var> be an algorithm that accepts a <a>chunk</a> object and returns
a non-negative, non-NaN, non-infinite number, chosen by the user agent.

<li><p>Let <var>stream</var> be a <a>new</a> {{ReadableStream}}.

<li><p><a for=ReadableStream>Set up</a> <var>stream</var> with
<a for="ReadableStream/set up"><var>pullAlgorithm</var></a> set to <var>pullAlgorithm</var>,
<a for="ReadableStream/set up"><var>cancelAlgorithm</var></a> set to <var>cancelAlgorithm</var>,
<a for="ReadableStream/set up"><var>highWaterMark</var></a> set to <var>highWaterMark</var>, and
<a for="ReadableStream/set up"><var>sizeAlgorithm</var></a> set to <var>sizeAlgorithm</var>.
<li><p>[=ReadableStream/set up with byte reading support|Set up=] |stream| with byte reading
support with <var>[=ReadableStream/set up/pullAlgorithm=]</var> set to |pullAlgorithm|,
<var>[=ReadableStream/set up/cancelAlgorithm=]</var> set to |cancelAlgorithm|.

<li><p>Set <var>response</var>'s <a for=response>body</a> to a new <a for=/>body</a> whose
<a for=body>stream</a> is <var>stream</var>.
Expand Down Expand Up @@ -5986,15 +6016,7 @@ optional boolean <var>forceNewConnection</var> (default false), run these steps:
<li><p>If <var>bytes</var> is failure, then <a for="fetch controller">terminate</a>
<var>fetchParams</var>'s <a for="fetch params">controller</a>.

<li><p><a for=ReadableStream>Enqueue</a> a {{Uint8Array}} wrapping an {{ArrayBuffer}}
containing <var>bytes</var> into <var>stream</var>.

<li><p>If <var>stream</var> is <a for=ReadableStream>errored</a>, then
<a for="fetch controller">terminate</a> <var>fetchParams</var>.'s
<a for="fetch params">controller</a>.

<li><p>If <var>stream</var> doesn't <a for=ReadableStream>need more data</a> ask the user
agent to <a for=fetch>suspend</a> the ongoing fetch.
<li><p>Append |bytes| to |buffer|.
</ol>

<li><p>Otherwise, if the bytes transmission for <var>response</var>'s message body is done
Expand Down Expand Up @@ -6723,7 +6745,8 @@ steps:
running <var>object</var>'s <a for=Blob>get stream</a>.

<li><p>Otherwise, set <var>stream</var> to a <a>new</a> {{ReadableStream}} object, and
<a for=ReadableStream>set up</a> <var>stream</var>.
[=ReadableStream/set up with byte reading support|set up=] <var>stream</var> with byte reading
support.

<li><p><a for=/>Assert</a>: <var>stream</var> is a {{ReadableStream}} object.

Expand Down