Skip to content
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

Jump to bottom

(service-worker) Regression with failed cache-busted request. #39775

Closed
gkalpak opened this issue Nov 20, 2020 · 1 comment
Closed

(service-worker) Regression with failed cache-busted request. #39775

gkalpak opened this issue Nov 20, 2020 · 1 comment
Assignees
@gkalpak
Labels
area: service-worker Issues related to the @angular/service-worker package P2 The issue is important to a large percentage of users, with a workaround regression Indicates than the issue relates to something that worked in a previous version type: bug/fix
Milestone
Backlog

Comments

@gkalpak
Copy link
Member

gkalpak commented Nov 20, 2020 •
edited

Some recent changes to AssetGroup's cacheBustedFetchFromNetwork() (in 5be4edf and 036a2fa) have caused the following regression:

Whenever the client is offline, makeCacheBustedFetch will be true, which will result in making a cache-busted request, which in turn will fail as well (since the client is offline), causing an SwCriticalError to be thrown. Finally, throwing an SwCriticalError will cause the ServiceWorker to unnecessarily enter an EXISTING_CLIENTS_ONLY degraded mode.

Here is a description of the logic in cacheBustedFetchFromNetwork().

Based on that, there are a 5 basic scenarios that are of interest. For brevity, I will use the following symbols:

  • "✉️1": The non-cache-busted request.
  • "✉️2": The cache-busted request.
  • "✉️_✔️": A successful request (i.e. one for which response.ok === true).
  • "✉️_❌": A failed request (i.e. one for which response.ok === false).
  • "#️⃣ 1": The hash of the response of "✉️1".
  • "#️⃣ 2": The hash of the response of "✉️2".
  • "#️⃣ _✔️": A response hash that matches the canonical hash.
  • "#️⃣ _❌": A response hash that does not match the canonical hash.

Scenarios:

  1. "✉️1❌" (=> "✉️2❌")
  2. "✉️1✔️" => "#️⃣ 1✔️"
  3. "✉️1✔️" => "#️⃣ 1❌" => "✉️2✔️" => "#️⃣ 2✔️"
  4. "✉️1✔️" => "#️⃣ 1❌" => "✉️2✔️" => "#️⃣ 2❌"
  5. "✉️1✔️" => "#️⃣ 1❌" => "✉️2❌"

To give some historical context, here is what was returned on each scenario in the past:

  • Initialy (commit d442b68):
    1. Scenario 1: The (failed) response of "✉️1".
    2. Scenario 2: The (successful) response of "✉️1".
    3. Scenario 3: The (successful) response of "✉️2".
    4. Scenario 4: Throws an Error.
    5. Scenario 5: Throws an Error.

  • Later, when SwCriticalError was introduced (commit 3951098):
    1. Scenario 1: The (failed) response of "✉️1".
    2. Scenario 2: The (successful) response of "✉️1".
    3. Scenario 3: The (successful) response of "✉️2".
    4. Scenario 4: Throws an SwCriticalError.
    5. Scenario 5: Throws an SwCriticalError.

  • After a recent attempt to fix the logic (commit 5be4edf):
    1. Scenario 1: Throws an SwCriticalError.
    2. Scenario 2: The (successful) response of "✉️1".
    3. Scenario 3: The (successful) response of "✉️2".
    4. Scenario 4: Throws an SwCriticalError.
    5. Scenario 5: Throws an SwCriticalError.

This last change made the implementation match the logic described in this comment. Upon further inspection (and based on other nearby comments), it doesn't seem desirable to make a cache-busted request when the non-cache-busted one failed.

Therefore, I believe the desired behavior would be to revert to the previous logic (where a cache-busted request is made only if the non-cache-busted one is successful) and add extra logic to port the changes from 036a2fa (which introduced the SwUnrecoverableStateError) to non-cache-busted requests.

For completeness, below are the current behavior (with commit 036a2fa) and the desired/proposed one. Note that there is additional distinction of failed requests based on the returned HTTP status code (denoted in parenthesis).

  • Current behavior (with commit 036a2fa):
    1. Scenario 1a (status: 404): Throws an SwUnrecoverableStateError.
    2. Scenario 1b (status: *): Throws an SwCriticalError.
    3. Scenario 2: The (successful) response of "✉️1".
    4. Scenario 3: The (successful) response of "✉️2".
    5. Scenario 4: Throws an SwCriticalError.
    6. Scenario 5a (status: 404): Throws an SwUnrecoverableStateError.
    7. Scenario 5b (status: *): Throws an SwCriticalError.

  • Desired/proposed behavior (to be implemented):
    1. Scenario 1a (status: 404): Throws an SwUnrecoverableStateError.
    2. Scenario 1b (status: *): The (failed) response of "✉️1".
    3. Scenario 2: The (successful) response of "✉️1".
    4. Scenario 3: The (successful) response of "✉️2".
    5. Scenario 4: Throws an SwCriticalError.
    6. Scenario 5a (status: 404): Throws an SwUnrecoverableStateError.
    7. Scenario 5b (status: *): The (failed) response of "✉️2".
