Return a "controller" instance from fetch #1329

noamr · 2021-10-12T08:39:54Z

Use the controller for external timing reporting and aborts.

This allows removing the 1:1 dependency between timing info and a response, as a response can be shared across fetch
instances (e.g. when restoring a response from cache, or when forwarding a response through a service worker).

See #1208

At least two implementers are interested (and none opposed):
- Should reflect current implementation.
Tests are written and can be reviewed and commented upon at:
- …
Implementation bugs are filed:
- Chrome: …
- Firefox: …
- Safari: …

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

Preview | Diff

fetch.bs

noamr · 2021-10-19T08:46:29Z

@annevk i think this is ready for another review, thanks!

annevk

This good, thanks for working on this! I suggest we wait a bit with landing this until @jakearchibald is back home (next week iirc) as he worked on this last and might have some relevant insights.

fetch.bs

annevk

\o/ looks even better. I'll file a follow-up on where we should define fetch params and such.

This PR should also update the "Invoking fetch" section with a few words about the return value of fetch.

fetch.bs

jakearchibald

Just a few nits really.

fetch.bs

jakearchibald · 2021-10-27T10:30:42Z

fetch.bs

@@ -2247,14 +2278,14 @@ functionality.

 <p>A <a for="fetch group">fetch record</a> has an associated
 <dfn export for="fetch record" id=concept-fetch-record-fetch>fetch</dfn> (a
-<a for=/>fetch</a> algorithm or null).
+<a for=/>fetch controller</a> or null).


I can't see any cases where it's null.

I suggest we tackle this in a follow-up.

Can you file an issue for this?

fetch.bs

noamr

Fixed the detailed CR comments.

fetch.bs

annevk · 2021-11-03T10:00:47Z

As noted on Matrix we need to account for service workers and in particular "handle fetch". Either we make it so that "handle fetch" returns a controller for the service worker fetch event, or we pass the controller into "handle fetch" so the service worker can observe it. The former seems a bit nicer as it reuses the pattern introduced here and could be used for other kinds of information exchange in the future. It would require "handle fetch" to take a callback.

@yutakahirano @jakearchibald @noamr thoughts?

noamr · 2021-11-03T10:02:37Z

As noted on Matrix we need to account for service workers and in particular "handle fetch". Either we make it so that "handle fetch" returns a controller for the service worker fetch event, or we pass the controller into "handle fetch" so the service worker can observe it. The former seems a bit nicer as it reuses the pattern introduced here and could be used for other kinds of information exchange in the future. It would require "handle fetch" to take a callback.

@yutakahirano @jakearchibald @noamr thoughts?

I like the idea of returning a controller from SW's fetch rather than having SW query its caller fetch for "is terminated"...
Does it affect this PR?

annevk · 2021-11-03T10:05:24Z

Yeah, I think we need to tackle it as otherwise fetch termination in service workers ends up in UB territory.

noamr · 2021-11-03T10:14:38Z

Yeah, I think we need to tackle it as otherwise fetch termination in service workers ends up in UB territory.

It is currently in undefined behavior territory, this doesn't change much about it. can't we fix this iteratively?

In conjunction with whatwg/fetch#1329. When the SW fetch starts, it emits an `onStart` callback with a controller instance. That controller instance can be used to terminate the service worker fetch and it's internal fetches.

noamr · 2021-11-08T13:02:03Z

As noted on Matrix we need to account for service workers and in particular "handle fetch". Either we make it so that "handle fetch" returns a controller for the service worker fetch event, or we pass the controller into "handle fetch" so the service worker can observe it. The former seems a bit nicer as it reuses the pattern introduced here and could be used for other kinds of information exchange in the future. It would require "handle fetch" to take a callback.

@yutakahirano @jakearchibald @noamr thoughts?

See w3c/ServiceWorker#1612 and latest revision of this PR.

Instead of returning a controller, we emit it in a new onFetchStart callback
Termination of the service worker and of fetches initiated by the service worker are done explicitly

(It currently fails to build because of the dependency on the SW patch)

fetch.bs

annevk

Are you working on a new service worker PR? I think the Fetch side looks good, though I'd like to do another detailed read once we have the full story. @jakearchibald what do you think?

fetch.bs

noamr · 2021-11-19T13:04:20Z

@annevk I was working on the corresponding SW PR: #1329 and I see that it's closed for some reason, I must have closed it by mistake and I don't seem to be able to re-open. Opened a new one: w3c/ServiceWorker#1620

fetch.bs

annevk · 2021-11-22T12:53:16Z

fetch.bs

