Service worker locked in EXISTING_CLIENTS_ONLY state

[H--o-l]

🐞 bug report

Affected Package

The issue is caused by package @angular/pwa

Is this a regression?

Not a regression I think.

Description

If a network failure happens, the service worker will enter the
EXISTING_CLIENTS_ONLY state.

If a connected client reloads (F5), the service worker will stay in that state
forever and serve nothing to the client, nor to other clients or reloading clients.

In my opinion, it would be better if the service worker kept serving the old
cached version, while waiting for the network to recover and the update to occur successfully.
Otherwise, offline mode is broken.

🔬 Minimal Reproduction

To enter EXISTING_CLIENTS_ONLY state:

1. Create an Angular app:

    mkdir drop_me
    cd drop_me
    npm install @angular/cli
    node_modules/.bin/ng new my-dream-app
    cd my-dream-app
    node_modules/.bin/ng add @angular/pwa --project my-dream-app
    node_modules/.bin/ng build --prod
    npm install http-server
    node_modules/.bin/http-server -p 8080 -c-1 dist/my-dream-app
   

2. Open http://localhost:8080 in Chrome and wait for the page

3. Kill the server

4. Change something in src/app/app.component.html

5. Rebuild and serve:

    node_modules/.bin/ng build --prod
    node_modules/.bin/http-server -p 8080 -c-1 dist/my-dream-app
   

6. Go back in Chrome, it slow 3g network in dev tool, hit F5, and wait for
   the service worker to start downloading index.html, main.js or any file of
   the update.

   When that happen, check chrome devtool Offline checkbox.

   The service worker is now in EXISTING_CLIENTS_ONLY.

From now, to create the issue, just hit F5.

The application will be broken offline, even after network reconnection,
tab close/re-open, etc.

For me, beside unregistering the service worker manually, only a new update of
the application can fix the state.

🌍 Your Environment

Angular Version:

Angular CLI: 8.0.3
Node: 11.5.0
OS: linux x64
Angular: 8.0.1
... animations, common, compiler, compiler-cli, core, forms
... language-service, platform-browser, platform-browser-dynamic
... router, service-worker

Package                           Version
-----------------------------------------------------------
@angular-devkit/architect         0.800.3
@angular-devkit/build-angular     0.800.3
@angular-devkit/build-optimizer   0.800.3
@angular-devkit/build-webpack     0.800.3
@angular-devkit/core              8.0.3
@angular-devkit/schematics        8.0.3
@angular/cli                      8.0.3
@angular/pwa                      0.800.3
@ngtools/webpack                  8.0.3
@schematics/angular               8.0.3
@schematics/update                0.800.3
rxjs                              6.4.0
typescript                        3.4.5
webpack                           4.30.0

Anything else relevant?

Only tested on Chrome.

[H--o-l]

Finally I think even an update of the application, and thus a new ngsw.json can not fix the state of the service worker.
For me the servie worker try to make the n-1 broken version list of files match with the new ngsw.json which of course can not happen.
Only an update the ngsw-worker.js file itself re-compute the service worker driver state.

Do you think a solution can be found, at least so the service worker can go out of the broken state on an ngsw.json update?

[H--o-l]

I think a code diff that could work to restore the service worker nominal state after a degraded state could be:

--- node_modules/@angular/service-worker/ngsw-worker.js
+++ node_modules/@angular/service-worker/ngsw-worker.js
@@ -2472,6 +2472,14 @@
                 // Future new clients will use this hash as the latest version.
                 this.latestHash = hash;
                 yield this.sync();
+                // A new version have been correctly fecthed and registered.
+                // It is ready to be assigned to previously disconnect clients
+                // and to new clients, thus let's go out of
+                // `EXISTING_CLIENTS_ONLY` mode now.
+                if (this.state == DriverReadyState.EXISTING_CLIENTS_ONLY) {
+                    this.state = DriverReadyState.NORMAL;
+                    this.stateMessage = '(nominal)';
+                }
                 yield this.notifyClientsAboutUpdate();
             });
         }

@gkalpak Sorry to ping you, but maybe you have an opinion if it's a good proposal or not?

[gkalpak]

Reading through your comments, everything sounds reasonable, @H--o-l.
But I have to take a closer look at the code and the current behavior (it does definitely sound like a bug). I'll try to get to it in the next few days.

[gkalpak]

@H--o-l, I finally got around to looking into this. I think the current behavior is (sort of) intended.

What you said above is correct, except that this only last for as long as the ServiceWorker instance is in EXISTING_CLIENT_ONLY. Note, that, once the ServiceWorker is not used for a while, the browser will stop it (to save resources). It will create a new ServiceWorker instance the next time it needs to handle an event (e.g. a fetch event).

