From fddb50b597ceaacc77f8412dda29f0ae2bd3efbe Mon Sep 17 00:00:00 2001 From: George Kalpakas Date: Tue, 5 Oct 2021 20:13:39 +0300 Subject: [PATCH] fix(service-worker): make `ngsw.json` generation deterministic and correct (#43679) Previously, all asset-groups from `ngsw-config.json` were processed in parallel. For each asset-group, we retrieved all files for the current build, filtered out files that were already matched by other asset-groups, determined which of the remaining files belonged to the current asset-group and generated entries for the `ngsw.json` manifest. This process was susceptible to race conditions when there were files that would be matched by multiple asset-groups. This made the generation of the `ngsw.json` manifest non-deterministic and violated the rule that each file would belong to the first asset-group that matched it (based on the asset-groups' order of appearance in `ngsw-config.json`), thus leading to broken ServiceWorker behavior. This commit fixes it by ensuring that the generation process is deterministic and that asset-groups are processed in the proper order. NOTE 1: The generation process has been broken since the beginning, but we have only noticed this recently. This is possibly related to the CLI's switching from a virtual file system host (which has more consistent timing characteristics) to the Node.js built-in `fs.promises` in angular/angular-cli@d3bc530c103849ca83742ab63eea7709d9af613d. NOTE 2: This commit also ensures that files in the `ngsw.json` hash-table are in alphabetic order. Previously, the files were added to the hash-table in blocks corresponding to each asset-group. This change is not necessary (i.e. the order of keys in the hash-table makes no difference in behavior), but it makes it easier to scan for a file (for example, for debugging purposes). PR Close #43679 --- .../service-worker/config/src/generator.ts | 46 +++++---- .../config/test/generator_spec.ts | 96 +++++++++++++++++++ 2 files changed, 123 insertions(+), 19 deletions(-) diff --git a/packages/service-worker/config/src/generator.ts b/packages/service-worker/config/src/generator.ts index 61f3e0e5da1..99feba49d53 100644 --- a/packages/service-worker/config/src/generator.ts +++ b/packages/service-worker/config/src/generator.ts @@ -9,7 +9,7 @@ import {parseDurationToMs} from './duration'; import {Filesystem} from './filesystem'; import {globToRegex} from './glob'; -import {Config} from './in'; +import {AssetGroup, Config} from './in'; const DEFAULT_NAVIGATION_URLS = [ '/**', // Include all URLs. @@ -45,8 +45,13 @@ export class Generator { private async processAssetGroups(config: Config, hashTable: {[file: string]: string|undefined}): Promise { + // Retrieve all files of the build. + const allFiles = await this.fs.list('/'); const seenMap = new Set(); - return Promise.all((config.assetGroups || []).map(async (group) => { + const filesPerGroup = new Map(); + + // Computed which files belong to each asset-group. + for (const group of (config.assetGroups || [])) { if ((group.resources as any).versionedFiles) { throw new Error( `Asset-group '${group.name}' in 'ngsw-config.json' uses the 'versionedFiles' option, ` + @@ -54,27 +59,30 @@ export class Generator { } const fileMatcher = globListToMatcher(group.resources.files || []); - const allFiles = await this.fs.list('/'); - const matchedFiles = allFiles.filter(fileMatcher).filter(file => !seenMap.has(file)).sort(); + matchedFiles.forEach(file => seenMap.add(file)); + filesPerGroup.set(group, matchedFiles); + } - // Add the hashes. - await matchedFiles.reduce(async (previous, file) => { - await previous; - const hash = await this.fs.hash(file); - hashTable[joinUrls(this.baseHref, file)] = hash; - }, Promise.resolve()); + // Compute hashes for all matched files and add them to the hash-table. + const allMatchedFiles = ([] as string[]).concat(...Array.from(filesPerGroup.values())).sort(); + const allMatchedHashes = await Promise.all(allMatchedFiles.map(file => this.fs.hash(file))); + allMatchedFiles.forEach((file, idx) => { + hashTable[joinUrls(this.baseHref, file)] = allMatchedHashes[idx]; + }); - return { - name: group.name, - installMode: group.installMode || 'prefetch', - updateMode: group.updateMode || group.installMode || 'prefetch', - cacheQueryOptions: buildCacheQueryOptions(group.cacheQueryOptions), - urls: matchedFiles.map(url => joinUrls(this.baseHref, url)), - patterns: (group.resources.urls || []).map(url => urlToRegex(url, this.baseHref, true)), - }; - })); + // Generate and return the processed asset-groups. + return Array.from(filesPerGroup.entries()) + .map(([group, matchedFiles]) => ({ + name: group.name, + installMode: group.installMode || 'prefetch', + updateMode: group.updateMode || group.installMode || 'prefetch', + cacheQueryOptions: buildCacheQueryOptions(group.cacheQueryOptions), + urls: matchedFiles.map(url => joinUrls(this.baseHref, url)), + patterns: + (group.resources.urls || []).map(url => urlToRegex(url, this.baseHref, true)), + })); } private processDataGroups(config: Config): Object[] { diff --git a/packages/service-worker/config/test/generator_spec.ts b/packages/service-worker/config/test/generator_spec.ts index f94a2125d32..085b64d794e 100644 --- a/packages/service-worker/config/test/generator_spec.ts +++ b/packages/service-worker/config/test/generator_spec.ts @@ -128,6 +128,102 @@ describe('Generator', () => { }); }); + it('assigns files to the first matching asset-group (unaffected by file-system access delays)', + async () => { + const fs = new MockFilesystem({ + '/index.html': 'This is a test', + '/foo/script-1.js': 'This is script 1', + '/foo/script-2.js': 'This is script 2', + '/bar/script-3.js': 'This is script 3', + '/bar/script-4.js': 'This is script 4', + '/qux/script-5.js': 'This is script 5', + }); + + // Simulate fluctuating file-system access delays. + const allFiles = await fs.list('/'); + spyOn(fs, 'list') + .and.returnValues( + new Promise(resolve => setTimeout(resolve, 2000, allFiles.slice())), + new Promise(resolve => setTimeout(resolve, 3000, allFiles.slice())), + new Promise(resolve => setTimeout(resolve, 1000, allFiles.slice())), + ); + + const gen = new Generator(fs, ''); + const config = await gen.process({ + index: '/index.html', + assetGroups: [ + { + name: 'group-foo', + resources: {files: ['/foo/**/*.js']}, + }, + { + name: 'group-bar', + resources: {files: ['/bar/**/*.js']}, + }, + { + name: 'group-fallback', + resources: {files: ['/**/*.js']}, + }, + ], + }); + + expect(config).toEqual({ + configVersion: 1, + timestamp: 1234567890123, + appData: undefined, + index: '/index.html', + assetGroups: [ + { + name: 'group-foo', + installMode: 'prefetch', + updateMode: 'prefetch', + cacheQueryOptions: {ignoreVary: true}, + urls: [ + '/foo/script-1.js', + '/foo/script-2.js', + ], + patterns: [], + }, + { + name: 'group-bar', + installMode: 'prefetch', + updateMode: 'prefetch', + cacheQueryOptions: {ignoreVary: true}, + urls: [ + '/bar/script-3.js', + '/bar/script-4.js', + ], + patterns: [], + }, + { + name: 'group-fallback', + installMode: 'prefetch', + updateMode: 'prefetch', + cacheQueryOptions: {ignoreVary: true}, + urls: [ + '/qux/script-5.js', + ], + patterns: [], + }, + ], + dataGroups: [], + hashTable: { + '/bar/script-3.js': 'bc0a9b488b5707757c491ddac66f56304310b6b1', + '/bar/script-4.js': 'b7782e97a285f1f6e62feca842384babaa209040', + '/foo/script-1.js': '3cf257d7ef7e991898f8506fd408cab4f0c2de91', + '/foo/script-2.js': '9de2ba54065bb9d610bce51beec62e35bea870a7', + '/qux/script-5.js': '3dceafdc0a1b429718e45fbf8e3005dd767892de' + }, + navigationUrls: [ + {positive: true, regex: '^\\/.*$'}, + {positive: false, regex: '^\\/(?:.+\\/)?[^/]*\\.[^/]*$'}, + {positive: false, regex: '^\\/(?:.+\\/)?[^/]*__[^/]*$'}, + {positive: false, regex: '^\\/(?:.+\\/)?[^/]*__[^/]*\\/.*$'}, + ], + navigationRequestStrategy: 'performance', + }); + }); + it('uses default `navigationUrls` if not provided', async () => { const fs = new MockFilesystem({ '/index.html': 'This is a test',