@@ -2247,14 +2278,14 @@ functionality.

 <p>A <a for="fetch group">fetch record</a> has an associated
 <dfn export for="fetch record" id=concept-fetch-record-fetch>fetch</dfn> (a
-<a for=/>fetch</a> algorithm or null).
+<a for=/>fetch controller</a> or null).


I suggest we tackle this in a follow-up.

annevk · 2021-12-03T10:21:14Z

@jakearchibald @yutakahirano could you please review this PR as well as w3c/ServiceWorker#1620?

noamr · 2022-02-07T15:06:03Z

@annevk ping? The corresponding service-workers PR has already been merged.

annevk

Found a couple minor remaining issues. Also pinging @jakearchibald and @yutakahirano again for review.

fetch.bs

annevk · 2022-02-14T13:29:55Z

fetch.bs

@@ -2247,14 +2278,14 @@ functionality.

 <p>A <a for="fetch group">fetch record</a> has an associated
 <dfn export for="fetch record" id=concept-fetch-record-fetch>fetch</dfn> (a
-<a for=/>fetch</a> algorithm or null).
+<a for=/>fetch controller</a> or null).


Can you file an issue for this?

fetch.bs

Co-authored-by: Anne van Kesteren <[email protected]>

noamr · 2022-02-28T12:38:42Z

Found a couple minor remaining issues. Also pinging @jakearchibald and @yutakahirano again for review.

@jakearchibald ping?

jakearchibald

LGTM. One comment, but it can be a followup

jakearchibald · 2022-03-10T10:25:55Z

fetch.bs

+
+<dl>
+ <dt><dfn for="fetch controller">state</dfn> (default "<code>ongoing</code>")
+ <dd>"<code>ongoing</code>", "<code>terminated</code>", or "<code>aborted</code>"


Should there be a state that represents a fetch that can no longer be aborted or terminated? I guess that would be "complete".

It seems weird that a fully completed fetch would have a state of "ongoing".

Yes! I will add it when "Finalize and report timing" would move to the fetch controller, otherwise no one would set that value :)

annevk · 2022-03-11T10:40:40Z

This looks good to me too. One thing we'll need is a tentative commit message that summarizes the changes and links the various follow-up issues, the resolved issues, and the related service worker PR. Is that something you can put together @noamr? In a new comment would be best.

noamr · 2022-03-11T10:45:40Z

This looks good to me too. One thing we'll need is a tentative commit message that summarizes the changes and links the various follow-up issues, the resolved issues, and the related service worker PR. Is that something you can put together @noamr? In a new comment would be best.

Sure, I will post follow up issues and comment with a commit message.

noamr · 2022-03-13T07:24:43Z

Commit message:

Return a "controller" instance from the fetch algorithm, which allows clients of fetch to perform certain actions on an ongoing fetch, initially to abort or terminate it, and later on to conclude it.

This will eventually remove the need of using response as the object that retains timing info, as that creates certain ambiguities.

Followup action items:
#1412

annevk reviewed Oct 13, 2021

View reviewed changes

fetch.bs Outdated Show resolved Hide resolved

noamr force-pushed the controller branch from a9c1380 to da4db3d Compare October 14, 2021 14:17

annevk reviewed Oct 21, 2021

View reviewed changes

fetch.bs Outdated Show resolved Hide resolved

fetch.bs Outdated Show resolved Hide resolved

fetch.bs Outdated Show resolved Hide resolved

noamr force-pushed the controller branch from da4db3d to 89988e8 Compare October 21, 2021 15:21

annevk reviewed Oct 22, 2021

View reviewed changes

fetch.bs Outdated Show resolved Hide resolved

fetch.bs Outdated Show resolved Hide resolved

fetch.bs Outdated Show resolved Hide resolved

fetch.bs Outdated Show resolved Hide resolved

annevk mentioned this pull request Oct 22, 2021

Editorial: find a new home for "fetch params" and friends #1340

Open

annevk requested a review from jakearchibald October 25, 2021 12:05

yutakahirano mentioned this pull request Oct 27, 2021

Add abort reason to abort fetch #1343

Merged

3 tasks

jakearchibald requested changes Oct 27, 2021

View reviewed changes

noamr force-pushed the controller branch from 06473cc to cf0e8d0 Compare October 27, 2021 16:39

noamr commented Oct 27, 2021

View reviewed changes

fetch.bs Outdated Show resolved Hide resolved

noamr mentioned this pull request Nov 8, 2021

When response comes from a worker, some of the timing-info should pass through #1208

Closed

noamr mentioned this pull request Nov 8, 2021

Specify how "terminate" works w3c/ServiceWorker#1612

Closed

annevk reviewed Nov 16, 2021

