From 70a5c31f3d7b6ed3a9e38924ce51d768b91bee9a Mon Sep 17 00:00:00 2001 From: George Kalpakas Date: Fri, 26 Jul 2019 20:15:56 +0300 Subject: [PATCH] fix(service-worker): keep serving clients on older versions if latest 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 --- packages/service-worker/worker/src/driver.ts | 18 ++-- .../service-worker/worker/test/happy_spec.ts | 84 ++++++++++++++++++- 2 files changed, 89 insertions(+), 13 deletions(-) diff --git a/packages/service-worker/worker/src/driver.ts b/packages/service-worker/worker/src/driver.ts index a7513bc886826..3c44568657bd6 100644 --- a/packages/service-worker/worker/src/driver.ts +++ b/packages/service-worker/worker/src/driver.ts @@ -742,11 +742,16 @@ export class Driver implements Debuggable, UpdateSource { // This particular AppVersion is broken. First, find the manifest hash. const broken = Array.from(this.versions.entries()).find(([hash, version]) => version === appVersion); + if (broken === undefined) { // This version is no longer in use anyway, so nobody cares. return; } + const brokenHash = broken[0]; + const affectedClients = Array.from(this.clientVersionMap.entries()) + .filter(([clientId, hash]) => hash === brokenHash) + .map(([clientId]) => clientId); // TODO: notify affected apps. @@ -759,17 +764,12 @@ export class Driver implements Debuggable, UpdateSource { this.state = DriverReadyState.EXISTING_CLIENTS_ONLY; this.stateMessage = `Degraded due to: ${errorToString(err)}`; - // Cancel the binding for these clients. - Array.from(this.clientVersionMap.keys()) - .forEach(clientId => this.clientVersionMap.delete(clientId)); + // Cancel the binding for the affected clients. + affectedClients.forEach(clientId => this.clientVersionMap.delete(clientId)); } else { - // The current version is viable, but this older version isn't. The only + // The latest version is viable, but this older version isn't. The only // possible remedy is to stop serving the older version and go to the network. - // Figure out which clients are affected and put them on the latest. - const affectedClients = - Array.from(this.clientVersionMap.keys()) - .filter(clientId => this.clientVersionMap.get(clientId) ! === brokenHash); - // Push the affected clients onto the latest version. + // Put the affected clients on the latest version. affectedClients.forEach(clientId => this.clientVersionMap.set(clientId, this.latestHash !)); } diff --git a/packages/service-worker/worker/test/happy_spec.ts b/packages/service-worker/worker/test/happy_spec.ts index e28777e3bde1e..c9d550ac20472 100644 --- a/packages/service-worker/worker/test/happy_spec.ts +++ b/packages/service-worker/worker/test/happy_spec.ts @@ -52,7 +52,10 @@ import {SwTestHarness, SwTestHarnessBuilder} from '../testing/scope'; .addUnhashedFile('/ignored/dir/file2', 'this is not handled by the SW either') .build(); - const brokenFs = new MockFileSystemBuilder().addFile('/foo.txt', 'this is foo').build(); + const brokenFs = new MockFileSystemBuilder() + .addFile('/foo.txt', 'this is foo (broken)') + .addFile('/bar.txt', 'this is bar (broken)') + .build(); const brokenManifest: Manifest = { configVersion: 1, @@ -72,6 +75,35 @@ import {SwTestHarness, SwTestHarnessBuilder} from '../testing/scope'; hashTable: tmpHashTableForFs(brokenFs, {'/foo.txt': true}), }; + const brokenLazyManifest: Manifest = { + configVersion: 1, + timestamp: 1234567890123, + index: '/foo.txt', + assetGroups: [ + { + name: 'assets', + installMode: 'prefetch', + updateMode: 'prefetch', + urls: [ + '/foo.txt', + ], + patterns: [], + }, + { + name: 'lazy-assets', + installMode: 'lazy', + updateMode: 'lazy', + urls: [ + '/bar.txt', + ], + patterns: [], + }, + ], + dataGroups: [], + navigationUrls: processNavigationUrls(''), + hashTable: tmpHashTableForFs(brokenFs, {'/bar.txt': true}), + }; + // Manifest without navigation urls to test backward compatibility with // versions < 6.0.0. interface ManifestV5 { @@ -226,6 +258,11 @@ import {SwTestHarness, SwTestHarnessBuilder} from '../testing/scope'; const brokenServer = new MockServerStateBuilder().withStaticFiles(brokenFs).withManifest(brokenManifest).build(); + const brokenLazyServer = new MockServerStateBuilder() + .withStaticFiles(brokenFs) + .withManifest(brokenLazyManifest) + .build(); + const server404 = new MockServerStateBuilder().withStaticFiles(dist).build(); const manifestHash = sha1(JSON.stringify(manifest)); @@ -1150,7 +1187,7 @@ import {SwTestHarness, SwTestHarnessBuilder} from '../testing/scope'; (scope.registration as any).scope = 'http://site.com'; driver = new Driver(scope, scope, new CacheDatabase(scope, scope)); - expect(await makeRequest(scope, '/foo.txt')).toEqual('this is foo'); + expect(await makeRequest(scope, '/foo.txt')).toEqual('this is foo (broken)'); }); it('enters degraded mode when update has a bad index', async() => { @@ -1193,6 +1230,45 @@ import {SwTestHarness, SwTestHarnessBuilder} from '../testing/scope'; server.assertSawRequestFor('/foo.txt'); }); + it('enters degraded mode when something goes wrong with the latest version', async() => { + await driver.initialized; + + // Two clients on initial version. + expect(await makeRequest(scope, '/foo.txt', 'client1')).toBe('this is foo'); + expect(await makeRequest(scope, '/foo.txt', 'client2')).toBe('this is foo'); + + // Install a broken version (`bar.txt` has invalid hash). + scope.updateServerState(brokenLazyServer); + await driver.checkForUpdate(); + + // Update `client1` but not `client2`. + await makeNavigationRequest(scope, '/', 'client1'); + server.clearRequests(); + brokenLazyServer.clearRequests(); + + expect(await makeRequest(scope, '/foo.txt', 'client1')).toBe('this is foo (broken)'); + expect(await makeRequest(scope, '/foo.txt', 'client2')).toBe('this is foo'); + server.assertNoOtherRequests(); + brokenLazyServer.assertNoOtherRequests(); + + // Trying to fetch `bar.txt` (which has an invalid hash) should invalidate the latest + // version, enter degraded mode and "forget" clients that are on that version (i.e. + // `client1`). + expect(await makeRequest(scope, '/bar.txt', 'client1')).toBe('this is bar (broken)'); + expect(driver.state).toBe(DriverReadyState.EXISTING_CLIENTS_ONLY); + brokenLazyServer.sawRequestFor('/bar.txt'); + brokenLazyServer.clearRequests(); + + // `client1` should not be served from the network. + expect(await makeRequest(scope, '/foo.txt', 'client1')).toBe('this is foo (broken)'); + brokenLazyServer.sawRequestFor('/foo.txt'); + + // `client2` should still be served from the old version (since it never updated). + expect(await makeRequest(scope, '/foo.txt', 'client2')).toBe('this is foo'); + server.assertNoOtherRequests(); + brokenLazyServer.assertNoOtherRequests(); + }); + it('ignores invalid `only-if-cached` requests ', async() => { const requestFoo = (cache: RequestCache | 'only-if-cached', mode: RequestMode) => makeRequest(scope, '/foo.txt', undefined, {cache, mode}); @@ -1234,7 +1310,7 @@ import {SwTestHarness, SwTestHarnessBuilder} from '../testing/scope'; expect(requestUrls2).toContain(httpsRequestUrl); }); - describe('Backwards compatibility with v5', () => { + describe('backwards compatibility with v5', () => { beforeEach(() => { const serverV5 = new MockServerStateBuilder() .withStaticFiles(dist) @@ -1246,7 +1322,7 @@ import {SwTestHarness, SwTestHarnessBuilder} from '../testing/scope'; }); // Test this bug: https://github.com/angular/angular/issues/27209 - it('Fill previous versions of manifests with default navigation urls for backwards compatibility', + it('fills previous versions of manifests with default navigation urls for backwards compatibility', async() => { expect(await makeRequest(scope, '/foo.txt')).toEqual('this is foo'); await driver.initialized;