Skip to content

Commit

Permalink
fix(service-worker): correctly handle failed cache-busted request (#3…
Browse files Browse the repository at this point in the history 
…9786)

Since 5be4edf, a failing cache-busted
network request (such as requests for fetching uncached assets) will
cause the ServiceWorker to incorrectly enter a degraded
`EXISTING_CLIENTS_ONLY` mode. A failing network request could be caused
by many reasons, including the client or server being offline, and does
not necessarily signify a broken ServiceWorker state.

This commit fixes the logic in `cacheBustedFetchFromNetwork()` to
correctly handle errors in network requests.
For more details on the problem and the implemented fix see #39775.

Fixes #39775

PR Close #39786
  • Loading branch information
@gkalpak @AndrewKushnir
gkalpak authored and AndrewKushnir committed Nov 23, 2020
1 parent 5a49465 commit 6046419
Show file tree
Hide file tree
Showing 2 changed files with 54 additions and 41 deletions.
72 changes: 33 additions & 39 deletions packages/service-worker/worker/src/assets.ts
View file
Expand Up @@ -388,18 +388,17 @@ export abstract class AssetGroup {
// a stale response.

// Fetch the resource from the network (possibly hitting the HTTP cache).
const networkResult = await this.safeFetch(req);

// Decide whether a cache-busted request is necessary. It might be for two independent
// reasons: either the non-cache-busted request failed (hopefully transiently) or if the
// hash of the content retrieved does not match the canonical hash from the manifest. It's
// only valid to access the content of the first response if the request was successful.
let makeCacheBustedRequest: boolean = !networkResult.ok;
if (networkResult.ok) {
let response = await this.safeFetch(req);

// Decide whether a cache-busted request is necessary. A cache-busted request is necessary
// only if the request was successful but the hash of the retrieved contents does not match
// the canonical hash from the manifest.
let makeCacheBustedRequest = response.ok;
if (makeCacheBustedRequest) {
// The request was successful. A cache-busted request is only necessary if the hashes
// don't match. Compare them, making sure to clone the response so it can be used later
// if it proves to be valid.
const fetchedHash = sha1Binary(await networkResult.clone().arrayBuffer());
// don't match.
// (Make sure to clone the response so it can be used later if it proves to be valid.)
const fetchedHash = sha1Binary(await response.clone().arrayBuffer());
makeCacheBustedRequest = (fetchedHash !== canonicalHash);
}

Expand All @@ -411,39 +410,34 @@ export abstract class AssetGroup {
// request will differentiate these two situations.
// TODO: handle case where the URL has parameters already (unlikely for assets).
const cacheBustReq = this.adapter.newRequest(this.cacheBust(req.url));
const cacheBustedResult = await this.safeFetch(cacheBustReq);

// If the response was unsuccessful, there's nothing more that can be done.
if (!cacheBustedResult.ok) {
if (cacheBustedResult.status === 404) {
throw new SwUnrecoverableStateError(
`Failed to retrieve hashed resource from the server. (AssetGroup: ${
this.config.name} | URL: ${url})`);
} else {
throw new SwCriticalError(
`Response not Ok (cacheBustedFetchFromNetwork): cache busted request for ${
req.url} returned response ${cacheBustedResult.status} ${
cacheBustedResult.statusText}`);
response = await this.safeFetch(cacheBustReq);

// If the response was successful, check the contents against the canonical hash.
if (response.ok) {
// Hash the contents.
// (Make sure to clone the response so it can be used later if it proves to be valid.)
const cacheBustedHash = sha1Binary(await response.clone().arrayBuffer());

// If the cache-busted version doesn't match, then the manifest is not an accurate
// representation of the server's current set of files, and the SW should give up.
if (canonicalHash !== cacheBustedHash) {
throw new SwCriticalError(`Hash mismatch (cacheBustedFetchFromNetwork): ${
req.url}: expected ${canonicalHash}, got ${cacheBustedHash} (after cache busting)`);
}
}
}

// Hash the contents.
const cacheBustedHash = sha1Binary(await cacheBustedResult.clone().arrayBuffer());

// If the cache-busted version doesn't match, then the manifest is not an accurate
// representation of the server's current set of files, and the SW should give up.
if (canonicalHash !== cacheBustedHash) {
throw new SwCriticalError(`Hash mismatch (cacheBustedFetchFromNetwork): ${
req.url}: expected ${canonicalHash}, got ${cacheBustedHash} (after cache busting)`);
}

// If it does match, then use the cache-busted result.
return cacheBustedResult;
// At this point, `response` is either successful with a matching hash or is unsuccessful.
// Before returning it, check whether it failed with a 404 status. This would signify an
// unrecoverable state.
if (!response.ok && (response.status === 404)) {
throw new SwUnrecoverableStateError(
`Failed to retrieve hashed resource from the server. (AssetGroup: ${
this.config.name} | URL: ${url})`);
}

// Excellent, the version from the network matched on the first try, with no need for
// cache-busting. Use it.
return networkResult;
// Return the response (successful or unsuccessful).
return response;
} else {
// This URL doesn't exist in our hash database, so it must be requested directly.
return this.safeFetch(req);
Expand Down
23 changes: 21 additions & 2 deletions packages/service-worker/worker/test/happy_spec.ts
View file
Expand Up @@ -794,7 +794,7 @@ describe('Driver', () => {
serverUpdate.assertNoOtherRequests();
});

it('should bypass serviceworker on ngsw-bypass parameter', async () => {
it('bypasses the ServiceWorker on `ngsw-bypass` parameter', async () => {
// NOTE:
// Requests that bypass the SW are not handled at all in the mock implementation of `scope`,
// therefore no requests reach the server.
Expand Down Expand Up @@ -1115,7 +1115,7 @@ describe('Driver', () => {
server.assertNoOtherRequests();
});

it(`doesn't error when 'Cache-Control' is 'no-cache'`, async () => {
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');
Expand Down Expand Up @@ -1744,6 +1744,25 @@ describe('Driver', () => {
expect(requestUrls2).toContain(httpsRequestUrl);
});

it('does not enter degraded mode when offline while fetching an uncached asset', async () => {
// Trigger SW initialization and wait for it to complete.
expect(await makeRequest(scope, '/foo.txt')).toBe('this is foo');
await driver.initialized;

// Request an uncached asset while offline.
// The SW will not be able to get the content, but it should not enter a degraded mode either.
server.online = false;
await expectAsync(makeRequest(scope, '/baz.txt'))
.toBeRejectedWithError(
'Response not Ok (fetchAndCacheOnce): request for /baz.txt returned response 504 Gateway Timeout');
expect(driver.state).toBe(DriverReadyState.NORMAL);

// Once we are back online, everything should work as expected.
server.online = true;
expect(await makeRequest(scope, '/baz.txt')).toBe('this is baz');
expect(driver.state).toBe(DriverReadyState.NORMAL);
});

describe('unrecoverable state', () => {
const generateMockServerState = (fileSystem: MockFileSystem) => {
const manifest: Manifest = {
Expand Down

0 comments on commit 6046419

Please sign in to comment.