View reviewed changes

fetch.bs Outdated Show resolved Hide resolved

fetch.bs Outdated Show resolved Hide resolved

noamr force-pushed the controller branch from 8a555fe to fccc4d2 Compare November 17, 2021 16:52

annevk reviewed Nov 19, 2021

View reviewed changes

fetch.bs Outdated Show resolved Hide resolved

noamr mentioned this pull request Nov 19, 2021

Pass controller from fetch, and abort SW fetch when it's canceled w3c/ServiceWorker#1620

Merged

annevk reviewed Nov 22, 2021

View reviewed changes

annevk requested review from jakearchibald and yutakahirano November 22, 2021 12:54

noamr and others added 10 commits January 17, 2022 17:37

Use controller for terminating

925806a

Use enum instead of flag

f4d1a55

Rename terminated to canceled, remove note

643aaa1

Add some prose

b634ecd

nits

2f5922c

nits

010c0d3

Fix many CR comments

2014a3b

Invert controller to include only state

a045089

Pass controller to SW

3402759

Switch up order of parameters

ec4c543

noamr force-pushed the controller branch from 6558469 to ec4c543 Compare January 17, 2022 15:38

noamr mentioned this pull request Feb 7, 2022

A scenario where transferSize is 0 for non cached loads (or may be not) w3c/navigation-timing#146

Closed

annevk reviewed Feb 14, 2022

View reviewed changes

noamr and others added 4 commits February 14, 2022 15:37

Update fetch.bs

8be225f

Co-authored-by: Anne van Kesteren <[email protected]>

Update fetch.bs

6a9f4d0

Co-authored-by: Anne van Kesteren <[email protected]>

Update fetch.bs

06fe6b9

Co-authored-by: Anne van Kesteren <[email protected]>

Fix lt

7899e79

kicking build

445af05

jakearchibald approved these changes Mar 10, 2022

View reviewed changes

formatting nits

dc7f07c

noamr mentioned this pull request Mar 13, 2022

Followup: controller #1412

Open

4 tasks

annevk merged commit 4561d5f into whatwg:main Mar 14, 2022

noamr deleted the controller branch March 14, 2022 13:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Return a "controller" instance from fetch #1329

Return a "controller" instance from fetch #1329

noamr commented Oct 12, 2021 •

edited by pr-preview bot

Loading

noamr commented Oct 19, 2021

annevk left a comment

annevk left a comment

jakearchibald left a comment

jakearchibald Oct 27, 2021

annevk Nov 22, 2021

annevk Feb 14, 2022

noamr Feb 14, 2022

noamr left a comment

annevk commented Nov 3, 2021

noamr commented Nov 3, 2021

annevk commented Nov 3, 2021

noamr commented Nov 3, 2021

noamr commented Nov 8, 2021 •

edited

Loading

annevk left a comment

noamr commented Nov 19, 2021

annevk Nov 22, 2021

annevk commented Dec 3, 2021 •

edited

Loading

noamr commented Feb 7, 2022

annevk left a comment

annevk Feb 14, 2022

noamr commented Feb 28, 2022

jakearchibald left a comment

jakearchibald Mar 10, 2022

noamr Mar 10, 2022

annevk commented Mar 11, 2022 •

edited

Loading

noamr commented Mar 11, 2022

noamr commented Mar 13, 2022

Return a "controller" instance from fetch #1329

Return a "controller" instance from fetch #1329

Conversation

noamr commented Oct 12, 2021 • edited by pr-preview bot Loading

noamr commented Oct 19, 2021

annevk left a comment

Choose a reason for hiding this comment

annevk left a comment

Choose a reason for hiding this comment

jakearchibald left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noamr left a comment

Choose a reason for hiding this comment

annevk commented Nov 3, 2021

noamr commented Nov 3, 2021

annevk commented Nov 3, 2021

noamr commented Nov 3, 2021

noamr commented Nov 8, 2021 • edited Loading

annevk left a comment

Choose a reason for hiding this comment

noamr commented Nov 19, 2021

Choose a reason for hiding this comment

annevk commented Dec 3, 2021 • edited Loading

noamr commented Feb 7, 2022

annevk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noamr commented Feb 28, 2022

jakearchibald left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annevk commented Mar 11, 2022 • edited Loading

noamr commented Mar 11, 2022

noamr commented Mar 13, 2022

Commit message:

noamr commented Oct 12, 2021 •

edited by pr-preview bot

Loading

noamr commented Nov 8, 2021 •

edited

Loading

annevk commented Dec 3, 2021 •

edited

Loading

annevk commented Mar 11, 2022 •

edited

Loading