From 52483bf6803166a0b004bbc68dfec97b0346937d Mon Sep 17 00:00:00 2001 From: Denis Omelkov Date: Mon, 14 Oct 2019 19:41:35 +0500 Subject: [PATCH] fix(service-worker): continue serving api requests on cache failure (#32996) When responses are cached ok during sw initialization, but caching throws an error when handling api response, this response never gets to client. Fix response delivery by catching errors, add logging and 2 test cases. Fixes #21412 PR Close #32996 --- .../service-worker/worker/src/app-version.ts | 15 +++-- packages/service-worker/worker/src/data.ts | 13 ++-- packages/service-worker/worker/src/driver.ts | 6 +- .../service-worker/worker/test/happy_spec.ts | 60 ++++++++++++++++++- 4 files changed, 81 insertions(+), 13 deletions(-) diff --git a/packages/service-worker/worker/src/app-version.ts b/packages/service-worker/worker/src/app-version.ts index 5db2d6d3a4..434b1ad038 100644 --- a/packages/service-worker/worker/src/app-version.ts +++ b/packages/service-worker/worker/src/app-version.ts @@ -11,6 +11,7 @@ import {CacheState, UpdateCacheStatus, UpdateSource} from './api'; import {AssetGroup, LazyAssetGroup, PrefetchAssetGroup} from './assets'; import {DataGroup} from './data'; import {Database} from './database'; +import {DebugHandler} from './debug'; import {IdleScheduler} from './idle'; import {Manifest} from './manifest'; @@ -60,7 +61,8 @@ export class AppVersion implements UpdateSource { constructor( private scope: ServiceWorkerGlobalScope, private adapter: Adapter, private database: Database, - private idle: IdleScheduler, readonly manifest: Manifest, readonly manifestHash: string) { + private idle: IdleScheduler, private debugHandler: DebugHandler, readonly manifest: Manifest, + readonly manifestHash: string) { // The hashTable within the manifest is an Object - convert it to a Map for easier lookups. Object.keys(this.manifest.hashTable).forEach(url => { this.hashTable.set(url, this.manifest.hashTable[url]); @@ -85,11 +87,12 @@ export class AppVersion implements UpdateSource { }); // Process each `DataGroup` declared in the manifest. - this.dataGroups = (manifest.dataGroups || []) - .map( - config => new DataGroup( - this.scope, this.adapter, config, this.database, - `${adapter.cacheNamePrefix}:${config.version}:data`)); + this.dataGroups = + (manifest.dataGroups || []) + .map( + config => new DataGroup( + this.scope, this.adapter, config, this.database, this.debugHandler, + `${adapter.cacheNamePrefix}:${config.version}:data`)); // This keeps backwards compatibility with app versions without navigation urls. // Fix: https://github.com/angular/angular/issues/27209 diff --git a/packages/service-worker/worker/src/data.ts b/packages/service-worker/worker/src/data.ts index 9ba08f029f..4d9fdd7242 100644 --- a/packages/service-worker/worker/src/data.ts +++ b/packages/service-worker/worker/src/data.ts @@ -8,6 +8,7 @@ import {Adapter, Context} from './adapter'; import {Database, Table} from './database'; +import {DebugHandler} from './debug'; import {DataGroupConfig} from './manifest'; /** @@ -243,7 +244,8 @@ export class DataGroup { constructor( private scope: ServiceWorkerGlobalScope, private adapter: Adapter, - private config: DataGroupConfig, private db: Database, private prefix: string) { + private config: DataGroupConfig, private db: Database, private debugHandler: DebugHandler, + private prefix: string) { this.patterns = this.config.patterns.map(pattern => new RegExp(pattern)); this.cache = this.scope.caches.open(`${this.prefix}:dynamic:${this.config.name}:cache`); this.lruTable = this.db.open(`${this.prefix}:dynamic:${this.config.name}:lru`); @@ -356,7 +358,7 @@ export class DataGroup { ctx.waitUntil(this.safeCacheResponse(req, networkFetch, lru)); } else { // The request completed in time, so cache it inline with the response flow. - await this.cacheResponse(req, res, lru); + await this.safeCacheResponse(req, res, lru); } return res; @@ -385,7 +387,7 @@ export class DataGroup { const fromCache = await this.loadFromCache(req, lru); res = (fromCache !== null) ? fromCache.res : null; } else { - await this.cacheResponse(req, res, lru, true); + await this.safeCacheResponse(req, res, lru, true); } // Either the network fetch didn't time out, or the cache yielded a usable response. @@ -442,7 +444,10 @@ export class DataGroup { } catch (err) { // Saving the API response failed. This could be a result of a full storage. // Since this data is cached lazily and temporarily, continue serving clients as usual. - // TODO: Log error + this.debugHandler.log( + err, + `DataGroup(${this.config.name}@${this.config.version}).safeCacheResponse(${req.url}, status: ${res.status})`); + // TODO: Better detect/handle full storage; e.g. using // [navigator.storage](https://developer.mozilla.org/en-US/docs/Web/API/NavigatorStorage/storage). } diff --git a/packages/service-worker/worker/src/driver.ts b/packages/service-worker/worker/src/driver.ts index 09b966aeed..afa0eb71b0 100644 --- a/packages/service-worker/worker/src/driver.ts +++ b/packages/service-worker/worker/src/driver.ts @@ -533,7 +533,8 @@ export class Driver implements Debuggable, UpdateSource { // created for it. if (!this.versions.has(hash)) { this.versions.set( - hash, new AppVersion(this.scope, this.adapter, this.db, this.idle, manifest, hash)); + hash, new AppVersion( + this.scope, this.adapter, this.db, this.idle, this.debugger, manifest, hash)); } }); @@ -783,7 +784,8 @@ export class Driver implements Debuggable, UpdateSource { } private async setupUpdate(manifest: Manifest, hash: string): Promise { - const newVersion = new AppVersion(this.scope, this.adapter, this.db, this.idle, manifest, hash); + const newVersion = + new AppVersion(this.scope, this.adapter, this.db, this.idle, this.debugger, manifest, hash); // Firstly, check if the manifest version is correct. if (manifest.configVersion !== SUPPORTED_CONFIG_VERSION) { diff --git a/packages/service-worker/worker/test/happy_spec.ts b/packages/service-worker/worker/test/happy_spec.ts index aa6a673473..0bb0a93005 100644 --- a/packages/service-worker/worker/test/happy_spec.ts +++ b/packages/service-worker/worker/test/happy_spec.ts @@ -34,6 +34,9 @@ import {SwTestHarness, SwTestHarnessBuilder} from '../testing/scope'; .addFile('/lazy/unchanged2.txt', 'this is unchanged (2)') .addUnhashedFile('/unhashed/a.txt', 'this is unhashed', {'Cache-Control': 'max-age=10'}) .addUnhashedFile('/unhashed/b.txt', 'this is unhashed b', {'Cache-Control': 'no-cache'}) + .addUnhashedFile('/api/foo', 'this is api foo', {'Cache-Control': 'no-cache'}) + .addUnhashedFile( + '/api-static/bar', 'this is static api bar', {'Cache-Control': 'no-cache'}) .build(); const distUpdate = @@ -172,11 +175,21 @@ import {SwTestHarness, SwTestHarnessBuilder} from '../testing/scope'; version: 42, maxAge: 3600000, maxSize: 100, - strategy: 'performance', + strategy: 'freshness', patterns: [ '/api/.*', ], }, + { + name: 'api-static', + version: 43, + maxAge: 3600000, + maxSize: 100, + strategy: 'performance', + patterns: [ + '/api-static/.*', + ], + }, ], navigationUrls: processNavigationUrls(''), hashTable: tmpHashTableForFs(dist), @@ -829,6 +842,9 @@ import {SwTestHarness, SwTestHarnessBuilder} from '../testing/scope'; `ngsw:${baseHref}:42:data:dynamic:api:cache`, `ngsw:${baseHref}:db:ngsw:${baseHref}:42:data:dynamic:api:lru`, `ngsw:${baseHref}:db:ngsw:${baseHref}:42:data:dynamic:api:age`, + `ngsw:${baseHref}:43:data:dynamic:api-static:cache`, + `ngsw:${baseHref}:db:ngsw:${baseHref}:43:data:dynamic:api-static:lru`, + `ngsw:${baseHref}:db:ngsw:${baseHref}:43:data:dynamic:api-static:age`, ]; const getClientAssignments = async(sw: SwTestHarness, baseHref: string) => { @@ -1230,6 +1246,48 @@ import {SwTestHarness, SwTestHarnessBuilder} from '../testing/scope'; server.assertSawRequestFor('/foo.txt'); }); + it('keeps serving api requests with freshness strategy when failing to write to cache', + async() => { + // Initialize the SW. + expect(await makeRequest(scope, '/foo.txt')).toEqual('this is foo'); + await driver.initialized; + server.clearRequests(); + + // Make the caches unwritable. + spyOn(MockCache.prototype, 'put').and.throwError('Can\'t touch this'); + spyOn(driver.debugger, 'log'); + + expect(await makeRequest(scope, '/api/foo')).toEqual('this is api foo'); + expect(driver.state).toBe(DriverReadyState.NORMAL); + // Since we are swallowing an error here, make sure it is at least properly logged + expect(driver.debugger.log) + .toHaveBeenCalledWith( + new Error('Can\'t touch this'), + 'DataGroup(api@42).safeCacheResponse(/api/foo, status: 200)'); + server.assertSawRequestFor('/api/foo'); + }); + + it('keeps serving api requests with performance strategy when failing to write to cache', + async() => { + // Initialize the SW. + expect(await makeRequest(scope, '/foo.txt')).toEqual('this is foo'); + await driver.initialized; + server.clearRequests(); + + // Make the caches unwritable. + spyOn(MockCache.prototype, 'put').and.throwError('Can\'t touch this'); + spyOn(driver.debugger, 'log'); + + expect(await makeRequest(scope, '/api-static/bar')).toEqual('this is static api bar'); + expect(driver.state).toBe(DriverReadyState.NORMAL); + // Since we are swallowing an error here, make sure it is at least properly logged + expect(driver.debugger.log) + .toHaveBeenCalledWith( + new Error('Can\'t touch this'), + 'DataGroup(api-static@43).safeCacheResponse(/api-static/bar, status: 200)'); + server.assertSawRequestFor('/api-static/bar'); + }); + it('enters degraded mode when something goes wrong with the latest version', async() => { await driver.initialized;