ServiceWorker
Add race-network-and-cache source to static routing api.
#1764
Open

Add race-network-and-cache source to static routing api. #1764

monica-ch wants to merge 6 commits into w3c:main from monica-ch:SW-Race-NetworkRace
monica-ch
monica-ch94 days ago (edited 88 days ago)

This PR adds new source race-network-and-cache to the SW static routing api per https://github.com/WICG/service-worker-static-routing-api/blob/main/final-form.md.


Preview | Diff

monica-ch add race-network-and-cache
778bf3bc
monica-ch minor format fixes
50af98bc
monica-ch monica-ch requested a review from yoshisatoyanagisawa yoshisatoyanagisawa 94 days ago
monica-ch
monica-ch94 days ago

@yoshisatoyanagisawa PTAL when you get a chance, thanks!

yoshisatoyanagisawa
yoshisatoyanagisawa commented on 2025-04-16
Conversation is marked as resolved
Show resolved
docs/index.bs
3290 1. [=queue/Enqueue=] |response| to |queue|.
3291 1. If |raceFetchController| is not null, [=fetch controller/abort=] |raceFetchController|.
3292 1. Wait until |queue| is not empty.
3293
1. If |queue| is empty, return null.
yoshisatoyanagisawa93 days ago

What does this step mean?
L3292 says "Wait until |queue| is not empty.", then L3293 should not happen?

yoshisatoyanagisawa93 days ago (edited 93 days ago)

I guess L3292 might wait for:

  1. |queue| is fulfilled
  2. or, both of network fetch and cache lookup failure.

But stepping back, race-network-and-fetch-event may also need to be revised for both failure case.

monica-ch93 days ago

Added flags to track if they are finished and Wait until |queue| is not empty step is updated so it won't wait indefinitely if both fails. The next step handles if they fail and |queue| is empty our algorithm returns.

Is this change, okay? Feel free to suggest for corrections or if it is unclear

yoshisatoyanagisawa93 days ago (edited 93 days ago)

Thanks for the update.
The algorithm looks good except for the abort network scenario.
I feel the explanation looks good as C++ code, but I am not sure if it is usual way to write down in the specification. I want a spec expert to check the way to write the procedure down to specification.

yoshisatoyanagisawa
yoshisatoyanagisawa93 days ago

@sisidovski Will you take a look?
I assume it mostly fine but I feel all failure scenarios may not work well for both race-network-and-fetch-event and race-network-and-cache.

monica-ch update failure cases
5b14fcd7
yoshisatoyanagisawa
yoshisatoyanagisawa commented on 2025-04-17
docs/index.bs
32573262 1. If |fetchHandlerResponse| is not null and not a [=network error=], and |raceFetchController| is not null, [=fetch controller/abort=] |raceFetchController|.
32583263 1. [=queue/Enqueue=] |fetchHandlerResponse| to |queue|.
3259 1. Wait until |queue| is not empty.
3264 1. Set |fetchHandlerCompleted| to true.
3265
1. Wait until |queue| is not empty or (|networkFetchCompleted| is true and |fetchHandlerCompleted| is true).
yoshisatoyanagisawa93 days ago

@domenic Can I ask your advice on how this kinds of thing is usually written in the spec?
The explanation sounds reasonable as code, but I am afraid it is not usual way of writing spec.

domenic93 days ago

Are you concerned mainly about the parentheses? I think they are OK since they are easy to understand, but yes, it is unusual. The pattern I would use would be:

Wait until any of the following are true:

  • |queue| is not empty; or
  • |networkFetchCompleted| is true and |fetchHandlerCompleted| is true.
yoshisatoyanagisawa92 days ago

Yes, I wanted to know how people usually write this because I have not seen specs using parentheses.
And, thank you for the advice.

@monica-ch Will you revise the sentences as @domenic's way?

sisidovski92 days ago

I'm not sure if we really really need to introduce networkFetchCompleted and fetchHandlerCompleted. For race-network-and-fetch-handler, the race is expressed by queue. Why we need these two flags?

It looks networkFetchCompleted doesn't wait for the promise resolve. So assuming queue can be still empty even when networkFetchCompleted is true. This will return null, which means the fetch event fallback in the handle fetch algorithm.

Also not sure why we should wait for fetchHandlerCompleted as well. This means we have to wait for both the network and fetch handler completion.

IIUC race-network-and-fetch-handler won't run the race anymore after this change.

yoshisatoyanagisawa92 days ago

This change should be due to #1764 (comment).
I concerned that if both network and cache failed, nothing is enqueued, and waiting for queue won't finish. I stepped back and saw the race-network-and-fetch-handler, and felt it may also have the same issue.
Having the way to know network and cache lookup completion should be needed to avoid the infinite queue waiting.

