fix(service-worker): continue serving api requests on cache failure (#…

…33165)

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.
Same changes for master branch in PR #32996

Fixes #21412

PR Close #33165

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

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

packages/service-worker/worker/src/driver.ts

@@ -534,7 +534,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));
       }
     });
 
@@ -784,7 +785,8 @@ export class Driver implements Debuggable, UpdateSource {
   }
 
   private async setupUpdate(manifest: Manifest, hash: string): Promise<void> {
-    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) {

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 =
@@ -140,11 +143,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),
@@ -809,6 +822,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) => {
@@ -1212,6 +1228,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('ignores invalid `only-if-cached` requests ', async() => {
         const requestFoo = (cache: RequestCache | 'only-if-cached', mode: RequestMode) =>
             makeRequest(scope, '/foo.txt', undefined, {cache, mode});