-
Notifications
You must be signed in to change notification settings - Fork 1
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.
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
Add race-network-and-fetch-handler
option
#10
Add race-network-and-fetch-handler
option
#10
Conversation
@yoshisatoyanagisawa @domenic Could you take a look? As this is the first draft, I'm not confident with how to describe the parallelism on this option, and want to know what's missing. |
Updated to address review points and some concerns.
Note: In the race, the response from the network request is queued only when the result is successful (currently 200). On the other hand, the fetch handler response is queued regardless of the result. |
Other than my minor comments, the PR looks good to me. |
@domenic Could you take a look? Thanks! |
Some of the in-parallel stuff you're running into here is a preexisting issue with the spec, it seems. I filed whatwg/fetch#1734. I think my above advice helps avoid making the existing problem worse and so is still worth following. |
…00 to [=ok status=], updated the caller for fetch |reqeuest|
Reflecting the comment on yoshisatoyanagisawa/ServiceWorker#10 (comment) Bug: 1371756 Change-Id: I6de1fed384e32fbdae40799ef48d4ce2eeb3eccd
Reflecting the comment on yoshisatoyanagisawa/ServiceWorker#10 (comment) Bug: 1371756 Change-Id: I6de1fed384e32fbdae40799ef48d4ce2eeb3eccd Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5276401 Reviewed-by: Minoru Chikamune <[email protected]> Reviewed-by: Yoshisato Yanagisawa <[email protected]> Reviewed-by: Kouhei Ueno <[email protected]> Commit-Queue: Shunya Shishido <[email protected]> Cr-Commit-Position: refs/heads/main@{#1257762}
Reflecting the comment on yoshisatoyanagisawa/ServiceWorker#10 (comment) Bug: 1371756 Change-Id: I6de1fed384e32fbdae40799ef48d4ce2eeb3eccd Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5276401 Reviewed-by: Minoru Chikamune <[email protected]> Reviewed-by: Yoshisato Yanagisawa <[email protected]> Reviewed-by: Kouhei Ueno <[email protected]> Commit-Queue: Shunya Shishido <[email protected]> Cr-Commit-Position: refs/heads/main@{#1257762}
Reflecting the comment on yoshisatoyanagisawa/ServiceWorker#10 (comment) Bug: 1371756 Change-Id: I6de1fed384e32fbdae40799ef48d4ce2eeb3eccd Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5276401 Reviewed-by: Minoru Chikamune <[email protected]> Reviewed-by: Yoshisato Yanagisawa <[email protected]> Reviewed-by: Kouhei Ueno <[email protected]> Commit-Queue: Shunya Shishido <[email protected]> Cr-Commit-Position: refs/heads/main@{#1257762}
docs/index.bs
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: | ||
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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
- Added
request/service-workers race token
and set it when the race network request is created. - 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. - Added
Lookup Race Response
. That returns the promise if there is an entry in the map, otherwise null. That will be called fromMain fetch
in the fetch spec.
I also updated the fetch spec.
- Added
>service-workers race token
as an associated property of request. - Added the logic to return
response
early if there is a corresponding entry with the token string.
…t response, a=testonly Automatic update from web-platform-tests Use OK status to check RaceNetworkRequest response Reflecting the comment on yoshisatoyanagisawa/ServiceWorker#10 (comment) Bug: 1371756 Change-Id: I6de1fed384e32fbdae40799ef48d4ce2eeb3eccd Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5276401 Reviewed-by: Minoru Chikamune <[email protected]> Reviewed-by: Yoshisato Yanagisawa <[email protected]> Reviewed-by: Kouhei Ueno <[email protected]> Commit-Queue: Shunya Shishido <[email protected]> Cr-Commit-Position: refs/heads/main@{#1257762} -- wpt-commits: 4c4bec5ff51e2230e4d3ee69773c608ae255f04d wpt-pr: 44460
docs/index.bs
Outdated
@@ -3195,6 +3238,11 @@ spec: storage; urlPrefix: https://storage.spec.whatwg.org/ | |||
1. Else: | |||
1. Set |workerRealm| to the [=relevant realm=] of the |activeWorker|'s [=service worker/global object=]. | |||
1. Set |eventHandled| to [=a new promise=] in |workerRealm|. | |||
1. If |raceResponse| is not null, then: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible raceResponse will be set to a promise in step 13.4.3.1.1 of Handle Fetch, but never resolved with a response by step 13.4.3.1.2.1? Or, it will be rejected because of the fetch abort that occurs in step 13.4.4?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now it might be "pending", I think? The map does not say it is allowed to contain "pending", but maybe it should be expanded to allow that?
docs/index.bs
Outdated
1. Let |map| be |activeWorker|'s [=service worker/global object=]'s [=race response map=]. | ||
1. Let |token| be the result of [=generating a random UUID=]. | ||
1. [=map/Set=] |map|[|token|] to |raceResponse|. | ||
1. Set |request|'s [=request/service-workers race token=] to |token|. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to just make the map keyed by requests, instead of strings? Then you don't need this token setup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to get the map entry by using a struct (or object) as a key? If so that would be feasible.
On the other hand, the implementation side may become a bit difficult due to the type inconsistency of request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. In specs maps can be keyed by anything. There's no need to change the implementation; as long as the spec is observably equivalent, it is OK for them to depart.
But, be sure to distinguish between requests and Request
s in the spec...
docs/index.bs
Outdated
1. Let |raceFetchController| be null. | ||
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 calling [=fetch=] given |request|, with [=fetch/processResponse=] set to the following steps given a [=/response=] |raceNetworkRequestResponse|: | ||
1. Set |raceResponse| to [=a new promise=]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure promises work well here. Promises can only store JavaScript or IDL objects, like Response
, but you are instead resolving them with a response. That isn't allowed.
You could try to wrap the response into a Response
. I guess that would mean creating a new Response
whose response is set to the response. But as explained in a comment later on, I don't think that is the right architecture. In general, I don't think using promises (which are main thread objects, very tied to JavaScript) inside this background-thread processing is a good idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried to update not to use promise here. Does that look better?
docs/index.bs
Outdated
@@ -3236,6 +3284,10 @@ spec: storage; urlPrefix: https://storage.spec.whatwg.org/ | |||
1. If |eventHandled| is not null, then [=reject=] |eventHandled| with a "{{NetworkError}}" {{DOMException}} in |workerRealm|. | |||
2. Return a [=network error=]. | |||
1. If |eventHandled| is not null, then [=resolve=] |eventHandled|. | |||
1. If |raceResponse| is not null, then: | |||
1. [=promise/React=] to |raceResponse|: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can't return from inside the react steps. Also, as discussed above, there is a type mismatch here: promises can only hold JavaScript objects, and you need to return a response, not a Response
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking like it will work :).
docs/index.bs
Outdated
@@ -3195,6 +3238,11 @@ spec: storage; urlPrefix: https://storage.spec.whatwg.org/ | |||
1. Else: | |||
1. Set |workerRealm| to the [=relevant realm=] of the |activeWorker|'s [=service worker/global object=]. | |||
1. Set |eventHandled| to [=a new promise=] in |workerRealm|. | |||
1. If |raceResponse| is not null, then: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now it might be "pending", I think? The map does not say it is allowed to contain "pending", but maybe it should be expanded to allow that?
docs/index.bs
Outdated
:: |workerRealm|, a [=relevant realm=] of the [=service worker/global object=] | ||
:: |reservedClient|, a [=request/reserved client=] | ||
:: |preloadResponse|, a [=promise=] | ||
:: |raceResponse|, a null, "<code>pending</code>" or [=/response=] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Parameter passing in specs generally follows JavaScript-style conventions. So, you cannot modify a parameter from outside the called algorithm, once it has been passed.
To get around this, create a tiny wrapper struct, with one item. Then the other algorithm can modify the struct item.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now it might be "pending", I think? The map does not say it is allowed to contain "pending", but maybe it should be expanded to allow that?
Yes, in L3141, the state is either "pending" or [=/response=]. I'll update the map to accept "pending".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To get around this, create a tiny wrapper struct, with one item. Then the other algorithm can modify the struct item.
Added a new struct and use it to the parameter, but not fully confident if this direction is correct. Let me know the usage is wrong.
docs/index.bs
Outdated
@@ -1079,7 +1079,7 @@ spec: storage; urlPrefix: https://storage.spec.whatwg.org/ | |||
}; | |||
</pre> | |||
|
|||
A {{ServiceWorkerGlobalScope}} object represents the global execution context of a [=/service worker=]. A {{ServiceWorkerGlobalScope}} object has an associated <dfn export for="ServiceWorkerGlobalScope">service worker</dfn> (a [=/service worker=]). A {{ServiceWorkerGlobalScope}} object has an associated <dfn for="ServiceWorkerGlobalScope">force bypass cache for import scripts flag</dfn>. It is initially unset. | |||
A {{ServiceWorkerGlobalScope}} object represents the global execution context of a [=/service worker=]. A {{ServiceWorkerGlobalScope}} object has an associated <dfn export for="ServiceWorkerGlobalScope">service worker</dfn> (a [=/service worker=]). A {{ServiceWorkerGlobalScope}} object has an associated <dfn for="ServiceWorkerGlobalScope">force bypass cache for import scripts flag</dfn>. A {{ServiceWorkerGlobalScope}} object has an associated <dfn for="ServiceWorkerGlobalScope">race response map</dfn> which is an [=ordered map=] where the [=map/keys=] are [=requests=] and the [=map/values=] are <a>promises</a>. It is initially unset. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the map values are now "response"s?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"response"s or "pending"? I updated it with the new struct though.
docs/index.bs
Outdated
: Input | ||
:: |request|, a [=request=] | ||
: Output | ||
:: a [=race response=]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this is called from the fetch spec, if possible, perhaps it's convenient to set just "<code>pending</code>", [=/response=], or null
as an output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think unwrapping the struct would be better, so that the Fetch spec doesn't need to know about it. (And then you can avoid export
ing it.)
Also, is Fetch really going to treat all three outcomes differently? Or maybe if it's still pending at the time of the call, it will treat pending the same as null? If so the you could handle that in this algorithm too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Updated to return [=/response=] or null. Fetch is going to treat if there is an map entry, which returns [=/response=], or no entry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I gave a few samples of how to do the struct, but I did not comment on every access.
docs/index.bs
Outdated
: Input | ||
:: |request|, a [=request=] | ||
: Output | ||
:: a [=race response=]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think unwrapping the struct would be better, so that the Fetch spec doesn't need to know about it. (And then you can avoid export
ing it.)
Also, is Fetch really going to treat all three outcomes differently? Or maybe if it's still pending at the time of the call, it will treat pending the same as null? If so the you could handle that in this algorithm too.
docs/index.bs
Outdated
1. Let |timingInfo| be a new [=service worker timing info=]. | ||
1. Let |raceResponse| be null. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here it would be: Set |raceResponse| to a [=race response=] whose [=race response/value=] is null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is Let
?
…t response, a=testonly Automatic update from web-platform-tests Use OK status to check RaceNetworkRequest response Reflecting the comment on yoshisatoyanagisawa/ServiceWorker#10 (comment) Bug: 1371756 Change-Id: I6de1fed384e32fbdae40799ef48d4ce2eeb3eccd Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5276401 Reviewed-by: Minoru Chikamune <[email protected]> Reviewed-by: Yoshisato Yanagisawa <[email protected]> Reviewed-by: Kouhei Ueno <[email protected]> Commit-Queue: Shunya Shishido <[email protected]> Cr-Commit-Position: refs/heads/main@{#1257762} -- wpt-commits: 4c4bec5ff51e2230e4d3ee69773c608ae255f04d wpt-pr: 44460
@@ -3231,11 +3280,15 @@ spec: storage; urlPrefix: https://storage.spec.whatwg.org/ | |||
|
|||
1. Wait for |task| to have executed or for |handleFetchFailed| to be true. | |||
1. If |shouldSoftUpdate| is true, then [=in parallel=] run the [=Soft Update=] algorithm with |registration|. | |||
1. If |raceResponseMap|[|request|] [=map/exists=], [=map/remove=] |raceResponseMap|[|request|]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This step is needed to remove the map entry, which is not consumed by Fetch. The fetch handler may not have fetch(e.request)
e.g. fallback, offline etc. Otherwise map entries are infinitely added.
docs/index.bs
Outdated
1. Let |queue| be an empty [=queue=] of [=/response=]. | ||
1. Let |raceFetchController| be null. | ||
1. Set |raceResponse|'s [=race response/value=] to "<code>pending</code>". | ||
1. [=map/Set=] |activeWorker|'s [=service worker/global object=]'s [=race response map=][|request|] to |raceResponse|. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be simpler if the race response map is associates with activeWorker (i.e. a service worker) rather than its global scope? Especially as at this point in the algorithm the worker hasn't been started yet, so global object might very well be null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point. How about keep the global scope having the map, and move this [=map/set=] to the step 16.2 in Create Fetch Event and Dispatch? At this point, Run Service Worker was already called to we can expect the global object is ready.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated as above. Let me know if this doesn't solve your concern.
…t response, a=testonly Automatic update from web-platform-tests Use OK status to check RaceNetworkRequest response Reflecting the comment on yoshisatoyanagisawa/ServiceWorker#10 (comment) Bug: 1371756 Change-Id: I6de1fed384e32fbdae40799ef48d4ce2eeb3eccd Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5276401 Reviewed-by: Minoru Chikamune <chikamunechromium.org> Reviewed-by: Yoshisato Yanagisawa <yyanagisawachromium.org> Reviewed-by: Kouhei Ueno <kouheichromium.org> Commit-Queue: Shunya Shishido <sisidovskichromium.org> Cr-Commit-Position: refs/heads/main{#1257762} -- wpt-commits: 4c4bec5ff51e2230e4d3ee69773c608ae255f04d wpt-pr: 44460 UltraBlame original commit: 2fa7948b44b6df8c83ff94f083948147447baecc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
c324260
into
yoshisatoyanagisawa:static_routing_api
Reflecting the comment on yoshisatoyanagisawa/ServiceWorker#10 (comment) Bug: 1371756 Change-Id: I6de1fed384e32fbdae40799ef48d4ce2eeb3eccd Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5276401 Reviewed-by: Minoru Chikamune <[email protected]> Reviewed-by: Yoshisato Yanagisawa <[email protected]> Reviewed-by: Kouhei Ueno <[email protected]> Commit-Queue: Shunya Shishido <[email protected]> Cr-Commit-Position: refs/heads/main@{#1257762}
…t response, a=testonly Automatic update from web-platform-tests Use OK status to check RaceNetworkRequest response Reflecting the comment on yoshisatoyanagisawa/ServiceWorker#10 (comment) Bug: 1371756 Change-Id: I6de1fed384e32fbdae40799ef48d4ce2eeb3eccd Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5276401 Reviewed-by: Minoru Chikamune <chikamunechromium.org> Reviewed-by: Yoshisato Yanagisawa <yyanagisawachromium.org> Reviewed-by: Kouhei Ueno <kouheichromium.org> Commit-Queue: Shunya Shishido <sisidovskichromium.org> Cr-Commit-Position: refs/heads/main{#1257762} -- wpt-commits: 4c4bec5ff51e2230e4d3ee69773c608ae255f04d wpt-pr: 44460 UltraBlame original commit: 2fa7948b44b6df8c83ff94f083948147447baecc
Reflecting the comment on yoshisatoyanagisawa/ServiceWorker#10 (comment) Bug: 1371756 Change-Id: I6de1fed384e32fbdae40799ef48d4ce2eeb3eccd Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5276401 Reviewed-by: Minoru Chikamune <[email protected]> Reviewed-by: Yoshisato Yanagisawa <[email protected]> Reviewed-by: Kouhei Ueno <[email protected]> Commit-Queue: Shunya Shishido <[email protected]> Cr-Commit-Position: refs/heads/main@{#1257762}
This PR will add the spec explanation for the
race-network-and-fetch-handler
source in the static routing API.This option starts the race between the network request and the fetch handler execution, and use the result whichever comes first. Currently the result from the network request is used only when the the response is successfully returned.
Borrowed sentences mostly from the navigation preload.
Preview | Diff