Cache SW: Support Precent Diversions #7914

jridgewell · 2017-03-01T22:51:49Z

This introduces the concept of RTV environments and the /diversions endpoint to the Cache SW.

The Cache SW now understands the "production" environment, and arbitrary diversion environments. The production env is whatever the env of the SW's version (the RTV is part of the Sw's bundled code). Any other env is considered a diversion.

Diversions are treated specially inside the stale-while-revalidate logic. Firstly, a diversion will never be served in place of (a "stale" serve) a request to a production env. Only cached scripts in the production environment can be stale served for a production request. Secondly, any request for a diversion will always be served with that version (hopefully from cache). That is, diversion requests are never stale served an older version. This is because stale serving a 3 week old diversion is useless for our data collection.

Now for the /diversions endpoint. The SW will maintain a cached request to the endpoint, and the endpoint returns a JSON encoded array of diversion RTVs (note, they must not be production ENV). Whenever the SW "revalidates" (grabs a newer copy of a production script in the background), we will issue requests for the current diversions of the script as well. This allows us to eagerly cache our diversions, making sure they will be served as speedily as our normal production scripts (and we don't see any false-negative stats).

The /diversions endpoint will be live soon. 🤞

cramforce

Nice!

cramforce · 2017-03-02T00:09:21Z

src/service-worker/core.js

+
+  return fetchPromises[url] = cache.match(request)
+      .then(response => {
+        if (response && !expired(response)) {


This is only relevant for the diversions endpoint, right? Because rtv URLs practically never expire.

cramforce · 2017-03-02T00:10:12Z

src/service-worker/core.js

+
+          // Did we receive a invalid response?
+          if (!response.ok) {
+            throw new Error(`failed fetching ${url}`);


No more info here?

This is just a statusCode check. I'll add the statusCode it fails with.

cramforce · 2017-03-02T00:13:43Z

src/service-worker/core.js

+    date = headers.get('expires');
+  }
+
+  return Date.now() >= Number(new Date(date)) + age;


Scary :)

I'd consider adding a shortcut for "reasonably close to infinity".

Wouldn't this consider everything expired if the machine's clock is in the future?

I'd consider adding a shortcut for "reasonably close to infinity".

What do you mean?

Wouldn't this consider everything expired if the machine's clock is in the future?

Hmm, I can override the header on the Date header on our side, ensuring the response uses the user's clock time.

cramforce · 2017-03-02T00:15:27Z

src/service-worker/core.js

+    // Fetch all diversions of this file.
+    // This intentionally does not block the request resolution to speed
+    // things up.
+    diversions(cache).then(diversions => {


Does it make sense to debounce this with like setTimeout(…, 10000) to get out of the way of the critical path?

I thought about that, and thought maybe (slightly) delaying the initial requests to batch them together would be a better approach. It would help this case, and would allow us to be smarter about which cached version to serve.

But, this isn't in the critical path. Notice this line isn't returned, the line two under this statement is.

Not in the critical path, but I'm seeing us bandwidth constraint downloading JS a lot!

Say, you are on 2G and only get .5 MBit/s. Then it makes a big difference to use the bandwidth for concurrent non-critical downloads.

cramforce · 2017-03-02T00:17:53Z

test/functional/test-cache-sw-core.js

+    describe('with cache-control and date', () => {
+      describe('date in the past', () => {
+        beforeEach(() => {
+          response.headers.set('date', new Date(Date.now() - 1000)


Add test case with actual date copy pasted from a real SFFE request.

This is what SFFE generates.

// From SFFE Request // date:Thu, 02 Mar 2017 00:36:19 GMT // cache-control:public, max-age=31536000 new Date().toUTCString(); // => "Thu, 02 Mar 2017 00:37:11 GMT"

Always better not to generate stuff in tests. Makes failures more obvious.

Not sure I can do this one, since it will start failing in a year. Maybe just a comment?

Use a fake clock.

cramforce · 2017-03-02T00:18:26Z

test/functional/test-cache-sw-core.js

+      });
+    });
+
+    describe('with expires', () => {


Consider deleting this code. Our responses will always have max-age

dknecht · 2017-03-02T01:04:50Z

Anything changes we need to make to Cloudflare cache?

Returns a struct of all the data, instead of issuing multiple `#exec` calls to grab a particular sub group.

This includes fetching JS from cache or network, purging old versions, and a new expired response helper.

jridgewell · 2017-03-02T01:11:06Z

Anything changes we need to make to Cloudflare cache?

Not necessarily. If you plan on supporting 1% diversions of the AMP runtime, than you'll need to return a JSON array of the diversion's RTV. If not, then you don't have to do anything.

However, if you don't return a valid (200 status code) response with a cache time header, you'll end up getting a lot of /diversions requests. Just returning an empty JSON array response, with a 1 day cache control should save your servers the headache and speed up the SW.

cramforce · 2017-03-05T14:39:31Z

src/service-worker/core.js

+    // Fetch all diversions of this file.
+    // This intentionally does not block the request resolution to speed
+    // things up.
+    diversions(cache).then(diversions => {


Say, you are on 2G and only get .5 MBit/s. Then it makes a big difference to use the bandwidth for concurrent non-critical downloads.

cramforce · 2017-03-05T14:39:40Z

test/functional/test-cache-sw-core.js

+    describe('with cache-control and date', () => {
+      describe('date in the past', () => {
+        beforeEach(() => {
+          response.headers.set('date', new Date(Date.now() - 1000)


Use a fake clock.

They must each get there own clone of the response.

jridgewell · 2017-03-09T22:56:14Z

Ping @cramforce

jridgewell · 2017-03-16T19:02:54Z

It's live!

* Reduce Regex#exec calls Returns a struct of all the data, instead of issuing multiple `#exec` calls to grab a particular sub group. * Split helpers into separate methods This includes fetching JS from cache or network, purging old versions, and a new expired response helper. * Support the /diversions endpoint * Linting * Update tests * No need for global regex * Remove expires header support * Ensure diversions are not prod env * Output statusCode in failed requests * Avoid server/client time skew * Fix tests * Fix bug with batching fetch requests They must each get there own clone of the response. * Fetch diversions after 10 seconds * Use exact date header strings from amp cache * lint

jridgewell requested a review from cramforce March 1, 2017 22:51

erwinmombay self-requested a review March 2, 2017 00:13

cramforce requested changes Mar 2, 2017

View reviewed changes

jridgewell added 10 commits March 1, 2017 17:05

Reduce Regex#exec calls

e74da71

Returns a struct of all the data, instead of issuing multiple `#exec` calls to grab a particular sub group.

Split helpers into separate methods

ff5bc3a

This includes fetching JS from cache or network, purging old versions, and a new expired response helper.

Support the /diversions endpoint

12af193

Linting

bfd2762

Update tests

3fbe6cb

No need for global regex

6331e65

Remove expires header support

e399974

Ensure diversions are not prod env

fda1951

Output statusCode in failed requests

670f9c4

Avoid server/client time skew

a227f4a

jridgewell force-pushed the cache-sw-diversions branch from 7521d41 to a227f4a Compare March 2, 2017 01:05

Fix tests

3e4aa51

erwinmombay removed their request for review March 3, 2017 01:29

cramforce requested changes Mar 5, 2017

View reviewed changes

jridgewell added 3 commits March 6, 2017 19:04

Fix bug with batching fetch requests

1aa7cf4

They must each get there own clone of the response.

Fetch diversions after 10 seconds

c8c9f7a

Use exact date header strings from amp cache

827711c

jridgewell force-pushed the cache-sw-diversions branch from 2414e65 to c93910c Compare March 7, 2017 02:49

lint

c93910c

cramforce approved these changes Mar 10, 2017

View reviewed changes

jridgewell merged commit c860a24 into ampproject:master Mar 16, 2017

jridgewell deleted the cache-sw-diversions branch March 16, 2017 19:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cache SW: Support Precent Diversions #7914

Cache SW: Support Precent Diversions #7914

jridgewell commented Mar 1, 2017

cramforce left a comment

cramforce Mar 2, 2017

jridgewell Mar 2, 2017

cramforce Mar 2, 2017

jridgewell Mar 2, 2017

cramforce Mar 2, 2017

jridgewell Mar 2, 2017

cramforce Mar 2, 2017

jridgewell Mar 2, 2017

cramforce Mar 2, 2017

jridgewell Mar 2, 2017

cramforce Mar 5, 2017

jridgewell Mar 7, 2017

cramforce Mar 2, 2017

jridgewell Mar 2, 2017

cramforce Mar 2, 2017

jridgewell Mar 2, 2017

cramforce Mar 5, 2017

jridgewell Mar 7, 2017

cramforce Mar 2, 2017

jridgewell Mar 2, 2017

dknecht commented Mar 2, 2017

jridgewell commented Mar 2, 2017 •

edited

cramforce Mar 5, 2017

cramforce Mar 5, 2017

jridgewell commented Mar 9, 2017

jridgewell commented Mar 16, 2017

Cache SW: Support Precent Diversions #7914

Cache SW: Support Precent Diversions #7914

Conversation

jridgewell commented Mar 1, 2017

cramforce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dknecht commented Mar 2, 2017

jridgewell commented Mar 2, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jridgewell commented Mar 9, 2017

jridgewell commented Mar 16, 2017

jridgewell commented Mar 2, 2017 •

edited