Skip to content

fix(service-worker): avoid network requests when looking up hashed resources in cache #24127

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
gkalpak wants to merge 2 commits into from
Closed

fix(service-worker): avoid network requests when looking up hashed resources in cache #24127

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
101015f
refactor(service-worker): avoid unnecessary operations and remove unu…
gkalpak May 25, 2018
3743f97
fix(service-worker): avoid network requests when looking up hashed re…
gkalpak May 25, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 3 additions & 9 deletions packages/service-worker/worker/src/app-version.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,8 +198,6 @@ export class AppVersion implements UpdateSource {
* Check this version for a given resource with a particular hash.
*/
async lookupResourceWithHash(url: string, hash: string): Promise<Response|null> {
const req = this.adapter.newRequest(url);

// Verify that this version has the requested resource cached. If not,
// there's no point in trying.
if (!this.hashTable.has(url)) {
Expand All @@ -208,16 +206,12 @@ export class AppVersion implements UpdateSource {

// Next, check whether the resource has the correct hash. If not, any cached
// response isn't usable.
if (this.hashTable.get(url) ! !== hash) {
if (this.hashTable.get(url) !== hash) {
return null;
}

// TODO: no-op context and appropriate contract. Currently this is a violation
// of the typings and could cause issues if handleFetch() has side effects. A
// better strategy to deal with side effects is needed.
// TODO: this could result in network fetches if the response is lazy. Refactor
// to avoid them.
return this.handleFetch(req, null !);
const cacheState = await this.lookupResourceWithoutHash(url);
return cacheState && cacheState.response;
}

/**
Expand Down
33 changes: 14 additions & 19 deletions packages/service-worker/worker/src/driver.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,16 +225,21 @@ export class Driver implements Debuggable, UpdateSource {
// Initialization is the only event which is sent directly from the SW to itself,
// and thus `event.source` is not a Client. Handle it here, before the check
// for Client sources.
if (data.action === 'INITIALIZE' && this.initialized === null) {
// Initialize the SW.
this.initialized = this.initialize();
if (data.action === 'INITIALIZE') {
// Only initialize if not already initialized (or initializing).
if (this.initialized === null) {
// Initialize the SW.
this.initialized = this.initialize();

// Wait until initialization is properly scheduled, then trigger idle
// events to allow it to complete (assuming the SW is idle).
event.waitUntil((async() => {
await this.initialized;
await this.idle.trigger();
})());
}

// Wait until initialization is properly scheduled, then trigger idle
// events to allow it to complete (assuming the SW is idle).
event.waitUntil((async() => {
await this.initialized;
await this.idle.trigger();
})());
return;
}

// Only messages from true clients are accepted past this point (this is essentially
Expand Down Expand Up @@ -725,16 +730,6 @@ 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);

// Try to determine a version that's safe to update from.

@alxhub alxhub Jul 3, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch.

let updateFrom: AppVersion|undefined = undefined;

// It's always safe to update from a version, even a broken one, as it will still
// only have valid resources cached. If there is no latest version, though, this
// update will have to install as a fresh version.
if (this.latestHash !== null) {
updateFrom = this.versions.get(this.latestHash);
}

// Firstly, check if the manifest version is correct.
if (manifest.configVersion !== SUPPORTED_CONFIG_VERSION) {
await this.deleteAllCaches();
Expand Down
55 changes: 51 additions & 4 deletions packages/service-worker/worker/test/happy_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,9 @@ const dist =
.addFile('/baz.txt', 'this is baz')
.addFile('/qux.txt', 'this is qux')
.addFile('/quux.txt', 'this is quux')
.addFile('/quuux.txt', 'this is quuux')
.addFile('/lazy/unchanged1.txt', 'this is unchanged (1)')
.addFile('/lazy/unchanged2.txt', 'this is unchanged (2)')
.addUnhashedFile('/unhashed/a.txt', 'this is unhashed', {'Cache-Control': 'max-age=10'})
.build();

Expand All @@ -34,6 +37,9 @@ const distUpdate =
.addFile('/baz.txt', 'this is baz v2')
.addFile('/qux.txt', 'this is qux v2')
.addFile('/quux.txt', 'this is quux v2')
.addFile('/quuux.txt', 'this is quuux v2')
.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('/ignored/file1', 'this is not handled by the SW')
.addUnhashedFile('/ignored/dir/file2', 'this is not handled by the SW either')
Expand Down Expand Up @@ -92,7 +98,12 @@ const manifest: Manifest = {
name: 'lazy_prefetch',
installMode: 'lazy',
updateMode: 'prefetch',
urls: ['/quux.txt'],
urls: [
'/quux.txt',
'/quuux.txt',
'/lazy/unchanged1.txt',
'/lazy/unchanged2.txt',
],
patterns: [],
}
],
Expand Down Expand Up @@ -134,7 +145,12 @@ const manifestUpdate: Manifest = {
name: 'lazy_prefetch',
installMode: 'lazy',
updateMode: 'prefetch',
urls: ['/quux.txt'],
urls: [
'/quux.txt',
'/quuux.txt',
'/lazy/unchanged1.txt',
'/lazy/unchanged2.txt',
],
patterns: [],
}
],
Expand Down Expand Up @@ -528,14 +544,45 @@ const manifestUpdateHash = sha1(JSON.stringify(manifestUpdate));
async_it('prefetches updates to lazy cache when set', async() => {
expect(await makeRequest(scope, '/foo.txt')).toEqual('this is foo');
await driver.initialized;

// Fetch some files from the `lazy_prefetch` asset group.
expect(await makeRequest(scope, '/quux.txt')).toEqual('this is quux');
expect(await makeRequest(scope, '/lazy/unchanged1.txt')).toEqual('this is unchanged (1)');

// Install update.
scope.updateServerState(serverUpdate);
expect(await driver.checkForUpdate()).toEqual(true);
expect(await driver.checkForUpdate()).toBe(true);

// Previously requested and changed: Fetch from network.
serverUpdate.assertSawRequestFor('/quux.txt');
// Never requested and changed: Don't fetch.
serverUpdate.assertNoRequestFor('/quuux.txt');
// Previously requested and unchanged: Fetch from cache.
serverUpdate.assertNoRequestFor('/lazy/unchanged1.txt');
// Never requested and unchanged: Don't fetch.
serverUpdate.assertNoRequestFor('/lazy/unchanged2.txt');

serverUpdate.clearRequests();

// Update client.
await driver.updateClient(await scope.clients.get('default'));
expect(await makeRequest(scope, '/quux.txt')).toEqual('this is quux v2');

// Already cached.
expect(await makeRequest(scope, '/quux.txt')).toBe('this is quux v2');
serverUpdate.assertNoOtherRequests();

// Not cached: Fetch from network.
expect(await makeRequest(scope, '/quuux.txt')).toBe('this is quuux v2');
serverUpdate.assertSawRequestFor('/quuux.txt');

// Already cached (copied from old cache).
expect(await makeRequest(scope, '/lazy/unchanged1.txt')).toBe('this is unchanged (1)');
serverUpdate.assertNoOtherRequests();

// Not cached: Fetch from network.
expect(await makeRequest(scope, '/lazy/unchanged2.txt')).toBe('this is unchanged (2)');
serverUpdate.assertSawRequestFor('/lazy/unchanged2.txt');

serverUpdate.assertNoOtherRequests();
});

Expand Down