Re: [whatwg/fetch] Set body with byte reading support (PR #1593)

@domenic commented on this pull request.

I'm happy with this approach to the network-stack buffer, modulo my comment about a nonzero lower limit

> @@ -5914,25 +5914,59 @@ 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:
+
+  <ol>
+   <li>Let |promise| be [=a new promise=].
+
+   <li>Return |promise| and run the remaining steps [=in parallel=].
+
+   <li>If |buffer| is empty and the ongoing fetch is [=fetch/suspend|suspended=],

It shouldn't be required to be empty before you resume. So you should add a similar user-agent-defined lower limit at which it can resume.

> @@ -5914,25 +5914,59 @@ 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:
+
+  <ol>
+   <li>Let |promise| be [=a new promise=].
+
+   <li>Return |promise| and run the remaining steps [=in parallel=].

We've been avoiding this pattern these days, and instead using proper nesting.

> +
+  <ol>
+   <li>Let |promise| be [=a new promise=].
+
+   <li>Return |promise| and run the remaining steps [=in parallel=].
+
+   <li>If |buffer| is empty and the ongoing fetch is [=fetch/suspend|suspended=],
+   [=fetch/resumed|resume=] the fetch.
+
+   <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

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.

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

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.

-- 
Reply to this email directly or view it on GitHub:
https://github.com/whatwg/fetch/pull/1593#pullrequestreview-1272076751
You are receiving this because you are subscribed to this thread.

Message ID: <whatwg/fetch/pull/1593/review/1272076751@github.com>