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.

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

feat(service-worker): fixes/improvements related to entering/recovering from `EXISTING_CLIENTS_ONLY` mode #31865

Closed
wants to merge 4 commits into from

Conversation

@gkalpak
Copy link
Member

commented Jul 26, 2019

Some fixes/improvements related to entering and recovering from the EXISTING_CLIENTS_ONLY mode. See individual commints for more details:

  • refactor(service-worker): remove redundant argument to versionFailed()
  • test(service-worker): add helper function for making navigation requests
  • fix(service-worker): keep serving clients on older versions if latest is invalidated
  • feat(service-worker): recover from EXISTING_CLIENTS_ONLY mode when there is a valid update

Fixes #31109.

gkalpak added 3 commits Jul 26, 2019
The `latest` argument was only ever set to the value of comparing
`this.latestHash` with the `appVersion` hash, which is already computed
inside `versionFailed()`, so there is no reason to pass it as an
argument as well.

This doesn't have any impact on the current behavior of the SW.
Helper functions for making navigation requests were created in several
places inside the test suite, so this commit creates a top-level such
helper and uses that in all tests that need it.
… is invalidated

Previously, when the latest version was invalidated (e.g. due to a hash
mismatch), the SW entered a degraded `EXISTING_CLIENTS_ONLY` mode and
removed _all_ clients from its client-version map (essentially stopping
to serve any clients). Based on the code and surrounding comments, the
intention seems to have been to only remove clients that were on the
invalidated version, but keep other clients on older versions.

This commit fixes it by only unassigning clients what were on the latest
version and keep clients assigned to older versions.
@gkalpak gkalpak requested a review from angular/fw-service-worker as a code owner Jul 26, 2019
@googlebot googlebot added the cla: yes label Jul 26, 2019
@gkalpak gkalpak changed the title feat(service-worker): fixes/improvements related to entering and recovering from the `EXISTING_CLIENTS_ONLY` mode feat(service-worker): fixes/improvements related to entering/recovering from `EXISTING_CLIENTS_ONLY` mode Jul 26, 2019
@ngbot ngbot bot added this to the needsTriage milestone Jul 26, 2019
@mary-poppins

This comment has been minimized.

Copy link

commented Jul 26, 2019

…there is a valid update

Previously, when the ServiceWorker entered a degraded mode
(`EXISTING_CLIENTS_ONLY` or `SAFE_MODE`) it would remain in that mode
for the rest of the lifetime of ServiceWorker instance. Note that
ServiceWorkers are stopped by the browser after a certain period of
inactivity and a new instance is created as soon as the ServiceWorker
needs to handle an event (such as a request from the page). Those new
instances would start from the `NORMAL` mode.

The reason for this behavior is to err on the side of caution: If we
can't be sure why the ServiceWorker entered the degraded mode, it is
risky to try recovering on the same instance and might lead to
unexpected behavior.

However, it turns out that the `EXISTING_CLIENTS_ONLY` mode can only be
a result of some error happening with the latest version (e.g. a hash
mismatch in the manifest). Therefore, it is safe to recover from that
mode once a new, valid update is successfully installed and to start
accepting new clients.

This commit ensures that the mode is set back to `NORMAL`, when (a) an
update is successfully installed and (b) the current mode is
`EXISTING_CLIENTS_ONLY`.

Besides making the behavior more predictable (instead of relying on the
browser to decide when to terminate the current ServiceWorker instance
and create a new one), this change can also improve the developer
experience:
When people notice the error during debugging and fix it by deploying a
new version (either to production or locally), it is confusing that the
ServiceWorker will fetch and install the update (as seen by the requests
in the Network panel in DevTools) but not serve it to clients. With this
change, the update will be served to new clients as soon as it is
installed.

Fixes #31109
@mary-poppins

This comment has been minimized.

Copy link

commented Jul 26, 2019

H--o-l added a commit to H--o-l/angular that referenced this pull request Jul 29, 2019
@mhevery
mhevery approved these changes Sep 4, 2019
@gkalpak

This comment has been minimized.

Copy link
Member Author

commented Sep 4, 2019

merge-assistance: I am a code-owner for service-worker.

