Skip to content

fix(service-worker): cache opaque responses in data groups with freshness strategy #30977

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 9 commits into from
Closed

fix(service-worker): cache opaque responses in data groups with freshness strategy #30977

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
cb71a13
test(service-worker): remove obsolete async test helpers
gkalpak Jun 24, 2019
a725e1d
test(service-worker): simplify test helpers
gkalpak Jun 24, 2019
60c406b
test(service-worker): properly reset mock server state for each test
gkalpak Jun 24, 2019
b54d0a9
test(service-worker): better simulate opaque requests
gkalpak Jun 24, 2019
1fc2e3b
refactor(service-worker): make the caching behavior more explicit
gkalpak Jun 24, 2019
ea9f116
fix(service-worker): cache opaque responses in data groups with `fres…
gkalpak Jun 24, 2019
be5109a
fix(service-worker): cache opaque responses when requests exceeds tim…
gkalpak Jun 24, 2019
9f10975
refactor(service-worker): remove redundant cache operation
gkalpak Jun 24, 2019
b8b61c7
fixup! test(service-worker): better simulate opaque requests
gkalpak Jun 24, 2019
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
28 changes: 0 additions & 28 deletions packages/service-worker/test/async.ts
View file
Open in desktop

This file was deleted.

24 changes: 11 additions & 13 deletions packages/service-worker/test/comm_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,6 @@ import {SwPush} from '@angular/service-worker/src/push';
import {SwUpdate} from '@angular/service-worker/src/update';
import {MockPushManager, MockPushSubscription, MockServiceWorkerContainer, MockServiceWorkerRegistration, patchDecodeBase64} from '@angular/service-worker/testing/mock';

import {async_fit, async_it} from './async';

{
describe('ServiceWorker library', () => {
let mock: MockServiceWorkerContainer;
Expand Down Expand Up @@ -160,15 +158,15 @@ import {async_fit, async_it} from './async';
});

describe('requestSubscription()', () => {
async_it('returns a promise that resolves to the subscription', async() => {
it('returns a promise that resolves to the subscription', async() => {
const promise = push.requestSubscription({serverPublicKey: 'test'});
expect(promise).toEqual(jasmine.any(Promise));

const sub = await promise;
expect(sub).toEqual(jasmine.any(MockPushSubscription));
});

async_it('calls `PushManager.subscribe()` (with appropriate options)', async() => {
it('calls `PushManager.subscribe()` (with appropriate options)', async() => {
const decode = (charCodeArr: Uint8Array) =>
Array.from(charCodeArr).map(c => String.fromCharCode(c)).join('');

Expand All @@ -190,7 +188,7 @@ import {async_fit, async_it} from './async';
expect(actualAppServerKeyStr).toBe(appServerKeyStr);
});

async_it('emits the new `PushSubscription` on `SwPush.subscription`', async() => {
it('emits the new `PushSubscription` on `SwPush.subscription`', async() => {
const subscriptionSpy = jasmine.createSpy('subscriptionSpy');
push.subscription.subscribe(subscriptionSpy);
const sub = await push.requestSubscription({serverPublicKey: 'test'});
Expand All @@ -206,7 +204,7 @@ import {async_fit, async_it} from './async';
psUnsubscribeSpy = spyOn(MockPushSubscription.prototype, 'unsubscribe').and.callThrough();
});

async_it('rejects if currently not subscribed to push notifications', async() => {
it('rejects if currently not subscribed to push notifications', async() => {
try {
await push.unsubscribe();
throw new Error('`unsubscribe()` should fail');
Expand All @@ -215,14 +213,14 @@ import {async_fit, async_it} from './async';
}
});

async_it('calls `PushSubscription.unsubscribe()`', async() => {
it('calls `PushSubscription.unsubscribe()`', async() => {
await push.requestSubscription({serverPublicKey: 'test'});
await push.unsubscribe();

expect(psUnsubscribeSpy).toHaveBeenCalledTimes(1);
});

async_it('rejects if `PushSubscription.unsubscribe()` fails', async() => {
it('rejects if `PushSubscription.unsubscribe()` fails', async() => {
psUnsubscribeSpy.and.callFake(() => { throw new Error('foo'); });

try {
Expand All @@ -234,7 +232,7 @@ import {async_fit, async_it} from './async';
}
});

async_it('rejects if `PushSubscription.unsubscribe()` returns false', async() => {
it('rejects if `PushSubscription.unsubscribe()` returns false', async() => {
psUnsubscribeSpy.and.returnValue(Promise.resolve(false));

try {
Expand All @@ -246,7 +244,7 @@ import {async_fit, async_it} from './async';
}
});

async_it('emits `null` on `SwPush.subscription`', async() => {
it('emits `null` on `SwPush.subscription`', async() => {
const subscriptionSpy = jasmine.createSpy('subscriptionSpy');
push.subscription.subscribe(subscriptionSpy);

Expand All @@ -256,7 +254,7 @@ import {async_fit, async_it} from './async';
expect(subscriptionSpy).toHaveBeenCalledWith(null);
});

async_it('does not emit on `SwPush.subscription` on failure', async() => {
it('does not emit on `SwPush.subscription` on failure', async() => {
const subscriptionSpy = jasmine.createSpy('subscriptionSpy');
const initialSubEmit = new Promise(resolve => subscriptionSpy.and.callFake(resolve));

Expand Down Expand Up @@ -340,7 +338,7 @@ import {async_fit, async_it} from './async';
push.subscription.subscribe(subscriptionSpy);
});

async_it('emits on worker-driven changes (i.e. when the controller changes)', async() => {
it('emits on worker-driven changes (i.e. when the controller changes)', async() => {
// Initial emit for the current `ServiceWorkerController`.
await nextSubEmitPromise;
expect(subscriptionSpy).toHaveBeenCalledTimes(1);
Expand All @@ -355,7 +353,7 @@ import {async_fit, async_it} from './async';
expect(subscriptionSpy).toHaveBeenCalledWith(null);
});

async_it('emits on subscription changes (i.e. when subscribing/unsubscribing)', async() => {
it('emits on subscription changes (i.e. when subscribing/unsubscribing)', async() => {
await nextSubEmitPromise;
subscriptionSpy.calls.reset();

Expand Down
14 changes: 6 additions & 8 deletions packages/service-worker/test/integration_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,6 @@ import {SwTestHarness, SwTestHarnessBuilder} from '@angular/service-worker/worke
import {Observable} from 'rxjs';
import {take} from 'rxjs/operators';

import {async_beforeEach, async_fit, async_it} from './async';

(function() {
// Skip environments that don't support the minimum APIs needed to run the SW tests.
if (!SwTestHarness.envIsSupported()) {
Expand Down Expand Up @@ -80,7 +78,7 @@ import {async_beforeEach, async_fit, async_it} from './async';
let scope: SwTestHarness;
let driver: Driver;

async_beforeEach(async() => {
beforeEach(async() => {
// Fire up the client.
mock = new MockServiceWorkerContainer();
comm = new NgswCommChannel(mock as any);
Expand All @@ -100,12 +98,12 @@ import {async_beforeEach, async_fit, async_it} from './async';
await driver.initialized;
});

async_it('communicates back and forth via update check', async() => {
it('communicates back and forth via update check', async() => {
const update = new SwUpdate(comm);
await update.checkForUpdate();
});

async_it('detects an actual update', async() => {
it('detects an actual update', async() => {
const update = new SwUpdate(comm);
scope.updateServerState(serverUpdate);

Expand All @@ -116,7 +114,7 @@ import {async_beforeEach, async_fit, async_it} from './async';
await gotUpdateNotice;
});

async_it('receives push message notifications', async() => {
it('receives push message notifications', async() => {
const push = new SwPush(comm);
scope.updateServerState(serverUpdate);

Expand All @@ -133,7 +131,7 @@ import {async_beforeEach, async_fit, async_it} from './async';
await gotPushNotice;
});

async_it('receives push message click events', async() => {
it('receives push message click events', async() => {
const push = new SwPush(comm);
scope.updateServerState(serverUpdate);

Expand All @@ -147,4 +145,4 @@ import {async_beforeEach, async_fit, async_it} from './async';
await gotNotificationClick;
});
});
})();
})();
22 changes: 11 additions & 11 deletions packages/service-worker/worker/src/data.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -354,10 +354,11 @@ export class DataGroup {

// Cache the network response eventually.
ctx.waitUntil(this.safeCacheResponse(req, networkFetch));
} else {
// The request completed in time, so cache it inline with the response flow.
await this.cacheResponse(req, res, lru);
}

// The request completed in time, so cache it inline with the response flow.
await this.cacheResponse(req, res, lru);
return res;
}

Expand All @@ -377,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.
Expand All @@ -394,9 +395,7 @@ export class DataGroup {
}

// No response in the cache. No choice but to fall back on the full network fetch.
res = await networkFetch;
await this.cacheResponse(req, res, lru, true);
return res;
return networkFetch;
}

private networkFetchWithTimeout(req: Request): [Promise<Response|undefined>, Promise<Response>] {
Expand Down Expand Up @@ -433,9 +432,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?
}
Expand Down Expand Up @@ -480,10 +480,10 @@ export class DataGroup {
* If the request times out on the server, an error will be returned but the real network
* request will still be running in the background, to be cached when it completes.
*/
private async cacheResponse(
req: Request, res: Response, lru: LruList, okToCacheOpaque: boolean = false): Promise<void> {
private async cacheResponse(req: Request, res: Response, lru: LruList, okToCacheOpaque = false):
Promise<void> {
// Only cache successful responses.
if (!res.ok || (okToCacheOpaque && res.type === 'opaque')) {
if (!(res.ok || (okToCacheOpaque && res.type === 'opaque'))) {

@IgorMinar IgorMinar Jun 24, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Isn't this going to cause non-200 opaque responses to be cached?

@gkalpak gkalpak Jun 24, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes (if okToCacheOpaque is true, which is only the case when handling a DataGroup request with freshness (i.e. network first)).

With opaque responses, one can't tell if it was successful or not (res.status is 0, thus res.ok will always be false). So, what we do is that we cache opaque responses for DataGroup requests that are configured to fetch from the network first. So, if the client is online, the SW should fetch the response from the network anyway (and not use the cache), but if the client is offline, the SW will serve the cached response. There is little risk (other than the extra memory), because the request would have failed anyway when offline.

return;
}

Expand Down
28 changes: 0 additions & 28 deletions packages/service-worker/worker/test/async.ts
View file
Open in desktop

This file was deleted.

Loading