Skip to content

Add race-network-and-fetch-handler option #10

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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
yoshisatoyanagisawa merged 20 commits into from
Feb 16, 2024
Merged

Add race-network-and-fetch-handler option #10

Changes from 6 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
569c3de
Add `race-network-and-fetch-handler` option
sisidovski Feb 2, 2024
ad5f0b9
Abort the request if the fetch handler respond
sisidovski Feb 5, 2024
10db402
Fix nav preload condition, indent, FetchEvent
sisidovski Feb 6, 2024
e6e70bc
Split Handle Fetch
sisidovski Feb 6, 2024
e18d7ea
Fix id name and tags
sisidovski Feb 6, 2024
210fd4f
Restore old sentence
sisidovski Feb 6, 2024
8e087c8
Abort race request if not needed
sisidovski Feb 6, 2024
2e86804
Fixed passing the error to [=fetch controller/Abort=], indentation, 2…
sisidovski Feb 8, 2024
b9d6c63
Collapse some redundant steps, make enum multiline, fixed abot, remov…
sisidovski Feb 9, 2024
67561a6
Add fallback case
sisidovski Feb 9, 2024
f8a0d3d
Add token and lookup algorithm
sisidovski Feb 14, 2024
fd5b794
Drop incorrect note
sisidovski Feb 14, 2024
aa42d55
Fix incorrect format, update the map key to be request
sisidovski Feb 15, 2024
3245241
Do not use promise to manage [=/response=]
sisidovski Feb 15, 2024
01c0773
Added the race response struct
sisidovski Feb 15, 2024
6d43556
Update race response struct
sisidovski Feb 15, 2024
27ccc75
Remove unconsumed map entry
sisidovski Feb 15, 2024
1cd657a
Move adidng an entry after `run service worker`
sisidovski Feb 16, 2024
4765d5c
Merge branch 'static_routing_api' into patch-2
sisidovski Feb 16, 2024
bf4bdaf
Fix editorial issues
sisidovski Feb 16, 2024
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
54 changes: 48 additions & 6 deletions docs/index.bs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1578,7 +1578,7 @@ spec: storage; urlPrefix: https://storage.spec.whatwg.org/
};

enum RunningStatus { "running", "not-running" };
enum RouterSourceEnum { "cache", "fetch-event", "network" };
enum RouterSourceEnum { "cache", "fetch-event", "network", "race-network-and-fetch-handler" };
Comment thread
sisidovski marked this conversation as resolved.
Outdated
</pre>

<section>
Expand Down Expand Up @@ -3108,16 +3108,12 @@ spec: storage; urlPrefix: https://storage.spec.whatwg.org/
: Output
:: |response|, a [=/response=]

