From c03fb357eabdde5fb53cd0a65006cd4b00cfd0cb Mon Sep 17 00:00:00 2001 From: Jeff Posnick Date: Thu, 20 Jul 2017 14:18:51 -0400 Subject: [PATCH 1/4] Fixes a bug with cacheFirst handler + maxAgeSeconds --- .../src/lib/cache-expiration-plugin.js | 9 +++--- .../src/lib/cache-expiration.js | 11 ++++--- .../test/browser/end-to-end.js | 30 +++++++++++++++++++ .../static/cache-first-max-age-seconds.js | 23 ++++++++++++++ .../src/lib/request-wrapper.js | 6 ++-- utils/server-instance.js | 8 +++++ 6 files changed, 77 insertions(+), 10 deletions(-) create mode 100644 packages/workbox-cache-expiration/test/static/cache-first-max-age-seconds.js diff --git a/packages/workbox-cache-expiration/src/lib/cache-expiration-plugin.js b/packages/workbox-cache-expiration/src/lib/cache-expiration-plugin.js index 67d59093b..ef0df01c9 100644 --- a/packages/workbox-cache-expiration/src/lib/cache-expiration-plugin.js +++ b/packages/workbox-cache-expiration/src/lib/cache-expiration-plugin.js @@ -47,14 +47,15 @@ class CacheExpirationPlugin extends CacheExpiration { * * @private * @param {Object} input + * @param {string} input.cacheName Name of the cache the responses belong to. * @param {Response} input.cachedResponse The `Response` object that's been * read from a cache and whose freshness should be checked. * @param {Number} [input.now] A timestamp. Defaults to the current time. - * @return {Response|null} Either the `cachedResponse`, if it's fresh, or - * `null` if the `Response` is older than `maxAgeSeconds`. + * @return {Promise} Either the `cachedResponse`, if it's + * fresh, or `null` if the `Response` is older than `maxAgeSeconds`. */ - cacheWillMatch({cachedResponse, now} = {}) { - if (this.isResponseFresh({cachedResponse, now})) { + async cacheWillMatch({cacheName, cachedResponse, now} = {}) { + if (await this.isResponseFresh({cacheName, cachedResponse, now})) { return cachedResponse; } diff --git a/packages/workbox-cache-expiration/src/lib/cache-expiration.js b/packages/workbox-cache-expiration/src/lib/cache-expiration.js index 0a5901e22..a4835fc0a 100644 --- a/packages/workbox-cache-expiration/src/lib/cache-expiration.js +++ b/packages/workbox-cache-expiration/src/lib/cache-expiration.js @@ -131,21 +131,22 @@ class CacheExpiration { * be used. * * @param {Object} input + * @param {string} input.cacheName Name of the cache the responses belong to. * @param {Response} input.cachedResponse The `Response` object that's been * read from a cache and whose freshness should be checked. * @param {Number} [input.now] A timestamp. * * Defaults to the current time. - * @return {boolean} Either `true` if the response is fresh, or `false` if the - * `Response` is older than `maxAgeSeconds` and should no longer be - * used. + * @return {Promise} Either `true` if the response is fresh, or + * `false` if the `Response` is older than `maxAgeSeconds` and should no + * longer be used. * * @example * expirationPlugin.isResponseFresh({ * cachedResponse: responseFromCache * }); */ - isResponseFresh({cachedResponse, now} = {}) { + async isResponseFresh({cacheName, cachedResponse, now} = {}) { // Only bother checking for freshness if we have a valid response and if // maxAgeSeconds is set. Otherwise, skip the check and always return true. if (cachedResponse && this.maxAgeSeconds) { @@ -165,6 +166,8 @@ class CacheExpiration { // Only return false if all the conditions are met. return false; } + } else { + await this.expireEntries({cacheName, now}); } } diff --git a/packages/workbox-cache-expiration/test/browser/end-to-end.js b/packages/workbox-cache-expiration/test/browser/end-to-end.js index c3ccac189..4628b96d7 100644 --- a/packages/workbox-cache-expiration/test/browser/end-to-end.js +++ b/packages/workbox-cache-expiration/test/browser/end-to-end.js @@ -37,4 +37,34 @@ describe(`End to End Test of Cache Expiration`, function() { expect(keys).to.have.length(EXPECTED_CACHE_SIZE); }); + + it(`should work properly when a cache-first strategy + maxAgeSeconds is used, and responses lack a Date header`, async function() { + const iframe = await goog.swUtils.controlledBySW( + '/packages/workbox-cache-expiration/test/static/cache-first-max-age-seconds.js'); + + const currentUrl = new URL(location); + const nextPort = parseInt(currentUrl.port) + 1; + const corsOrigin = `${currentUrl.protocol}//${currentUrl.hostname}:${nextPort}`; + const testUrl = new URL('/__echo/cors-no-cache/test', corsOrigin); + + const firstResponse = await iframe.contentWindow.fetch(testUrl); + const firstResponseBody = await firstResponse.text(); + + // The service worker should be using a maxAgeSeconds of 1, so skip ahead + // 1.1 seconds. We can't use sinon's fake clock here, since we need a + // consistent clock between the iframe's scope and the service worker scope. + await new Promise((resolve) => setTimeout(resolve, 1100)); + const secondResponse = await iframe.contentWindow.fetch(testUrl); + const secondResponseBody = await secondResponse.text(); + // Since we're using a cache-first policy, the expiration happens after the + // previous entry had already been read from cache. So the first and second + // responses should be the same. + expect(firstResponseBody).to.eql(secondResponseBody); + + const thirdResponse = await iframe.contentWindow.fetch(testUrl); + const thirdResponseBody = await thirdResponse.text(); + // By the time the third response is read, the cache expiration will have + // completed, so this one should be different. + expect(firstResponseBody).not.to.eql(thirdResponseBody); + }); }); diff --git a/packages/workbox-cache-expiration/test/static/cache-first-max-age-seconds.js b/packages/workbox-cache-expiration/test/static/cache-first-max-age-seconds.js new file mode 100644 index 000000000..707e9cbad --- /dev/null +++ b/packages/workbox-cache-expiration/test/static/cache-first-max-age-seconds.js @@ -0,0 +1,23 @@ +importScripts('/__test/bundle/workbox-runtime-caching'); +importScripts('/__test/bundle/workbox-cache-expiration'); + +self.addEventListener('install', (event) => event.waitUntil(self.skipWaiting())); +self.addEventListener('activate', (event) => event.waitUntil(self.clients.claim())); + +const cacheExpirationPlugin= new workbox.cacheExpiration.CacheExpirationPlugin({ + maxAgeSeconds: 1, +}); +const requestWrapper = new workbox.runtimeCaching.RequestWrapper({ + cacheName: 'cache-first-max-age-seconds', + plugins: [cacheExpirationPlugin], +}); +const handler = new workbox.runtimeCaching.CacheFirst({ + requestWrapper, + waitOnCache: true, +}); + +self.addEventListener('fetch', (event) => { + if (event.request.url.includes('/__echo/cors-no-cache')) { + event.respondWith(handler.handle({event})); + } +}); diff --git a/packages/workbox-runtime-caching/src/lib/request-wrapper.js b/packages/workbox-runtime-caching/src/lib/request-wrapper.js index db98b4b6a..26e081978 100644 --- a/packages/workbox-runtime-caching/src/lib/request-wrapper.js +++ b/packages/workbox-runtime-caching/src/lib/request-wrapper.js @@ -175,8 +175,10 @@ class RequestWrapper { if (this.plugins.has('cacheWillMatch')) { const plugin = this.plugins.get('cacheWillMatch')[0]; - cachedResponse = plugin.cacheWillMatch({request, cache, cachedResponse, - matchOptions: this.matchOptions}); + cachedResponse = await plugin.cacheWillMatch({ + request, cache, cachedResponse, + matchOptions: this.matchOptions, cacheName: this.cacheName, + }); } return cachedResponse; diff --git a/utils/server-instance.js b/utils/server-instance.js index caf893d5e..83afca7bc 100644 --- a/utils/server-instance.js +++ b/utils/server-instance.js @@ -54,6 +54,14 @@ class ServerInstance { res.send(`${req.params.file}-${Date.now()}`); }); + this._app.all('/__echo/cors-no-cache/:file', function(req, res) { + res.setHeader('Access-Control-Allow-Methods', + 'POST, GET, PUT, DELETE, OPTIONS'); + res.setHeader('Cache-Control', 'no-cache'); + res.setHeader('Access-Control-Allow-Origin', '*'); + res.send(`${req.params.file}-${Date.now()}`); + }); + this._app.all('/__echo/date-with-cors/:file', function(req, res) { res.setHeader('Access-Control-Allow-Methods', 'POST, GET, PUT, DELETE, OPTIONS'); From 6d713323c1dcd27f72f9e465670faf42d84d1c67 Mon Sep 17 00:00:00 2001 From: Jeff Posnick Date: Thu, 20 Jul 2017 14:49:34 -0400 Subject: [PATCH 2/4] Keeps the original method signatures for the cacheWillMatch callback. --- .../src/lib/cache-expiration-plugin.js | 6 +++--- .../src/lib/cache-expiration.js | 6 +++--- .../test/browser/end-to-end.js | 14 ++++++++++++-- .../src/lib/request-wrapper.js | 2 +- 4 files changed, 19 insertions(+), 9 deletions(-) diff --git a/packages/workbox-cache-expiration/src/lib/cache-expiration-plugin.js b/packages/workbox-cache-expiration/src/lib/cache-expiration-plugin.js index ef0df01c9..d94f063da 100644 --- a/packages/workbox-cache-expiration/src/lib/cache-expiration-plugin.js +++ b/packages/workbox-cache-expiration/src/lib/cache-expiration-plugin.js @@ -51,11 +51,11 @@ class CacheExpirationPlugin extends CacheExpiration { * @param {Response} input.cachedResponse The `Response` object that's been * read from a cache and whose freshness should be checked. * @param {Number} [input.now] A timestamp. Defaults to the current time. - * @return {Promise} Either the `cachedResponse`, if it's + * @return {Response} Either the `cachedResponse`, if it's * fresh, or `null` if the `Response` is older than `maxAgeSeconds`. */ - async cacheWillMatch({cacheName, cachedResponse, now} = {}) { - if (await this.isResponseFresh({cacheName, cachedResponse, now})) { + cacheWillMatch({cacheName, cachedResponse, now} = {}) { + if (this.isResponseFresh({cacheName, cachedResponse, now})) { return cachedResponse; } diff --git a/packages/workbox-cache-expiration/src/lib/cache-expiration.js b/packages/workbox-cache-expiration/src/lib/cache-expiration.js index a4835fc0a..29eb6affe 100644 --- a/packages/workbox-cache-expiration/src/lib/cache-expiration.js +++ b/packages/workbox-cache-expiration/src/lib/cache-expiration.js @@ -137,7 +137,7 @@ class CacheExpiration { * @param {Number} [input.now] A timestamp. * * Defaults to the current time. - * @return {Promise} Either `true` if the response is fresh, or + * @return {boolean} Either `true` if the response is fresh, or * `false` if the `Response` is older than `maxAgeSeconds` and should no * longer be used. * @@ -146,7 +146,7 @@ class CacheExpiration { * cachedResponse: responseFromCache * }); */ - async isResponseFresh({cacheName, cachedResponse, now} = {}) { + isResponseFresh({cacheName, cachedResponse, now} = {}) { // Only bother checking for freshness if we have a valid response and if // maxAgeSeconds is set. Otherwise, skip the check and always return true. if (cachedResponse && this.maxAgeSeconds) { @@ -167,7 +167,7 @@ class CacheExpiration { return false; } } else { - await this.expireEntries({cacheName, now}); + this.expireEntries({cacheName, now}); } } diff --git a/packages/workbox-cache-expiration/test/browser/end-to-end.js b/packages/workbox-cache-expiration/test/browser/end-to-end.js index 4628b96d7..37e4b8a7a 100644 --- a/packages/workbox-cache-expiration/test/browser/end-to-end.js +++ b/packages/workbox-cache-expiration/test/browser/end-to-end.js @@ -6,6 +6,10 @@ describe(`End to End Test of Cache Expiration`, function() { beforeEach(() => goog.swUtils.cleanState()); + async function asyncDelay(seconds) { + await new Promise((resolve) => setTimeout(resolve, seconds * 1000)); + } + it(`should work properly when there are many simultaneous requests`, async function() { const iframe = await goog.swUtils.controlledBySW( '/packages/workbox-cache-expiration/test/static/cache-expiration.js'); @@ -39,6 +43,9 @@ describe(`End to End Test of Cache Expiration`, function() { }); it(`should work properly when a cache-first strategy + maxAgeSeconds is used, and responses lack a Date header`, async function() { + // See the comment later about a source of potential flakiness. + this.retries(2); + const iframe = await goog.swUtils.controlledBySW( '/packages/workbox-cache-expiration/test/static/cache-first-max-age-seconds.js'); @@ -51,9 +58,9 @@ describe(`End to End Test of Cache Expiration`, function() { const firstResponseBody = await firstResponse.text(); // The service worker should be using a maxAgeSeconds of 1, so skip ahead - // 1.1 seconds. We can't use sinon's fake clock here, since we need a + // 1.5 seconds. We can't use sinon's fake clock here, since we need a // consistent clock between the iframe's scope and the service worker scope. - await new Promise((resolve) => setTimeout(resolve, 1100)); + await asyncDelay(1.5); const secondResponse = await iframe.contentWindow.fetch(testUrl); const secondResponseBody = await secondResponse.text(); // Since we're using a cache-first policy, the expiration happens after the @@ -61,6 +68,9 @@ describe(`End to End Test of Cache Expiration`, function() { // responses should be the same. expect(firstResponseBody).to.eql(secondResponseBody); + // POTENTIALLY FLAKY: Wait another 2 seconds, after which point the cache + // expiration logic will have hopefully completed. + await asyncDelay(2); const thirdResponse = await iframe.contentWindow.fetch(testUrl); const thirdResponseBody = await thirdResponse.text(); // By the time the third response is read, the cache expiration will have diff --git a/packages/workbox-runtime-caching/src/lib/request-wrapper.js b/packages/workbox-runtime-caching/src/lib/request-wrapper.js index 26e081978..5edffe14d 100644 --- a/packages/workbox-runtime-caching/src/lib/request-wrapper.js +++ b/packages/workbox-runtime-caching/src/lib/request-wrapper.js @@ -175,7 +175,7 @@ class RequestWrapper { if (this.plugins.has('cacheWillMatch')) { const plugin = this.plugins.get('cacheWillMatch')[0]; - cachedResponse = await plugin.cacheWillMatch({ + cachedResponse = plugin.cacheWillMatch({ request, cache, cachedResponse, matchOptions: this.matchOptions, cacheName: this.cacheName, }); From 6c65eb809ee632b16d3c8035718fef35fb63b5a4 Mon Sep 17 00:00:00 2001 From: Jeff Posnick Date: Fri, 21 Jul 2017 15:21:37 -0400 Subject: [PATCH 3/4] Clarifications(?) via comments and restructured return statements. --- .../src/lib/cache-expiration.js | 30 ++++++++++++++----- 1 file changed, 23 insertions(+), 7 deletions(-) diff --git a/packages/workbox-cache-expiration/src/lib/cache-expiration.js b/packages/workbox-cache-expiration/src/lib/cache-expiration.js index 29eb6affe..50f6953b8 100644 --- a/packages/workbox-cache-expiration/src/lib/cache-expiration.js +++ b/packages/workbox-cache-expiration/src/lib/cache-expiration.js @@ -126,6 +126,8 @@ class CacheExpiration { * `Date` header and the `maxAgeSeconds` parameter passed into the * constructor. * + * The general approach is to default to fresh unless proven otherwise. + * * If `maxAgeSeconds` or the `Date` header is not set then it will * default to returning `true`, i.e. the response is still fresh and should * be used. @@ -148,7 +150,7 @@ class CacheExpiration { */ isResponseFresh({cacheName, cachedResponse, now} = {}) { // Only bother checking for freshness if we have a valid response and if - // maxAgeSeconds is set. Otherwise, skip the check and always return true. + // maxAgeSeconds is set. if (cachedResponse && this.maxAgeSeconds) { isInstance({cachedResponse}, Response); @@ -160,17 +162,31 @@ class CacheExpiration { const parsedDate = new Date(dateHeader); // If the Date header was invalid for some reason, parsedDate.getTime() - // will return NaN, and the comparison will always be false. That means - // that an invalid date will be treated as if the response is fresh. - if ((parsedDate.getTime() + (this.maxAgeSeconds * 1000)) < now) { - // Only return false if all the conditions are met. - return false; - } + // will return NaN, and the comparison will always be false. We want + // invalid dates to be treated as fresh. In order to ensure this + // behavior, we need to return the negation of this comparison. + return !((parsedDate.getTime() + (this.maxAgeSeconds * 1000)) < now); } else { + // TODO (jeffposnick): Change this method interface to be async, and + // check for the IDB for the specific URL in order to determine + // freshness when Date is not available. + + // If there is no Date header (i.e. if this is a cross-origin response), + // then we don't know for sure whether the response is fresh or not. + // One thing we can do is trigger cache expiration, which will clean up + // any old responses based on IDB timestamps, and ensure that when a + // cache-first handler is used, stale responses will eventually be + // replaced (though not until the *next* request is made). + // See https://github.com/GoogleChrome/workbox/issues/691 this.expireEntries({cacheName, now}); + // Return true, since otherwise a cross-origin cached response without + // a Date header would *never* be considered valid. + return true; } } + // If either cachedResponse or maxAgeSeconds wasn't set, then the response + // is "trivially" fresh, so return true. return true; } From b758a556cf3f6bbcf11f06dbc907cd0c9b28dc06 Mon Sep 17 00:00:00 2001 From: Jeff Posnick Date: Tue, 25 Jul 2017 15:54:33 -0400 Subject: [PATCH 4/4] Review feedback. --- .../src/lib/cache-expiration.js | 14 ++++++++++---- .../test/browser/end-to-end.js | 1 + 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/packages/workbox-cache-expiration/src/lib/cache-expiration.js b/packages/workbox-cache-expiration/src/lib/cache-expiration.js index 50f6953b8..490e2b93e 100644 --- a/packages/workbox-cache-expiration/src/lib/cache-expiration.js +++ b/packages/workbox-cache-expiration/src/lib/cache-expiration.js @@ -161,11 +161,17 @@ class CacheExpiration { } const parsedDate = new Date(dateHeader); + const headerTime = parsedDate.getTime(); // If the Date header was invalid for some reason, parsedDate.getTime() - // will return NaN, and the comparison will always be false. We want - // invalid dates to be treated as fresh. In order to ensure this - // behavior, we need to return the negation of this comparison. - return !((parsedDate.getTime() + (this.maxAgeSeconds * 1000)) < now); + // will return NaN. We want to treat that as a fresh response, since we + // assume fresh unless proven otherwise. + if (isNaN(headerTime)) { + return true; + } + + // If we have a valid headerTime, then our response is fresh iff the + // headerTime plus maxAgeSeconds is greater than the current time. + return (headerTime + (this.maxAgeSeconds * 1000)) > now; } else { // TODO (jeffposnick): Change this method interface to be async, and // check for the IDB for the specific URL in order to determine diff --git a/packages/workbox-cache-expiration/test/browser/end-to-end.js b/packages/workbox-cache-expiration/test/browser/end-to-end.js index 37e4b8a7a..77eb823b6 100644 --- a/packages/workbox-cache-expiration/test/browser/end-to-end.js +++ b/packages/workbox-cache-expiration/test/browser/end-to-end.js @@ -45,6 +45,7 @@ describe(`End to End Test of Cache Expiration`, function() { it(`should work properly when a cache-first strategy + maxAgeSeconds is used, and responses lack a Date header`, async function() { // See the comment later about a source of potential flakiness. this.retries(2); + this.timeout(6 * 1000); const iframe = await goog.swUtils.controlledBySW( '/packages/workbox-cache-expiration/test/static/cache-first-max-age-seconds.js');