diff --git a/packages/service-worker/worker/src/assets.ts b/packages/service-worker/worker/src/assets.ts index cec901050b97..fc1fbb2c1b72 100644 --- a/packages/service-worker/worker/src/assets.ts +++ b/packages/service-worker/worker/src/assets.ts @@ -16,6 +16,22 @@ import {AssetGroupConfig} from './manifest'; import {NamedCache} from './named-cache-storage'; import {sha1Binary} from './sha1'; +const UNCACHEABLE_CACHE_CONTROL_DIRECTIVES = new Set(['no-store', 'private', 'no-cache']); + +function hasUncacheableCacheControl(headers: Headers): boolean { + const cacheControl = headers.get('cache-control'); + + if (!cacheControl) { + return false; + } + + return cacheControl.split(',').some((directive) => { + const directiveName = directive.split('=', 1)[0].trim().toLowerCase(); + + return UNCACHEABLE_CACHE_CONTROL_DIRECTIVES.has(directiveName); + }); +} + /** * A group of assets that are cached in a `Cache` and managed by a given policy. * @@ -151,16 +167,20 @@ export abstract class AssetGroup { // the response. return cachedResponse; } else { - // This resource has no hash, and yet exists in the cache. Check how old this request is - // to make sure it's still usable. - if (await this.needToRevalidate(req, cachedResponse)) { - this.idle.schedule(`revalidate(${cache.name}): ${req.url}`, async () => { - await this.fetchAndCacheOnce(req); - }); + if (hasUncacheableCacheControl(cachedResponse.headers)) { + await this.removeCachedResource(req, cache); + } else { + // This resource has no hash, and yet exists in the cache. Check how old this request + // is to make sure it's still usable. + if (await this.needToRevalidate(req, cachedResponse)) { + this.idle.schedule(`revalidate(${cache.name}): ${req.url}`, async () => { + await this.fetchAndCacheOnce(req); + }); + } + + // In either case (revalidation or not), the cached response must be good. + return cachedResponse; } - - // In either case (revalidation or not), the cached response must be good. - return cachedResponse; } } @@ -298,10 +318,13 @@ export abstract class AssetGroup { * Fetch the given resource from the network, and cache it if able. */ protected async fetchAndCacheOnce(req: Request, used: boolean = true): Promise { + const url = this.adapter.normalizeUrl(req.url); + const shouldDeduplicate = this.hashes.has(url); + // The `inFlightRequests` map holds information about which caching operations are currently - // underway for known resources. If this request appears there, another "thread" is already + // underway for hashed resources. If this request appears there, another "thread" is already // in the process of caching it, and this work should not be duplicated. - if (this.inFlightRequests.has(req.url)) { + if (shouldDeduplicate && this.inFlightRequests.has(req.url)) { // There is a caching operation already in progress for this request. Wait for it to // complete, and hopefully it will have yielded a useful response. return this.inFlightRequests.get(req.url)!; @@ -313,7 +336,9 @@ export abstract class AssetGroup { // Save this operation in `inFlightRequests` so any other "thread" attempting to cache it // will block on this chain instead of duplicating effort. - this.inFlightRequests.set(req.url, fetchOp); + if (shouldDeduplicate) { + this.inFlightRequests.set(req.url, fetchOp); + } // Make sure this attempt is cleaned up properly on failure. try { @@ -329,6 +354,10 @@ export abstract class AssetGroup { ); } + if (!this.hashes.has(url) && hasUncacheableCacheControl(res.headers)) { + return res; + } + try { // This response is safe to cache (as long as it's cloned). Wait until the cache operation // is complete. @@ -359,7 +388,9 @@ export abstract class AssetGroup { } finally { // Finally, it can be removed from `inFlightRequests`. This might result in a double-remove // if some other chain was already making this request too, but that won't hurt anything. - this.inFlightRequests.delete(req.url); + if (shouldDeduplicate) { + this.inFlightRequests.delete(req.url); + } } } @@ -383,6 +414,15 @@ export abstract class AssetGroup { return res; } + private async removeCachedResource(req: Request, cache: Cache): Promise { + await cache.delete(req, this.config.cacheQueryOptions); + + if (!this.hashes.has(this.adapter.normalizeUrl(req.url))) { + const metaTable = await this.metadata; + await metaTable.delete(req.url); + } + } + /** * Load a particular asset from the network, accounting for hash validation. */ @@ -648,6 +688,10 @@ export class PrefetchAssetGroup extends AssetGroup { return; } + if (hasUncacheableCacheControl(res.response.headers)) { + return; + } + // Write it into the cache. It may already be expired, but it can still serve // traffic until it's updated (stale-while-revalidate approach). await cache.put(req, res.response); diff --git a/packages/service-worker/worker/test/happy_spec.ts b/packages/service-worker/worker/test/happy_spec.ts index 2af08b1b8be5..5c25243225fa 100644 --- a/packages/service-worker/worker/test/happy_spec.ts +++ b/packages/service-worker/worker/test/happy_spec.ts @@ -43,6 +43,14 @@ import {envIsSupported} from '../testing/utils'; .addFile('/lazy/redirect-target.txt', 'this was a redirect too') .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('/unhashed/no-store.txt', 'this is no-store', { + 'Cache-Control': 'no-store, max-age=60', + Pragma: 'no-cache', + Expires: '0', + }) + .addUnhashedFile('/unhashed/private.txt', 'this is private', { + 'Cache-Control': 'private, max-age=60', + }) .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(); @@ -58,6 +66,15 @@ import {envIsSupported} from '../testing/utils'; .addFile('/lazy/unchanged1.txt', 'this is unchanged (1)') .addFile('/lazy/unchanged2.txt', 'this is unchanged (2)') .addUnhashedFile('/unhashed/a.txt', 'this is unhashed v2', {'Cache-Control': 'max-age=10'}) + .addUnhashedFile('/unhashed/b.txt', 'this is unhashed b v2', {'Cache-Control': 'no-cache'}) + .addUnhashedFile('/unhashed/no-store.txt', 'this is no-store v2', { + 'Cache-Control': 'no-store, max-age=60', + Pragma: 'no-cache', + Expires: '0', + }) + .addUnhashedFile('/unhashed/private.txt', 'this is private v2', { + 'Cache-Control': 'private, max-age=60', + }) .addUnhashedFile('/ignored/file1', 'this is not handled by the SW') .addUnhashedFile('/ignored/dir/file2', 'this is not handled by the SW either') .build(); @@ -1795,13 +1812,107 @@ import {envIsSupported} from '../testing/utils'; server.assertNoOtherRequests(); }); - it(`don't error when 'Cache-Control' is 'no-cache'`, async () => { - expect(await makeRequest(scope, '/unhashed/b.txt')).toEqual('this is unhashed b'); - server.assertSawRequestFor('/unhashed/b.txt'); - expect(await makeRequest(scope, '/unhashed/b.txt')).toEqual('this is unhashed b'); + it('does not deduplicate concurrent unhashed requests', async () => { + server.pause(); + + const firstNetworkRequest = server.nextRequest; + const [firstResponse, firstDone] = scope.handleFetch( + new MockRequest('/unhashed/a.txt'), + 'default', + ); + await firstNetworkRequest; + + const secondNetworkRequest = server.nextRequest; + const [secondResponse, secondDone] = scope.handleFetch( + new MockRequest('/unhashed/a.txt'), + 'default', + ); + let sawSecondNetworkRequest = false; + secondNetworkRequest.then(() => (sawSecondNetworkRequest = true)); + await new Promise((resolve) => setTimeout(resolve)); // Wait for async operations to complete. + expect(sawSecondNetworkRequest).toBeTrue(); + + server.unpause(); + await Promise.all([firstDone, secondDone]); + + expect(await (await firstResponse)?.text()).toEqual('this is unhashed'); + expect(await (await secondResponse)?.text()).toEqual('this is unhashed'); + server.assertSawRequestFor('/unhashed/a.txt'); + server.assertSawRequestFor('/unhashed/a.txt'); server.assertNoOtherRequests(); }); + const uncacheableUnhashedResponses: { + description: string; + url: string; + first: string; + second: string; + firstInit?: Object; + secondInit?: Object; + }[] = [ + { + description: '`Cache-Control: no-cache`', + url: '/unhashed/b.txt', + first: 'this is unhashed b', + second: 'this is unhashed b v2', + }, + { + description: '`Cache-Control: no-store`', + url: '/unhashed/no-store.txt', + first: 'this is no-store', + second: 'this is no-store v2', + secondInit: {cache: 'no-store'}, + }, + { + description: '`Cache-Control: private`', + url: '/unhashed/private.txt', + first: 'this is private', + second: 'this is private v2', + }, + ]; + + uncacheableUnhashedResponses.forEach( + ({description, url, first, second, firstInit, secondInit}) => { + it(`do not cache responses with ${description}`, async () => { + expect(await makeRequest(scope, url, 'default', firstInit)).toEqual(first); + server.assertSawRequestFor(url); + server.assertNoOtherRequests(); + expect(await scope.caches.original.match(new MockRequest(url))).toBeUndefined(); + + scope.updateServerState(serverUpdate); + + expect(await makeRequest(scope, url, 'default', secondInit)).toEqual(second); + serverUpdate.assertSawRequestFor(url); + serverUpdate.assertNoOtherRequests(); + expect(await scope.caches.original.match(new MockRequest(url))).toBeUndefined(); + }); + }, + ); + + it(`does not serve a previously cached response with 'Cache-Control: no-store'`, async () => { + const cache = await scope.caches.open(`${manifestHash}:assets:assets:cache`); + await cache.put( + new MockRequest('/unhashed/no-store.txt'), + new MockResponse('stale no-store response', { + headers: {'Cache-Control': 'no-store'}, + }), + ); + expect( + await scope.caches.original.match(new MockRequest('/unhashed/no-store.txt')), + ).toBeDefined(); + + scope.updateServerState(serverUpdate); + + expect( + await makeRequest(scope, '/unhashed/no-store.txt', 'default', {cache: 'no-store'}), + ).toEqual('this is no-store v2'); + serverUpdate.assertSawRequestFor('/unhashed/no-store.txt'); + serverUpdate.assertNoOtherRequests(); + expect( + await scope.caches.original.match(new MockRequest('/unhashed/no-store.txt')), + ).toBeUndefined(); + }); + it('avoid opaque responses', async () => { expect( await makeRequest(scope, '/unhashed/a.txt', 'default', {