1. Let |handleFetchFailed| be false.
1. Let |respondWithEntered| be false.
1. Let |eventCanceled| be false.
1. Let |response| be null.
1. Let |registration| be null.
1. Let |client| be |request|'s [=request/client=].
1. Let |reservedClient| be |request|'s [=request/reserved client=].
1. Let |preloadResponse| be a new [=promise=].
1. Let |workerRealm| be null.
1. Let |eventHandled| be null.
1. Let |timingInfo| be a new [=service worker timing info=].
1. Assert: |request|'s [=request/destination=] is not "<code>serviceworker</code>".
1. If |request|'s [=request/destination=] is either "<code>embed</code>" or "<code>object</code>", then:
Expand Down Expand Up @@ -3154,7 +3150,30 @@ spec: storage; urlPrefix: https://storage.spec.whatwg.org/
1. If |client| is not null, |response|'s [=response/type=] is "`opaque`", and [=cross-origin resource policy check=] with |request|'s [=request/origin=], |client|, "", and |response|'s [=filtered response/internal response=] returns <b>blocked</b>, then return null.
1. Return |response|.
1. Return null.
1. If |request| is a <a>non-subresource request</a>, |request| is a [=navigation request=], |registration|'s [=navigation preload enabled flag=] is set, |request|'s [=request/method=] is \`<code>GET</code>\`, |registration|'s [=active worker=]'s [=set of event types to handle=] [=set/contains=] <code>fetch</code>, and |registration|'s [=active worker=]'s [=all fetch listeners are empty flag=] is not set then:
1. Else if |source| is {{RouterSourceEnum/"race-network-and-fetch-handler"}}, and |request|'s [=request/method=] is \`<code>GET</code>\` then:

Note: In order to avoid duplicated fetches to |request|, the user agent may reuse |raceNetworkRequestResponse| as the result of {{FetchEvent}}'s [=FetchEvent/potential response=], or the fallback request.
Copy link
Copy Markdown

@domenic domenic Feb 9, 2024

Choose a reason for hiding this comment

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

This seems like it's specifying a possibly significant and observable step, via a vague non-normative note. Am I understanding correctly?

It seems to me you need to have some place to store the raceNetworkRequestResponse, modify all the call sites that set the potential response, instead.

Copy link
Copy Markdown
Collaborator Author

@sisidovski sisidovski Feb 9, 2024

Choose a reason for hiding this comment

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

Yes, your understanding is correct. Let me try writing this behavior in a spec level.

Copy link
Copy Markdown
Collaborator Author

@sisidovski sisidovski Feb 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Updated a bit to capture the case when the user agent reuses the network response for the fallback.

The other part is more problematic.

the user agent may reuse |raceNetworkRequestResponse| as the result of {{FetchEvent}}'s [=FetchEvent/potential response=]

Actually this sentense was wrong. We have a dedupe mechanism for the |request| level, that means if fetch(e.request) is called inside the fetch handler, the user agent reuse the response dispatched before. But the response of fetch(e.request) is not [=FetchEvent/potential response=]. Partial response basically means the returned value in e.respondWith(), which may use fetch(e.request) response internally, but that's usually differnt data'.

I'm not sure how I can write this behavior in the spec, any advice would be appreciated.

Copy link
Copy Markdown

@domenic domenic Feb 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It sounds like you'd need to add quite a bit of spec machinery here, to parallel the implementation machinery. Probably that would involve a PR to https://fetch.spec.whatwg.org/ to modify the behavior of fetch(), or maybe one of the underlying algorithms like main fetch or HTTP-network fetch. And some sort of side table tracking the request -> response that can be used to short circuit the normal fetching, etc.

Basically, how did you implement this in Chromium? You'll need to write up equivalent spec text.

Copy link
Copy Markdown
Collaborator Author

@sisidovski sisidovski Feb 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Updated, diff from the last patch is:

  1. Added request/service-workers race token and set it when the race network request is created.
  2. Added race response map in ServiceWorkerGlobalScope. The map takes a token string as a key, and a promise that will be resolved with the response as a value.
  3. Added Lookup Race Response. That returns the promise if there is an entry in the map, otherwise null. That will be called from Main fetch in the fetch spec.

I also updated the fetch spec.

  1. Added >service-workers race token as an associated property of request.
  2. Added the logic to return response early if there is a corresponding entry with the token string.


1. Let |queue| be an empty [=queue=] of [=/response=].
1. Let |raceFetchController| be null.
Comment thread
yoshisatoyanagisawa marked this conversation as resolved.
1. Run the following substeps [=in parallel=], but [=abort when=] |controller|'s [=fetch controller/state=] is "<code>terminated</code>" or "<code>aborted</code>":
1. Set |raceFetchController| to the result of [=Fetch|fetching=] |request|.
Comment thread
sisidovski marked this conversation as resolved.
Outdated
1. To [=fetch/processResponse=] for |raceNetworkRequestResponse|, run these substeps:
Comment thread
sisidovski marked this conversation as resolved.
Outdated
1. If |raceNetworkRequestResponse|'s [=response/type=] is "`error`", then:
1. Terminate these substeps.
1. If |raceNetworkRequestResponse|'s [=response/status=] is `200`, then:
Comment thread
sisidovski marked this conversation as resolved.
Outdated
1. [=queue/Enqueue=] |raceNetworkRequestResponse| to |queue|.
Comment thread
sisidovski marked this conversation as resolved.
1. [=If aborted=], then:
1. Let |deserializedError| be the result of [=deserialize a serialized abort reason=] given null and |workerRealm|.
Comment thread
sisidovski marked this conversation as resolved.
Outdated
1. [=fetch controller/Abort=] |raceFetchController| with |deserializedError|.
Comment thread
sisidovski marked this conversation as resolved.
Outdated
1. Run the following substeps [=in parallel=]:
1. Resolve |preloadResponse| with undefined.
Comment thread
sisidovski marked this conversation as resolved.
Outdated
1. Let |fetchHandlerResponse| to the result of [=Create Fetch Event and Dispatch=] with |request|, |registration|, |useHighResPerformanceTimers|, |timingInfo|, |workerRealm|, |reservedClient|, and |preloadResponse|.
1. [=queue/Enqueue=] |fetchHandlerResponse| to |queue|.
1. Wait until |queue| is not empty.
1. Set |response| to the result of [=dequeue=] |queue|.
1. Return |response|.
Comment thread
sisidovski marked this conversation as resolved.
Outdated
1. If |request| is a <a>non-subresource request</a>, |request| is a [=navigation request=], |registration|'s [=navigation preload enabled flag=] is set, |request|'s [=request/method=] is \`<code>GET</code>\`, |registration|'s [=active worker=]'s [=set of event types to handle=] [=set/contains=] <code>fetch</code>, and |registration|'s [=active worker=]'s [=all fetch listeners are empty flag=] is not set, then:

Note: If the above is true except |registration|'s [=active worker=]'s <a>set of event types to handle</a> **does not** contain <code>fetch</code>, then the user agent may wish to show a console warning, as the developer's intent isn't clear.

Expand All @@ -3176,6 +3195,29 @@ spec: storage; urlPrefix: https://storage.spec.whatwg.org/
1. Let |deserializedError| be the result of [=deserialize a serialized abort reason=] given null and |workerRealm|.
1. [=fetch controller/Abort=] |preloadFetchController| with |deserializedError|.
1. Else, resolve |preloadResponse| with undefined.
1. Set |response| to the result of [=Create Fetch Event and Dispatch=] with |request|, |registration|, |useHighResPerformanceTimers|, |timingInfo|, |workerRealm|, |reservedClient|, and |preloadResponse|.
1. Retrun |response|.
Comment thread
sisidovski marked this conversation as resolved.
Outdated
</section>

<section algorithm>
<h3 id="create-fetch-event-and-dispatch-algorithm"><dfn>Create Fetch Event and Dispatch</dfn></h3>
: Input
:: |request|, a [=/request=]
:: |registration|, a [=/service worker registration=]
:: |useHighResPerformanceTimers|, a boolean
:: |timingInfo|, a [=service worker timing info=].
Comment thread
sisidovski marked this conversation as resolved.
Outdated
:: |workerRealm|, a [=relevant realm=] of the [=service worker/global object=].
Comment thread
sisidovski marked this conversation as resolved.
Outdated
:: |reservedClient|, a [=request/reserved client=].
Comment thread
sisidovski marked this conversation as resolved.
Outdated
:: |preloadResponse| a [=promise=].
Comment thread
sisidovski marked this conversation as resolved.
Outdated
: Output
:: |response|, a [=/response=]
Comment thread
sisidovski marked this conversation as resolved.
Outdated

1. Let |eventCanceled| be false.
Comment thread
sisidovski marked this conversation as resolved.
1. Let |client| be |request|'s [=request/client=].
1. Let |activeWorker| be |registration|'s <a>active worker</a>.
1. Let |eventHandled| be null.
1. Let |handleFetchFailed| be false.
1. Let |respondWithEntered| be false.
1. Let |shouldSoftUpdate| be true if any of the following are true, and false otherwise:
* |request| is a [=non-subresource request=].
* |request| is a [=subresource request=] and |registration| is [=stale=].
Expand Down