@mhevery mhevery closed this in 24b8b34 Sep 4, 2019
mhevery added a commit that referenced this pull request Sep 4, 2019
…sts (#31865)

Helper functions for making navigation requests were created in several
places inside the test suite, so this commit creates a top-level such
helper and uses that in all tests that need it.

PR Close #31865
mhevery added a commit that referenced this pull request Sep 4, 2019
… is invalidated (#31865)

Previously, when the latest version was invalidated (e.g. due to a hash
mismatch), the SW entered a degraded `EXISTING_CLIENTS_ONLY` mode and
removed _all_ clients from its client-version map (essentially stopping
to serve any clients). Based on the code and surrounding comments, the
intention seems to have been to only remove clients that were on the
invalidated version, but keep other clients on older versions.

This commit fixes it by only unassigning clients what were on the latest
version and keep clients assigned to older versions.

PR Close #31865
mhevery added a commit that referenced this pull request Sep 4, 2019
…there is a valid update (#31865)

Previously, when the ServiceWorker entered a degraded mode
(`EXISTING_CLIENTS_ONLY` or `SAFE_MODE`) it would remain in that mode
for the rest of the lifetime of ServiceWorker instance. Note that
ServiceWorkers are stopped by the browser after a certain period of
inactivity and a new instance is created as soon as the ServiceWorker
needs to handle an event (such as a request from the page). Those new
instances would start from the `NORMAL` mode.

The reason for this behavior is to err on the side of caution: If we
can't be sure why the ServiceWorker entered the degraded mode, it is
risky to try recovering on the same instance and might lead to
unexpected behavior.

However, it turns out that the `EXISTING_CLIENTS_ONLY` mode can only be
a result of some error happening with the latest version (e.g. a hash
mismatch in the manifest). Therefore, it is safe to recover from that
mode once a new, valid update is successfully installed and to start
accepting new clients.

This commit ensures that the mode is set back to `NORMAL`, when (a) an
update is successfully installed and (b) the current mode is
`EXISTING_CLIENTS_ONLY`.

Besides making the behavior more predictable (instead of relying on the
browser to decide when to terminate the current ServiceWorker instance
and create a new one), this change can also improve the developer
experience:
When people notice the error during debugging and fix it by deploying a
new version (either to production or locally), it is confusing that the
ServiceWorker will fetch and install the update (as seen by the requests
in the Network panel in DevTools) but not serve it to clients. With this
change, the update will be served to new clients as soon as it is
installed.

Fixes #31109

PR Close #31865
@gkalpak gkalpak deleted the gkalpak:fix-sw-recover-from-degraded branch Sep 5, 2019
sabeersulaiman added a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
…()` (angular#31865)

The `latest` argument was only ever set to the value of comparing
`this.latestHash` with the `appVersion` hash, which is already computed
inside `versionFailed()`, so there is no reason to pass it as an
argument as well.

This doesn't have any impact on the current behavior of the SW.

PR Close angular#31865
sabeersulaiman added a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
…sts (angular#31865)

Helper functions for making navigation requests were created in several
places inside the test suite, so this commit creates a top-level such
helper and uses that in all tests that need it.

PR Close angular#31865
sabeersulaiman added a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
… is invalidated (angular#31865)

Previously, when the latest version was invalidated (e.g. due to a hash
mismatch), the SW entered a degraded `EXISTING_CLIENTS_ONLY` mode and
removed _all_ clients from its client-version map (essentially stopping
to serve any clients). Based on the code and surrounding comments, the
intention seems to have been to only remove clients that were on the
invalidated version, but keep other clients on older versions.

This commit fixes it by only unassigning clients what were on the latest
version and keep clients assigned to older versions.

PR Close angular#31865
sabeersulaiman added a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
…there is a valid update (angular#31865)

Previously, when the ServiceWorker entered a degraded mode
(`EXISTING_CLIENTS_ONLY` or `SAFE_MODE`) it would remain in that mode
for the rest of the lifetime of ServiceWorker instance. Note that
ServiceWorkers are stopped by the browser after a certain period of
inactivity and a new instance is created as soon as the ServiceWorker
needs to handle an event (such as a request from the page). Those new
instances would start from the `NORMAL` mode.

The reason for this behavior is to err on the side of caution: If we
can't be sure why the ServiceWorker entered the degraded mode, it is
risky to try recovering on the same instance and might lead to
unexpected behavior.

However, it turns out that the `EXISTING_CLIENTS_ONLY` mode can only be
a result of some error happening with the latest version (e.g. a hash
mismatch in the manifest). Therefore, it is safe to recover from that
mode once a new, valid update is successfully installed and to start
accepting new clients.

This commit ensures that the mode is set back to `NORMAL`, when (a) an
update is successfully installed and (b) the current mode is
`EXISTING_CLIENTS_ONLY`.

Besides making the behavior more predictable (instead of relying on the
browser to decide when to terminate the current ServiceWorker instance
and create a new one), this change can also improve the developer
experience:
When people notice the error during debugging and fix it by deploying a
new version (either to production or locally), it is confusing that the
ServiceWorker will fetch and install the update (as seen by the requests
in the Network panel in DevTools) but not serve it to clients. With this
change, the update will be served to new clients as soon as it is
installed.

Fixes angular#31109

PR Close angular#31865
H--o-l added a commit to H--o-l/angular that referenced this pull request Sep 14, 2019
H--o-l added a commit to H--o-l/angular that referenced this pull request Sep 14, 2019
arnehoek added a commit to arnehoek/angular that referenced this pull request Sep 26, 2019
…()` (angular#31865)

The `latest` argument was only ever set to the value of comparing
`this.latestHash` with the `appVersion` hash, which is already computed
inside `versionFailed()`, so there is no reason to pass it as an
argument as well.

This doesn't have any impact on the current behavior of the SW.

PR Close angular#31865
arnehoek added a commit to arnehoek/angular that referenced this pull request Sep 26, 2019
…sts (angular#31865)

Helper functions for making navigation requests were created in several
places inside the test suite, so this commit creates a top-level such
helper and uses that in all tests that need it.

PR Close angular#31865
arnehoek added a commit to arnehoek/angular that referenced this pull request Sep 26, 2019
… is invalidated (angular#31865)

Previously, when the latest version was invalidated (e.g. due to a hash
mismatch), the SW entered a degraded `EXISTING_CLIENTS_ONLY` mode and
removed _all_ clients from its client-version map (essentially stopping
to serve any clients). Based on the code and surrounding comments, the
intention seems to have been to only remove clients that were on the
invalidated version, but keep other clients on older versions.

This commit fixes it by only unassigning clients what were on the latest
version and keep clients assigned to older versions.

PR Close angular#31865
arnehoek added a commit to arnehoek/angular that referenced this pull request Sep 26, 2019
…there is a valid update (angular#31865)

Previously, when the ServiceWorker entered a degraded mode
(`EXISTING_CLIENTS_ONLY` or `SAFE_MODE`) it would remain in that mode
for the rest of the lifetime of ServiceWorker instance. Note that
ServiceWorkers are stopped by the browser after a certain period of
inactivity and a new instance is created as soon as the ServiceWorker
needs to handle an event (such as a request from the page). Those new
instances would start from the `NORMAL` mode.

The reason for this behavior is to err on the side of caution: If we
can't be sure why the ServiceWorker entered the degraded mode, it is
risky to try recovering on the same instance and might lead to
unexpected behavior.

However, it turns out that the `EXISTING_CLIENTS_ONLY` mode can only be
a result of some error happening with the latest version (e.g. a hash
mismatch in the manifest). Therefore, it is safe to recover from that
mode once a new, valid update is successfully installed and to start
accepting new clients.

This commit ensures that the mode is set back to `NORMAL`, when (a) an
update is successfully installed and (b) the current mode is
`EXISTING_CLIENTS_ONLY`.

Besides making the behavior more predictable (instead of relying on the
browser to decide when to terminate the current ServiceWorker instance
and create a new one), this change can also improve the developer
experience:
When people notice the error during debugging and fix it by deploying a
new version (either to production or locally), it is confusing that the
ServiceWorker will fetch and install the update (as seen by the requests
in the Network panel in DevTools) but not serve it to clients. With this
change, the update will be served to new clients as soon as it is
installed.

Fixes angular#31109

PR Close angular#31865
H--o-l added a commit to H--o-l/angular that referenced this pull request Sep 30, 2019
H--o-l added a commit to H--o-l/angular that referenced this pull request Sep 30, 2019
H--o-l added a commit to H--o-l/angular that referenced this pull request Sep 30, 2019
atscott added a commit that referenced this pull request Sep 30, 2019
atscott added a commit that referenced this pull request Sep 30, 2019
@angular-automatic-lock-bot

This comment has been minimized.

Copy link

commented Oct 6, 2019

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 Oct 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
4 participants
You can’t perform that action at this time.