The Driver state (e.g. EXISTING_CLIENTS_ONLY) is not persisted, so each SW instance starts in the nominal state (it may enter a different state again if certain conditions are met, but it starts in normal mode). So, the behavior you observed is temporary (until the browser stops the current SW instance).

During debugging, you can verify that, by forcing the SW instance to be stopped:

• Go to chrome://serviceworker-internals.
• Search for the domain name your are debugging.
• Click on the Stop button.

So, once a ServiceWorker instance enters a degraded state (either EXISTING_CLIENTS_ONLY or SAFE_MODE), it will not leave that state. Once the instance is stopped after a while, the next instance that will be created will try to recover. This is by design: Because we can't be sure why the ServiceWorker entered the degraded state, it is risky to try and recover on the same instance (unless we kept more info on why a state was entered and what would be required to recover).

[H--o-l]

@H--o-l, I finally got around to looking into this. I think the current behavior is (sort of) intended.

Thanks for looking into this!

Sorry for the long answer too, I wasn't here.

It's an interesting fact that the Driver state is not persisted between service worker instances.
I did know that service workers would be stop by browsers, but not how to simulate it, thanks!
(Your instructions were almost perfect: I think it's the chrome://inspect/#service-workers page and the terminate button.)

That been said, I have some remarks around the design:

1. Service worker API is awesome to finely control caching behaviour, in the same way on each browsers.
   Building design around the fact that the ServiceWorker may be stopped when the browser decide, is, I think, a regression to olders API, where it was the browser do it's thing, deal with it.
   The behavior cannot be predicted on all browsers and at all time, and it will surely differ.

2. I think comment here suggests that the service worker would recover from the EXISTING_CLIENT_ONLY state with a new nsgw.json file (i.e. an update of the application).
   Sadly it's not enough as the service worker as to be stop by the browser as you say.

3. The documentation explains what are the degraded state, but not how to recover.
   I'd be happy to open a PR to add your explanation in it, but I rather the design being change.

4. I think many users are confuse about it and post unnecessary comments or issues:

   • they create a hash mismatch in their ngsw.json for a reason or another
   • they have a 504 error
   • they fix the hash mismatch on the fly on their server
   • the Driver state is still broken, they do not realise it was the correct solution because nothing change
   • they open an issue about having a 504 error.

   I think it would spare you a lot of time (and users too) if it was easier to recover from the degrade states.

What do you think about it @gkalpak?
I'm happy to continue the conversion or to propose a small documentation improvement if you think it's the best.
Thanks again for your time!

[gkalpak]

Ooops, I meant chrome://serviceworker-internals (instead of chrome://service-worker-internals). But chrome://inspect/#service-workers works too 👍

Regarding your points:

1. You are right that this leaves some things to the browser, but it was a design decision trying to balance different things. For example, balancing the benefits of caching with users' expectation that opening a new tab gives them a recent (if not the latest) version of the content. Also, keep in mind that the SW APIs should be seen as possible progressive enhancements only - not as integral parts of the app. So, we have chosen to err on the side of caution in some cases.
   That said, I will take a look to see if it might be safe to recover from the EXISTING_CLIENTS_ONLY state. (Recoverign from a SAFE_MODE state would be too risky imo, since we can't know exactly what brought us there.

2. That's just me not understanding all aspects of SW behavior at the time 😁
   (Not that I do now, but I hopefully understand some more than before 😛)

3. Yes, please ❤️ Adding some info in the docs would be great.

4. I agree that we should try to make it easier for people to recover or debug such states:

   • Adding more info to the docs is a good first step.
   • If you also have ideas about providing more helpful/actionable info in /ngsw/state, I'm open to suggestions.
   • There are some suggestions for possible improvements in [Service Worker] Can't update sw if in SAFE_MODE #22087 (comment) (e.g. allowing the driver state to be retrieved by the app). If you fancy working on that, I would be happy to help/discuss.
   • Finally, as mentioned above, I will look into whether we could at least safely recover from EXISTING_CLIENTS_ONLY.

Thx, again, for raising these points 👍

[gkalpak]

OK, I thought about it some more and I believe it is reasonable to try to recover from EXISTING_CLIENTS_ONLY, so I created #31865 with a couple of improvements related to entering/recovering from the EXISTING_CLIENTS_ONLY mode 🎉

[H--o-l]

Awesome!
I'm glad if you think it's reasonable and that you were able to make a PR about it.
Could I suggest a cherry-pick of that commit b643d51 to include doc improvement inside your PR?

Of course feel free to adapt it, or made your how entirely!

[gkalpak]

Sorry, @H--o-l, I forgot about incorporating b643d51.
Now that #31865 is merged (:tada:) would you be up to submitting a separate PR with the docs improvements?

[H--o-l]

@gkalpak

Now that #31865 is merged (:tada:)

🎉 🎉

No issue, I open the PR #32682 !

[angular-automatic-lock-bot]

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.}