yoshisatoyanagisawa
yoshisatoyanagisawa commented on 2025-04-17
Conversation is marked as resolved
Show resolved
docs/index.bs
3255 1. Set |networkFetchCompleted| to true.
32513256 1. [=If aborted=] and |raceFetchController| is not null, then:
32523257 1. [=fetch controller/Abort=] |raceFetchController|.
32533258
1. Set |raceResponse| to a [=race response=] whose [=race response/value=] is null.
yoshisatoyanagisawa93 days ago

I guess you might also need to set |networkFetchCompleted| to true. That is because network fetch will not move forward on abort.

yoshisatoyanagisawa
yoshisatoyanagisawa commented on 2025-04-17
Conversation is marked as resolved
Show resolved
docs/index.bs
3281 1. Set |networkFetchCompleted| to true.
3282 1. [=If aborted=] and |raceFetchController| is not null, then:
3283 1. If |raceFetchController|'s [=fetch controller/state=] is not "<code>terminated</code>", [=fetch controller/abort=] |raceFetchController|.
3284
1. Set |raceResponse| to a [=race response=] whose [=race response/value=] is null.
yoshisatoyanagisawa93 days ago

Also |networkFetchComplete| for this.

yoshisatoyanagisawa
yoshisatoyanagisawa commented on 2025-04-17
Conversation is marked as resolved
Show resolved
docs/index.bs
3284 1. Set |raceResponse| to a [=race response=] whose [=race response/value=] is null.
3285 1. Resolve |preloadResponse| with undefined.
3286 1. Run the following substeps [=in parallel=]:
3287
1. [=map/For each=] |cacheName| → |cache| of the |registration|'s [=service worker registration/storage key=]'s [=name to cache map=]:
yoshisatoyanagisawa93 days ago (edited 93 days ago)

I feel this as a duplication of the cache source procedure above. Can we factor this procedure out to the independent algorithm?

monica-ch89 days ago

Updated, please take a look and correct me if anything looks wrong.

sisidovski
sisidovski commented on 2025-04-18
Conversation is marked as resolved
Show resolved
docs/index.bs
15911591
15921592 dictionary RouterSourceDict {
15931593 DOMString cacheName;
1594
DOMString raceNetworkAndCacheCacheName;
sisidovski92 days ago

I'm not sure if we need a dedicated field to specify a cache name for race-network-and-cache. Also, could you give some example of how the final API shape will look like?

monica-ch92 days ago

using a cacheName in the source indicate the source as "cache". It is hard to identify the source if cacheName is used for both sources.

e.addRoutes([
{
condition: {
urlPattern: {pathname: "/race_network_and_cache_test_1"}
},
source: "race-network-and-cache" // Race b/w network and cache and without cache name indicates search all caches.
},
{
condition: {
urlPattern: "/race_network_and_cache_test_2",
},
source: {raceNetworkAndCacheCacheName: "test"} // Having raceNetworkAndCacheCacheName tells us to use race b/w network and cache source with the provided cacheName
condition: {
urlPattern: "/cache",
},
source: {cacheName: "test"} // existing example for cache source
},
]);

This is in-line with the final form for Race-Network-and-cache - https://github.com/WICG/service-worker-static-routing-api/blob/main/final-form.md#race-network-and-cache-storage

@yoshisatoyanagisawa Correct me if this isn't expected

yoshisatoyanagisawa92 days ago

Yes, that is my understanding.

sisidovski89 days ago

Thanks. RouterSourceDict can be a RouterSource so we need a dedicated field for the cache name of race-network-and-cache. That is reasonable.

monica-ch Follow spec format
b69c7da6
monica-ch use helper algorithm
8b8c331c
yoshisatoyanagisawa
yoshisatoyanagisawa commented on 2025-04-21
docs/index.bs
3252 1. Wait until any of the following are true:
3253 1. |queue| is not empty.
3254 1. Both |networkFetchCompleted| and |fetchHandlerCompleted| are true.
3255
1. If |queue| is empty, return null.
yoshisatoyanagisawa89 days ago

Is there a way to reuse raceResponse if possible?
The reason of making [=Create Fetch Event and Dispatch=] to take [=raceResponse=] is that we wanted to avoid sending a duplicated request to the server. If we follow that thought, it might be reasonable to reuse network error on both complete but queue empty case.

monica-ch88 days ago

Updated, does it looks better?

monica-ch use |raceResponse| when empty
3305a7f0

Login to write a write a comment.

Login via GitHub

Assignees
No one assigned
Labels
Milestone