Re: [whatwg/fetch] Deferred fetching (PR #1647)

@mingyc commented on this pull request.

noamr@ PTAL. I've added some more questions and comments. Really thanks for your help!

> +<dfn for=FetchLaterResult>deferred record</dfn>.
+
+<div algorithm>
+<p>The <dfn attribute for=FetchLaterResult><code>sent</code></dfn> getter steps are to return
+<a>this</a>'s <a for=FetchLaterResult>deferred record</a>'s <a for="deferred fetch record">sent</a>.
+</div>
+
+<div algorithm="dom-fetch-later">
+<p>The
+<dfn id=dom-global-fetch-later method for=WindowOrWorkerGlobalScope><code>fetchLater(<var>input</var>, <var>init</var>)</code></dfn>
+method steps are:
+
+<ol>
+ <li><p>Let <var>requestObject</var> be the result of invoking the initial value of {{Request}} as
+ constructor with <var>input</var> and <var>init</var> as arguments. If that threw an exception,
+ <a for=/>reject</a> <var>promise</var> with that exception and return <var>promise</var>.

> <a for=/>reject</a> <var>promise</var> ... return <var>promise</var>

As `fetchLater()` nows return `FetchLaterResult`, is it still a promise?

> +<p>The <dfn attribute for=FetchLaterResult><code>sent</code></dfn> getter steps are to return
+<a>this</a>'s <a for=FetchLaterResult>deferred record</a>'s <a for="deferred fetch record">sent</a>.
+</div>
+
+<div algorithm="dom-fetch-later">
+<p>The
+<dfn id=dom-global-fetch-later method for=WindowOrWorkerGlobalScope><code>fetchLater(<var>input</var>, <var>init</var>)</code></dfn>
+method steps are:
+
+<ol>
+ <li><p>Let <var>requestObject</var> be the result of invoking the initial value of {{Request}} as
+ constructor with <var>input</var> and <var>init</var> as arguments. If that threw an exception,
+ <a for=/>reject</a> <var>promise</var> with that exception and return <var>promise</var>.
+
+ <li><p>If <var>requestObject</var>'s <a for=Request>signal</a> is <a for=AbortSignal>aborted</a>,
+ then <a for=/>reject</a> <var>promise</var> with <var>requestObject</var>'s

> then <a for=/>reject</a> <var>promise</var> with <var>requestObject</var>'s <a for=Request>signal</a>'s <a for=AbortSignal>abort reason</a> and return <var>promise</var>

It looks like this step does not match the discussion in https://github.com/whatwg/fetch/pull/1647#discussion_r1223908043 where `abort` should only work if `currentState` is not yet `sent`?



> + <li><p>Let <var>request</var> be <var>requestObject</var>'s <a for=Request>request</a>.
+
+ <li><p>Let <var>backgroundTimeout</var> be null.
+
+ <li><p>If <var>init</var> is given and <var>init</var>["<code>backgroundTimeout</code>"]
+ <a for=map>exists</a> then set <var>backgroundTimeout</var> to
+ <var>init</var>["<code>backgroundTimeout</code>"].
+
+ <li><p>If <var>backgroundTimeout</var> is not a {{DOMHighResTimeStamp}} then throw a {{TypeError}}.
+
+ <li><p>Let <var>deferredRecord</var> be the result of calling
+ <a>request a deferred fetch</a> given <var>request</var> and <var>backgroundTimeout</var>. This
+ may throw an exception.
+
+ <li>
+  <p><a for=AbortSignal lt=add>Add the following abort steps</a> to <var>requestObject</var>'s

> <p><a for=AbortSignal lt=add>Add the following abort steps</a> to <var>requestObject</var>'s

Should it also specify in what condition can the abort happen?

> -<a>done flag</a> is unset or <a for=request>keepalive</a> is false,
-<a for="fetch controller">terminate</a> the <a for="fetch group">fetch record</a>'s
-<a for="fetch record">controller</a>.
+<p>When a <a for=fetch>fetch group</a> <var>fetchGroup</var> is
+<dfn export for="fetch group" id=concept-fetch-group-terminate>terminated</dfn>:
+
+<ol>
+ <li><p>For each associated <a for="fetch group">fetch record</a> <var>record</var>,
+ if <var>record</var>'s <a for="fetch record">controller</a> is non-null and
+ <var>record</var>'s <a for="fetch record">request</a>'s <a>done flag</a> is unset and
+ <a for=request>keepalive</a> is false, <a for="fetch controller">terminate</a> <var>record</var>'s
+ <a for="fetch record">controller</a>.
+
+ <li><p><a for=list>For each</a> <a for="fetch group">deferred fetch record</a>
+ <var>deferredRecord</var> in <var>fetchGroup</var>'s
+ <a for="fetch group">deferred fetch records</a>: If the result of atomically exchanging the value

> If the result of atomically exchanging the value of <var>deferredRecord</var>'s <a for="deferred fetch record">invoke state</a> with
 "<code>terminated</code>" is not "<code>sent</code>"

Could you please help me understand what does `atomically exchanging the value of deferredRecord' invoke state with terminated`, does it mean the result of calling [request a deferred fetch](https://whatpr.org/fetch/1647/094ea69...02a45de.html#request-a-deferred-fetch) or simply setting `deferredRecord.invokeState` to something? What is the subject the deferredRecord is exchanging invoke state with?

> + if <var>record</var>'s <a for="fetch record">controller</a> is non-null and
+ <var>record</var>'s <a for="fetch record">request</a>'s <a>done flag</a> is unset and
+ <a for=request>keepalive</a> is false, <a for="fetch controller">terminate</a> <var>record</var>'s
+ <a for="fetch record">controller</a>.
+
+ <li><p><a for=list>For each</a> <a for="fetch group">deferred fetch record</a>
+ <var>deferredRecord</var> in <var>fetchGroup</var>'s
+ <a for="fetch group">deferred fetch records</a>: If the result of atomically exchanging the value
+ of <var>deferredRecord</var>'s <a for="deferred fetch record">invoke state</a> with
+ "<code>terminated</code>" is not "<code>sent</code>", then the user agent should
+ <a for=/>fetch</a> <var>deferredRecord</var>'s <a for="deferred fetch record">request</a>. The
+ exact time in which the <a for=/>fetch</a> takes place is <a>implementation-defined</a>.
+</ol>
+
+<p>When a <a for=fetch>fetch group</a> <var>fetchGroup</var> is
+<dfn export for="fetch group" id=concept-fetch-group-deactivate>deactivated</dfn>:

> <p>When a <a for=fetch>fetch group</a> <var>fetchGroup</var> is <dfn export for="fetch group" id=concept-fetch-group-deactivate>deactivated</dfn>:

It's not clearly defined what `deactivated` fetch group is. Is it equivalent to a Document being unloaded or backgrounded?

> +   optimize batching of deferred fetches.
+
+   <li><p>If the result of atomically exchanging the value of <var>deferredRecord</var>'s
+   <a for="deferred fetch record">invoke state</a> with "<code>sent</code>" is
+   "<code>scheduled</code>", then <a for=/>fetch</a> <var>record</var>'s
+   <a for="fetch record">request</a>.
+  </ol>
+ </li>
+</ol>
+
+<p>When a <a for=fetch>fetch group</a> <var>fetchGroup</var> is
+<dfn export for="fetch group" id=concept-fetch-group-reactivate>reactivated</dfn>:
+<a for=list>For each</a> <a for=/>deferred fetch record</a> <var>deferredRecord</var> in
+<var>fetchGroup</var>'s <a for="fetch group">deferred fetch records</a>: If the result of atomically
+exchanging the value of <var>deferredRecord</var>'s <a for="deferred fetch record">invoke state</a>
+with "<code>deferred</code>" is "<code>sent</code>", then <a for=list>remove</a>

> <p>When a <a for=fetch>fetch group</a> <var>fetchGroup</var> is <dfn export for="fetch group" id=concept-fetch-group-reactivate>reactivated</dfn>:
> If the result of atomically exchanging the value of <var>deferredRecord</var>'s <a for="deferred fetch record">invoke state</a> with "<code>deferred</code>" is "<code>sent</code>", then <a for=list>remove</a> <var>deferredRecord</var> from <var>fetchGroup</var>'s <a for="fetch group">deferred fetch records</a> and set <var>deferredRecord</var>'s <a for="deferred fetch record">sent</a> to true.

Assuming `reactivated` means going out of bfcache (background to foreground): I feel that `sent` (or `committed`) deferredRecord should not be remove here. Can't this also be done in `deactivated` just following after the step that performs fetching the request?

> @@ -2689,6 +2689,9 @@ functionality.
 <p>A <a for=fetch>fetch group</a> holds an ordered list of
 <dfn lt="fetch record" export for="fetch group" id=concept-fetch-record>fetch records</dfn>.
 
+<p>A <a for=fetch>fetch group</a> holds an ordered list of

> Each [environment settings object ](https://html.spec.whatwg.org/multipage/webappapis.html#environment-settings-object)has an associated fetch group .
> <p>A <a for=fetch>fetch group</a> holds an ordered list of <dfn export for="fetch group" id=concept-defer=fetch-record>deferred fetch records</dfn>.

The above implies that a deferred fetch records belong to a Document, even after the Document is unloaded. In practice, user agent will have to put deferred fetch records in other context outside Document (e.g. browser process) in implementation. Does it make sense?

> +   <a for=request>body</a>'s <a for=body>length</a>.
+  </ol>
+ </li>
+
+ <li><p><a for=list>For each</a> <a>deferred fetch record</a> <var>deferredRecord</var> in
+ <var>request</var>'s <a for=request>client</a>'s <a for=fetch>fetch group</a>'s
+ <a for="fetch group">deferred fetch records</a>: if <var>deferredRecord</var>'s
+ <a for="deferred fetch record">request</a>'s <a for=request>body</a> is not null and
+ <var>deferredRecord</var>'s <a for="deferred fetch record">request</a>'s <a for=request>URL</a>'s
+ <a for=url>origin</a> is <a>same origin</a> with <var>request</var>'s <a for=request>URL</a>'s
+ <a for=url>origin</a>, then increment <var>totalScheduledDeferredBytesForOrigin</var> by
+ <var>deferredRecord</var>'s <a for="deferred fetch record">request</a>'s <a for=request>body</a>'s
+ <a for=body>length</a>.
+
+ <li><p>If <var>totalScheduledDeferredBytesForOrigin</var> is greater than 64 kilobytes, then
+ throw a {{QuotaExceededError}}.

Thanks for explaining. I added some more questions below

> in  request’s [ client ](https://whatpr.org/fetch/1647/094ea69...02a45de.html#concept-request-client)’s [ fetch group ](https://whatpr.org/fetch/1647/094ea69...02a45de.html#concept-fetch-group) ’s [ deferred fetch records ](https://whatpr.org/fetch/1647/094ea69...02a45de.html#concept-defer=fetch-record): ...
> if  deferredRecord’s [ request ](https://whatpr.org/fetch/1647/094ea69...02a45de.html#deferred-fetch-record-request)’s [ body ](https://whatpr.org/fetch/1647/094ea69...02a45de.html#concept-request-body) is not null and  deferredRecord’s [ request ](https://whatpr.org/fetch/1647/094ea69...02a45de.html#deferred-fetch-record-request)’s [ URL ](https://whatpr.org/fetch/1647/094ea69...02a45de.html#concept-request-url)’s [ origin ](https://url.spec.whatwg.org/#concept-url-origin) is [ same origin ](https://html.spec.whatwg.org/multipage/browsers.html#same-origin) with  request’s [ URL ](https://whatpr.org/fetch/1647/094ea69...02a45de.html#concept-request-url)’s [ origin ](https://url.spec.whatwg.org/#concept-url-origin) ...
> If  `totalScheduledDeferredBytesForOrigin`  is greater than 64 kilobytes, then throw a [ QuotaExceededError ](https://webidl.spec.whatwg.org/#quotaexceedederror).

It means `totalScheduledDeferredBytesForOrigin` only accumulates request lenth from same-origin deferred fetch within the same fetch group (e.g. same Document).

1. You mentioned that the quota is `per-target-origin quota`, how will this work with  records from different fetch group?
2. `each origin manages its own and autoflushes` is not specified in above. As Fergal mentioned, it would be difficult for userland code to track request size and flush by themselves. Could we specify auto flushes in FIFO order?

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

Message ID: <whatwg/fetch/pull/1647/review/1504462309@github.com>