@gkalpak gkalpak added type: bug/fix regression Indicates than the issue relates to something that worked in a previous version area: service-worker Issues related to the @angular/service-worker package P2 The issue is important to a large percentage of users, with a workaround labels Nov 20, 2020
@gkalpak gkalpak self-assigned this Nov 20, 2020
@ngbot ngbot bot added this to the Backlog milestone Nov 20, 2020
gkalpak added a commit to gkalpak/angular that referenced this issue Nov 20, 2020
@gkalpak
fix(service-worker): correctly handle failed cache-busted request
557021c 
Since 5be4edf, a failing cache-busted
request (such as those triggered for fetching an uncached asset) will
cause the ServiceWorker to incorrectly enter a degraded
`EXISTING_CLIENTS_ONLY` mode. A failing network request could be caused
by many reasons, including the client or server being offline, and does
not signify a broken ServiceWorker state.

This commit fixes the logic in `cacheBustedFetchFromNetwork()` to
correctly handle errors in network requests.

For more details on the problem and the implemented fix see angular#39775.
gkalpak added a commit to gkalpak/angular that referenced this issue Nov 20, 2020
@gkalpak
fix(service-worker): correctly handle failed cache-busted request
9b0002e 
Since 5be4edf, a failing cache-busted
request (such as those triggered for fetching an uncached asset) will
cause the ServiceWorker to incorrectly enter a degraded
`EXISTING_CLIENTS_ONLY` mode. A failing network request could be caused
by many reasons, including the client or server being offline, and does
not signify a broken ServiceWorker state.

This commit fixes the logic in `cacheBustedFetchFromNetwork()` to
correctly handle errors in network requests.

For more details on the problem and the implemented fix see angular#39775.
gkalpak added a commit to gkalpak/angular that referenced this issue Nov 20, 2020
@gkalpak
fix(service-worker): correctly handle failed cache-busted request
d834d24 
Since 5be4edf, a failing cache-busted
network request (such as requests for fetching uncached assets) will
cause the ServiceWorker to incorrectly enter a degraded
`EXISTING_CLIENTS_ONLY` mode. A failing network request could be caused
by many reasons, including the client or server being offline, and does
not necessarily signify a broken ServiceWorker state.

This commit fixes the logic in `cacheBustedFetchFromNetwork()` to
correctly handle errors in network requests.
For more details on the problem and the implemented fix see angular#39775.

Fixes angular#39775
@gkalpak gkalpak mentioned this issue Nov 20, 2020
Closed
gkalpak added a commit to gkalpak/angular that referenced this issue Nov 23, 2020
@gkalpak
fix(service-worker): correctly handle failed cache-busted request
9c042da 
Since 5be4edf, a failing cache-busted
network request (such as requests for fetching uncached assets) will
cause the ServiceWorker to incorrectly enter a degraded
`EXISTING_CLIENTS_ONLY` mode. A failing network request could be caused
by many reasons, including the client or server being offline, and does
not necessarily signify a broken ServiceWorker state.

This commit fixes the logic in `cacheBustedFetchFromNetwork()` to
correctly handle errors in network requests.
For more details on the problem and the implemented fix see angular#39775.

Fixes angular#39775
gkalpak added a commit to gkalpak/angular that referenced this issue Nov 23, 2020
@gkalpak
fix(service-worker): correctly handle failed cache-busted request
5e80c38 
Since 5be4edf, a failing cache-busted
network request (such as requests for fetching uncached assets) will
cause the ServiceWorker to incorrectly enter a degraded
`EXISTING_CLIENTS_ONLY` mode. A failing network request could be caused
by many reasons, including the client or server being offline, and does
not necessarily signify a broken ServiceWorker state.

This commit fixes the logic in `cacheBustedFetchFromNetwork()` to
correctly handle errors in network requests.
For more details on the problem and the implemented fix see angular#39775.

Fixes angular#39775
AndrewKushnir pushed a commit that referenced this issue Nov 23, 2020
@gkalpak @AndrewKushnir
fix(service-worker): correctly handle failed cache-busted request (#3…
7bf73d7 
…9786)

Since 5be4edf, a failing cache-busted
network request (such as requests for fetching uncached assets) will
cause the ServiceWorker to incorrectly enter a degraded
`EXISTING_CLIENTS_ONLY` mode. A failing network request could be caused
by many reasons, including the client or server being offline, and does
not necessarily signify a broken ServiceWorker state.

This commit fixes the logic in `cacheBustedFetchFromNetwork()` to
correctly handle errors in network requests.
For more details on the problem and the implemented fix see #39775.

Fixes #39775

PR Close #39786
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Dec 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@gkalpak gkalpak

Labels
area: service-worker Issues related to the @angular/service-worker package P2 The issue is important to a large percentage of users, with a workaround regression Indicates than the issue relates to something that worked in a previous version type: bug/fix
Projects
None yet
Milestone
Backlog
Development

Successfully merging a pull request may close this issue.

fix(service-worker): correctly handle failed cache-busted request gkalpak/angular
1 participant
@gkalpak