Fix: race-network-and-fetch-handler now uses actual |raceResponse| #1777
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@sisidovski @domenic Will you take a look? |
domenic
reviewed
Jun 11, 2025
sisidovski
reviewed
Jun 11, 2025
Apply the suggestion. Co-authored-by: Domenic Denicola <[email protected]>
Upon the discussion, I understand that the fetch event part will always be queued. However, we need to wait for the network response so that we can reuse it as response on error. We also need to set the proper |timingInfo| for that case.
yoshisatoyanagisawa
changed the title
domenic
reviewed
Jun 12, 2025
| @@ -3263,16 +3263,21 @@ spec: storage; urlPrefix: https://storage.spec.whatwg.org/ | |||
| 1. Let |queue| be an empty [=queue=] of [=race result=]. | |||
| 1. Let |raceFetchController| be null. | |||
| 1. Let |raceResponse| be a [=race response=] whose [=race response/value=] is "<code>pending</code>". | |||
| 1. Let |networkFetchCompleted| be false. | |||
| 1. Let |networkFetchResult| be null. | |||
| 1. Run the following substeps [=in parallel=], but [=abort when=] |fetchController|'s [=fetch controller/state=] is "<code>terminated</code>" or "<code>aborted</code>": | |||
There was a problem hiding this comment.
The existing setup seems a bit fragile to me. The definition of abort when only operates between steps, and there are only two steps nested under "If aborted"... In fact there is only one step before this PR.
Probably there should be a check on fetchController's state inside the processResponse callback?
There was a problem hiding this comment.
I am not confident but I tried to address that with #1779.
Is it what you meant?
sisidovski
reviewed
Jun 12, 2025
sisidovski
reviewed
Jun 12, 2025
As pointed out by the reviewer, the same algorithm can be realized without |networkFetchCompleted| and |networkFetchResult|. Let me remove them.
Since `Create Fetch Event And Dispatch` plumb |raceResponse|'s `value` when it is given, we actually do not need to wait the value. However, I found that `Create Fetch Event And Dispatch` returns [=network error=] without the |timingInfo|, it should be propagated properly.
yoshisatoyanagisawa
changed the title
domenic
approved these changes
Jun 13, 2025
|
The fetch should plumb the output to w3c/resource-timing#415. |
github-actions bot
added a commit
to yoshisatoyanagisawa/ServiceWorker
that referenced
this pull request
Jun 13, 2025
github-actions bot
added a commit
to asleekgeek/ServiceWorker
that referenced
this pull request
Jun 13, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request originates from the work proposed by @monica-ch in #1764, but has been extensively reworked based on review feedback.
This PR corrects response handling for race-network-and-fetch-handler.
Primary Fix: The handler now correctly uses the actual fetch response for |raceResponse| instead of a generic [=network error=].
Minor Improvement: |timingInfo| is now included in the [=network error=], when returned by [=Create Fetch Event and Dispatch=], which simplifies the calling code.
Preview | Diff