Add race-network-and-cache source to static routing api. #1764
Conversation
|
@yoshisatoyanagisawa PTAL when you get a chance, thanks! |
|
@sisidovski Will you take a look? |
docs/index.bs
Outdated
| @@ -3256,7 +3261,45 @@ spec: storage; urlPrefix: https://storage.spec.whatwg.org/ | |||
| 1. Let |fetchHandlerResponse| be the result of [=Create Fetch Event and Dispatch=] with |request|, |registration|, |useHighResPerformanceTimers|, |timingInfo|, |workerRealm|, |reservedClient|, |preloadResponse|, and |raceResponse|. | |||
| 1. If |fetchHandlerResponse| is not null and not a [=network error=], and |raceFetchController| is not null, [=fetch controller/abort=] |raceFetchController|. | |||
| 1. [=queue/Enqueue=] |fetchHandlerResponse| to |queue|. | |||
| 1. Wait until |queue| is not empty. | |||
| 1. Set |fetchHandlerCompleted| to true. | |||
| 1. Wait until |queue| is not empty or (|networkFetchCompleted| is true and |fetchHandlerCompleted| is true). | |||
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
docs/index.bs
Outdated
| @@ -3256,7 +3261,45 @@ spec: storage; urlPrefix: https://storage.spec.whatwg.org/ | |||
| 1. Let |fetchHandlerResponse| be the result of [=Create Fetch Event and Dispatch=] with |request|, |registration|, |useHighResPerformanceTimers|, |timingInfo|, |workerRealm|, |reservedClient|, |preloadResponse|, and |raceResponse|. | |||
| 1. If |fetchHandlerResponse| is not null and not a [=network error=], and |raceFetchController| is not null, [=fetch controller/abort=] |raceFetchController|. | |||
| 1. [=queue/Enqueue=] |fetchHandlerResponse| to |queue|. | |||
| 1. Wait until |queue| is not empty. | |||
| 1. Set |fetchHandlerCompleted| to true. | |||
| 1. Wait until |queue| is not empty or (|networkFetchCompleted| is true and |fetchHandlerCompleted| is true). | |||
There was a problem hiding this comment.
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.
docs/index.bs
Outdated
| 1. Wait until any of the following are true: | ||
| 1. |queue| is not empty. | ||
| 1. Both |networkFetchCompleted| and |fetchHandlerCompleted| are true. | ||
| 1. If |queue| is empty, return null. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Updated, does it looks better?
This PR adds new source
race-network-and-cacheto the SW static routing api per https://github.com/WICG/service-worker-static-routing-api/blob/main/final-form.md.Preview | Diff