fix(service-worker): cache opaque responses when requests exceeds timeout threshold (#30977)

gkalpak · alxhub · commit 93abc3521345 · 2019-06-27T09:52:29.000-07:00
PR Close #30977
diff --git a/packages/service-worker/worker/src/data.ts b/packages/service-worker/worker/src/data.ts
@@ -378,7 +378,7 @@ export class DataGroup {
 
     // If the network fetch times out or errors, fall back on the cache.
     if (res === undefined) {
-      ctx.waitUntil(this.safeCacheResponse(req, networkFetch));
+      ctx.waitUntil(this.safeCacheResponse(req, networkFetch, true));
 
       // Ignore the age, the network response will be cached anyway due to the
       // behavior of freshness.
@@ -434,9 +434,10 @@ export class DataGroup {
     }
   }
 
-  private async safeCacheResponse(req: Request, res: Promise<Response>): Promise<void> {
+  private async safeCacheResponse(req: Request, res: Promise<Response>, okToCacheOpaque?: boolean):
+      Promise<void> {
     try {
-      await this.cacheResponse(req, await res, await this.lru());
+      await this.cacheResponse(req, await res, await this.lru(), okToCacheOpaque);
     } catch {
       // TODO: handle this error somehow?
     }
diff --git a/packages/service-worker/worker/test/data_spec.ts b/packages/service-worker/worker/test/data_spec.ts
@@ -259,6 +259,36 @@ import {SwTestHarness, SwTestHarnessBuilder} from '../testing/scope';
         expect(await makeRequest(scope, '/refresh/data')).toEqual('this is refreshed data');
         serverUpdate.assertNoOtherRequests();
       });
+
+      it('caches opaque responses on refresh', async() => {
+        // Make the initial request and populate the cache.
+        expect(await makeRequest(scope, '/fresh/data')).toBe('this is fresh data');
+        server.assertSawRequestFor('/fresh/data');
+        server.clearRequests();
+
+        // Update the server state and pause the server, so the next request times out.
+        scope.updateServerState(serverUpdate);
+        serverUpdate.pause();
+        const [res, done] =
+            makePendingRequest(scope, new MockRequest('/fresh/data', {mode: 'no-cors'}));
+
+        // The network request times out after 1,000ms and the cached response is returned.
+        await serverUpdate.nextRequest;
+        scope.advance(2000);
+        expect(await res).toBe('this is fresh data');
+
+        // Unpause the server to allow the network request to complete and be cached.
+        serverUpdate.unpause();
+        await done;
+
+        // Pause the server to force the cached (opaque) response to be returned.
+        serverUpdate.pause();
+        const [res2] = makePendingRequest(scope, '/fresh/data');
+        await serverUpdate.nextRequest;
+        scope.advance(2000);
+
+        expect(await res2).toBe('');
+      });
     });
   });
 })();

Original file line number	Diff line number	Diff line change
`@@ -378,7 +378,7 @@ export class DataGroup {`
`378`	`378`
`379`	`379`	`// If the network fetch times out or errors, fall back on the cache.`
`380`	`380`	`if (res === undefined) {`
`381`		`- ctx.waitUntil(this.safeCacheResponse(req, networkFetch));`
	`381`	`+ ctx.waitUntil(this.safeCacheResponse(req, networkFetch, true));`
`382`	`382`
`383`	`383`	`// Ignore the age, the network response will be cached anyway due to the`
`384`	`384`	`// behavior of freshness.`
`@@ -434,9 +434,10 @@ export class DataGroup {`
`434`	`434`	`}`
`435`	`435`	`}`
`436`	`436`
`437`		`- private async safeCacheResponse(req: Request, res: Promise<Response>): Promise<void> {`
	`437`	`+ private async safeCacheResponse(req: Request, res: Promise<Response>, okToCacheOpaque?: boolean):`
	`438`	`+ Promise<void> {`
`438`	`439`	`try {`
`439`		`- await this.cacheResponse(req, await res, await this.lru());`
	`440`	`+ await this.cacheResponse(req, await res, await this.lru(), okToCacheOpaque);`
`440`	`441`	`} catch {`
`441`	`442`	`// TODO: handle this error somehow?`
`442`	`443`	`}`