Skip to content
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.

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

Add Stale While Revalidate Handling #853

Open
wants to merge 1 commit into
base: master
from

Conversation

9 participants
@dtapuska
Copy link

dtapuska commented Dec 21, 2018

Define a new cache mode "async-stale-revalidated" to indicate that the
fetch stack should return a stale response if it can and aysnchronously
revalidate the response.

Fixes #852


Preview | Diff

@dtapuska dtapuska force-pushed the dtapuska:master branch from e5c5baa to b351056 Dec 21, 2018

@annevk
Copy link
Member

annevk left a comment

I don't really like the name as fetches are already asynchronous. Perhaps we should simply use "stale-while-revalidate" as token name?

What about "must-revalidate" as @ziyunfei commented?

@mnot I'd like you to have a look too.

fetch.bs Outdated
@@ -4482,6 +4492,11 @@ Range Requests</cite>. [[HTTP-RANGE]] However, this is not widely supported by b
<li><p>If <i>authentication-fetch flag</i> is set, then create an <a>authentication entry</a>
for <var>request</var> and the given realm.

<li><p>If <i>revalidatingFlag</i> is set and <var>httpRequest</var>'s <a for=request>cache mode</a>
is "<code>async-stale-revalidated</code>" then asynchronously perform a

This comment has been minimized.

@annevk

annevk Jan 2, 2019

Member

This needs a comma before then. "asynchronously" is not a thing, perhaps "in parallel"?

This comment has been minimized.

@dtapuska
fetch.bs Outdated
@@ -4482,6 +4492,11 @@ Range Requests</cite>. [[HTTP-RANGE]] However, this is not widely supported by b
<li><p>If <i>authentication-fetch flag</i> is set, then create an <a>authentication entry</a>
for <var>request</var> and the given realm.

<li><p>If <i>revalidatingFlag</i> is set and <var>httpRequest</var>'s <a for=request>cache mode</a>
is "<code>async-stale-revalidated</code>" then asynchronously perform a
<a>HTTP-network-or-cache fetch</a> using <var>request</var> with <a for=request>cache mode</a>

This comment has been minimized.

@annevk

annevk Jan 2, 2019

Member

This will bypass the service worker. Is that okay? It seems problematic that you can create a network request like that.

This comment has been minimized.

@kinu

kinu Jan 18, 2019

SwR is a concept of HTTP cache, which basically sits below the service worker, so service workers being not able to observe revalidation requests sounds somewhat natural to me from implementation pov. That's said... in this way service workers may keep holding stale responses in cache storage, and now that we're talking about Fetch layer's SwR handling, it might be worth discussing.

@dtapuska dtapuska force-pushed the dtapuska:master branch from b351056 to 14280da Jan 15, 2019

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Jan 18, 2019

@dtapuska dtapuska force-pushed the dtapuska:master branch 2 times, most recently from da68bc4 to 788b7bc Jan 21, 2019

Show resolved Hide resolved fetch.bs Outdated

chromium-wpt-export-bot added a commit to web-platform-tests/wpt that referenced this pull request Jan 22, 2019

Add tentative WPT tests for stale while revalidate handling.
Add test to ensure that handling the fetch doesn't trigger stale
while revalidate loading.

Add test to ensure that scripts loaded trigger a stale while revalidate
cache hit and resource timing entries are correct.

The PR for the spec changes is here:
whatwg/fetch#853

BUG=348877

Change-Id: Ib07b98d0d2595b6b99857161f830343bf7516518

aarongable pushed a commit to chromium/chromium that referenced this pull request Jan 23, 2019

Add tentative WPT tests for stale while revalidate handling.
Add test to ensure that handling the fetch doesn't trigger stale
while revalidate loading.

Add test to ensure that scripts loaded trigger a stale while revalidate
cache hit and resource timing entries are correct.

The PR for the spec changes is here:
whatwg/fetch#853

BUG=348877

Change-Id: Ib07b98d0d2595b6b99857161f830343bf7516518
Reviewed-on: https://chromium-review.googlesource.com/c/1383297
Commit-Queue: Dave Tapuska <dtapuska@chromium.org>
Reviewed-by: Ben Kelly <wanderview@chromium.org>
Cr-Commit-Position: refs/heads/master@{#625228}

chromium-wpt-export-bot added a commit to web-platform-tests/wpt that referenced this pull request Jan 23, 2019

Add tentative WPT tests for stale while revalidate handling.
Add test to ensure that handling the fetch doesn't trigger stale
while revalidate loading.

Add test to ensure that scripts loaded trigger a stale while revalidate
cache hit and resource timing entries are correct.

The PR for the spec changes is here:
whatwg/fetch#853

BUG=348877

Change-Id: Ib07b98d0d2595b6b99857161f830343bf7516518
Reviewed-on: https://chromium-review.googlesource.com/c/1383297
Commit-Queue: Dave Tapuska <dtapuska@chromium.org>
Reviewed-by: Ben Kelly <wanderview@chromium.org>
Cr-Commit-Position: refs/heads/master@{#625228}

aarongable pushed a commit to chromium/chromium that referenced this pull request Jan 24, 2019

Findit
Revert "Add tentative WPT tests for stale while revalidate handling."
This reverts commit 05cdb4d.

Reason for revert:

Findit (https://goo.gl/kROfz5) identified CL at revision 625228 as the
culprit for flakes in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/flake/flake-culprit?key=ag9zfmZpbmRpdC1mb3ItbWVyQwsSDEZsYWtlQ3VscHJpdCIxY2hyb21pdW0vMDVjZGI0ZDFkOWNkYWEyZGYzZDkxY2Q5ZGMxNTBlZGE5MDNmZTM1Ygw

Sample Failed Build: https://ci.chromium.org/buildbot/chromium.linux/Linux%20Tests%20%28dbg%29%281%29/77482

Sample Failed Step: webkit_layout_tests

Sample Flaky Test: virtual/outofblink-cors-ns/external/wpt/fetch/stale-while-revalidate/stale-script.tentative.html

Original change's description:
> Add tentative WPT tests for stale while revalidate handling.
> 
> Add test to ensure that handling the fetch doesn't trigger stale
> while revalidate loading.
> 
> Add test to ensure that scripts loaded trigger a stale while revalidate
> cache hit and resource timing entries are correct.
> 
> The PR for the spec changes is here:
> whatwg/fetch#853
> 
> BUG=348877
> 
> Change-Id: Ib07b98d0d2595b6b99857161f830343bf7516518
> Reviewed-on: https://chromium-review.googlesource.com/c/1383297
> Commit-Queue: Dave Tapuska <dtapuska@chromium.org>
> Reviewed-by: Ben Kelly <wanderview@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#625228}

No-Presubmit: true
No-Tree-Checks: true
No-Try: true
BUG=348877

Change-Id: Ie11ef55a64fcf3e39781666b65a47bbd157f04a2
Reviewed-on: https://chromium-review.googlesource.com/c/1432937
Cr-Commit-Position: refs/heads/master@{#625497}

chromium-wpt-export-bot added a commit to web-platform-tests/wpt that referenced this pull request Jan 24, 2019

Reland add tentative WPT tests for stale while revalidate handling.
Add test to ensure that handling the fetch doesn't trigger stale
while revalidate loading.

Add test to ensure that scripts and css loaded trigger a stale
while revalidate cache hit.

The PR for the spec changes is here:
whatwg/fetch#853

This reland removes the resource timing and moves to css (instead of
images) which were both unreliable.

BUG=348877

Change-Id: Ibabd8d3fd0295bedc8259594fc926da6ab5cfc43

chromium-wpt-export-bot added a commit to web-platform-tests/wpt that referenced this pull request Jan 24, 2019

Reland add tentative WPT tests for stale while revalidate handling.
Add test to ensure that handling the fetch doesn't trigger stale
while revalidate loading.

Add test to ensure that scripts and css loaded trigger a stale
while revalidate cache hit.

The PR for the spec changes is here:
whatwg/fetch#853

This reland removes the resource timing and moves to css (instead of
images) which were both unreliable.

BUG=348877

Change-Id: Ibabd8d3fd0295bedc8259594fc926da6ab5cfc43

chromium-wpt-export-bot added a commit to web-platform-tests/wpt that referenced this pull request Jan 25, 2019

Reland add tentative WPT tests for stale while revalidate handling.
Add test to ensure that handling the fetch doesn't trigger stale
while revalidate loading.

Add test to ensure that scripts and css loaded trigger a stale
while revalidate cache hit.

The PR for the spec changes is here:
whatwg/fetch#853

This reland removes the resource timing and moves to css (instead of
images) which were both unreliable.

BUG=348877

Change-Id: Ibabd8d3fd0295bedc8259594fc926da6ab5cfc43

chromium-wpt-export-bot added a commit to web-platform-tests/wpt that referenced this pull request Jan 25, 2019

Reland add tentative WPT tests for stale while revalidate handling.
Add test to ensure that handling the fetch doesn't trigger stale
while revalidate loading.

Add test to ensure that scripts and css loaded trigger a stale
while revalidate cache hit.

The PR for the spec changes is here:
whatwg/fetch#853

This reland removes the resource timing and moves to css (instead of
images) which were both unreliable.

BUG=348877

Change-Id: Ibabd8d3fd0295bedc8259594fc926da6ab5cfc43

chromium-wpt-export-bot added a commit to web-platform-tests/wpt that referenced this pull request Jan 25, 2019

Reland add tentative WPT tests for stale while revalidate handling.
Add test to ensure that handling the fetch doesn't trigger stale
while revalidate loading.

Add test to ensure that scripts and css loaded trigger a stale
while revalidate cache hit.

The PR for the spec changes is here:
whatwg/fetch#853

This reland removes the resource timing and moves to css (instead of
images) which were both unreliable.

BUG=348877

Change-Id: Ibabd8d3fd0295bedc8259594fc926da6ab5cfc43
Reviewed-on: https://chromium-review.googlesource.com/c/1434776
Commit-Queue: Dave Tapuska <dtapuska@chromium.org>
Reviewed-by: Ben Kelly <wanderview@chromium.org>
Cr-Commit-Position: refs/heads/master@{#626264}

aarongable pushed a commit to chromium/chromium that referenced this pull request Jan 26, 2019

Reland add tentative WPT tests for stale while revalidate handling.
Add test to ensure that handling the fetch doesn't trigger stale
while revalidate loading.

Add test to ensure that scripts and css loaded trigger a stale
while revalidate cache hit.

The PR for the spec changes is here:
whatwg/fetch#853

This reland removes the resource timing and moves to css (instead of
images) which were both unreliable.

BUG=348877

Change-Id: Ibabd8d3fd0295bedc8259594fc926da6ab5cfc43
Reviewed-on: https://chromium-review.googlesource.com/c/1434776
Commit-Queue: Dave Tapuska <dtapuska@chromium.org>
Reviewed-by: Ben Kelly <wanderview@chromium.org>
Cr-Commit-Position: refs/heads/master@{#626264}

chromium-wpt-export-bot added a commit to web-platform-tests/wpt that referenced this pull request Jan 26, 2019

Reland add tentative WPT tests for stale while revalidate handling.
Add test to ensure that handling the fetch doesn't trigger stale
while revalidate loading.

Add test to ensure that scripts and css loaded trigger a stale
while revalidate cache hit.

The PR for the spec changes is here:
whatwg/fetch#853

This reland removes the resource timing and moves to css (instead of
images) which were both unreliable.

BUG=348877

Change-Id: Ibabd8d3fd0295bedc8259594fc926da6ab5cfc43
Reviewed-on: https://chromium-review.googlesource.com/c/1434776
Commit-Queue: Dave Tapuska <dtapuska@chromium.org>
Reviewed-by: Ben Kelly <wanderview@chromium.org>
Cr-Commit-Position: refs/heads/master@{#626264}
@dtapuska

This comment has been minimized.

Copy link
Author

dtapuska commented Jan 28, 2019

This pull request now has tests that have been merged, I think the label can be removed.

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Jan 28, 2019

@dtapuska so as far as I can tell the naming comment is still outstanding as well as the new ability to bypass service workers when it comes to network requests.

Also, is this parallel request fully defined? What if there's now a 401? Or a redirect (not followed per current rules)?

@dtapuska

This comment has been minimized.

Copy link
Author

dtapuska commented Jan 28, 2019

@annevk

We don't do anything different for 401s or redirects. It is exactly the same as another sub-resource; standard processing occurs. This is the benefit of having the revalidation associated with the context that initiated the resource request.

The text indicates "in parallel perform" is that the naming? I indicated "Done" on the comment above. Is that the naming you are referring to?

Did Kinuko's response not suffice? Top level requests are sent to the service worker if associated. Can you explain why you think the revalidation should go back to the service worker? Fetches created from the service worker have a "" destination.

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Jan 28, 2019

@dtapuska if standard processing occurs, the current PR is wrong, right? It invokes an algorithm that doesn't handle redirects.

The naming comment is about "async" in the new enum value. Fetching is already asynchronous for all intents and purposes so this name is rather confusing.

As for the service worker, we don't have many fetches that can bypass it, so the problem would be that now there is a way and the service worker can do nothing about it. That breaks a design decision around service workers.

@dtapuska dtapuska changed the title Add Async Stale While Revalidate Handling Add Stale While Revalidate Handling Jan 28, 2019

@dtapuska

This comment has been minimized.

Copy link
Author

dtapuska commented Jan 28, 2019

@annevk Sorry which enum value are you referring to? Only async I see in the pull request was in the title. I've since removed that.

So you are saying you want the algorithm to do in parallel the "HTTP redirect fetch" or a "HTTP fetch with service worker mode set to none" instead of going into the network or cache fetch directly. That seems to make sense.

fetch.bs Outdated
<li><p>If <var>httpRequest</var>'s <a for=request>allows stale content flag</a> is set and
<var>httpRequest</var>'s <a for=request>header list</a> <a for="header list">contains</a>
<code>Cache-Control: Must-Revalidate</code>, then unset <var>httpRequest</var>'s
<a for=request>allows stale content flag</a>.

This comment has been minimized.

@annevk

annevk Jan 28, 2019

Member

I'm sorry, I had missed the approach changed substantially (at least I thought there was an enum involved originally). This doesn't quite work. You need to extract the header value and parse/compare it in some manner.

This comment has been minimized.

@dtapuska

dtapuska Feb 6, 2019

Author

I've made it clear that we are looking for a directive in the header. Please let me know if this isn't sufficient. I've looked for other examples of parsing the cache-control header but didn't find any.

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Jan 28, 2019

I'm not sure, it's still not clear to me what the processing model is, and I'd like someone other than me to judge it's suddenly okay to bypass service workers when making network requests. That seems like a new primitive and rather isolated at that.

@dtapuska

This comment has been minimized.

Copy link
Author

dtapuska commented Feb 15, 2019

So by default. Fetches that enter the fetch spec (not via Request.init(...))) ie. HTML spec and friends that are GET subresources may set the allows stale content flag. Which will cause an additional request that doesn't have the revalidation bit set on if the response received is stale.

If you are asking will this be eventually exposed such that service workers can explicitly request stale content it can be done in the future via an explicit API (say on the cache mode). But I don't believe it is needed at this time.

@jakearchibald

This comment has been minimized.

Copy link
Collaborator

jakearchibald commented Feb 18, 2019

@dtapuska interesting. So individual APIs will have to opt into this by setting the mode to "stale-while-revalidate"?

@mnot

This comment has been minimized.

Copy link
Member

mnot commented Feb 19, 2019

@dtapuska you seem to be conflating "the request allows stale content" and "the use of stale content will kick off a revalidating request asynchronously." The former is max-stale in HTTP caching; the latter is stale-while-revalidate.

I was concerned about similar things to @jakearchibald above in #852; HTTP already defines how responses can be used for SwR, but there's no hint of that here.

@annevk this seems to be a recurring pattern; HTTP defines what happens on the wire, and Fetch defines a corresponding API for each feature. If that's going to be normal going forward, it'd probably be best to document the division of labour (as it were) so that people don't get confused.

Also, it'd be good to get alignment on whether or not browsers pay attention to request Cache-Control (see tests) and how that interacts with these Fetch directives.

@dtapuska dtapuska force-pushed the dtapuska:master branch 2 times, most recently from 654ab85 to 710c290 Feb 19, 2019

@dtapuska

This comment has been minimized.

Copy link
Author

dtapuska commented Feb 20, 2019

@mnot In looking at it again I can see how it is confusing. I've added definitions indicating "stale response in revalidate window" and "stale response". Hopefully that clarifies the conditions.

@jakearchibald I've removed the restriction on fetch as discussed offline.

@jakearchibald
Copy link
Collaborator

jakearchibald left a comment

Feels like this is heading in the right direction.

fetch.bs Outdated

<p class=note>See also the
"<a href=https://tools.ietf.org/html/rfc7234#section-4.3.4>Sending a Validation Request</a>"
chapter of <cite>HTTP Caching</cite> [[!HTTP-CACHING]].

<li><p>Otherwise, set <var>response</var> to <var>storedResponse</var> and set
<li><p>Otherwise, set <var>response</var> to <var>storedResponse</var> and set

This comment has been minimized.

@jakearchibald

jakearchibald Feb 21, 2019

Collaborator

This is the second "otherwise" to this if statement. Maybe the previous one is meant to be an otherwise-if, but it isn't clear without nesting.

This comment has been minimized.

@jakearchibald

jakearchibald Feb 21, 2019

Collaborator

It might be because of the messed-up if statement, but it kinda looks like a stale response could be used even if the stale content mode is "disallow".

This comment has been minimized.

@dtapuska

dtapuska Feb 21, 2019

Author

An <ol> is missing here. The last Otherwise is supposed to be inside the main Otherwise.

fetch.bs Outdated
<li><p>Set <var>response</var>'s <a for=response>cache state</a> to "<code>local</code>".
<li><p>In parallel perform <a for=main>main fetch</a> using <var>request</var> with
<ul class=brief>
<li><p><i>stale content mode</i> set to "<code>disallow</code>" and

This comment has been minimized.

@jakearchibald

jakearchibald Feb 21, 2019

Collaborator

Does a cache mode of "reload" work better here?

It may also be worth adding a note that the response isn't use other than to populate the cache.

If the user closes the tab, should this request be canceled?

This comment has been minimized.

@dtapuska

dtapuska Feb 21, 2019

Author

Reload doesn't work here because it won't cause a revalidation but a forced network load. Nor do we want to append any special Cache-Control headers.

I've added a note.

fetch.bs Outdated
@@ -1901,6 +1910,13 @@ is a <a>filtered response</a> whose
<li><p>Return <var>newResponse</var>.
</ol>

<p>A <dfn export id=concept-stale-response>stale response</dfn> is a <a for=/>response</a> where
the freshness lifetime calculation (excluding the stale-while-revalidate directive) has not
exceeded the current age.

This comment has been minimized.

@jakearchibald

jakearchibald Feb 21, 2019

Collaborator

Isn't this the definition of a fresh response?

This comment has been minimized.

@dtapuska

dtapuska Feb 21, 2019

Author

Yes you are correct

This comment has been minimized.

@jakearchibald

jakearchibald Feb 27, 2019

Collaborator

We shouldn't call a fresh response a "stale response".

This comment has been minimized.

@dtapuska
fetch.bs Outdated
exceeded the current age.

<p>A <dfn export id=concept-stale-response>stale response inside revalidate window</dfn> is
a <a>stale response</a> where the freshness lifetime calculation (including the stale-while-revalidate

This comment has been minimized.

@jakearchibald

jakearchibald Feb 21, 2019

Collaborator

I don't think this is quite right. To say an "A" is a "B", "A" must be a subset of "B", but in this case it seems the other way around.

This comment has been minimized.

@dtapuska

dtapuska Feb 21, 2019

Author

Probably using explicit text here is better. Done.

fetch.bs Outdated

<ol>
<li><p>If <var>httpRequest</var>'s <a for=request>cache mode</a> is "<code>default</code>",
<var>httpRequest</var>'s <a for=request>method</a> is `<code>GET</code>`, and

This comment has been minimized.

@jakearchibald

jakearchibald Feb 21, 2019

Collaborator

Is this applying the "GET" restriction to other kinds of state response fetching, like "only-if-cached"?

This comment has been minimized.

@dtapuska

dtapuska Feb 21, 2019

Author

No it isn't but perhaps the "stale content mode" name definitely gets confusing here. Not sure what to do.

fetch.bs Outdated
@@ -1526,6 +1531,10 @@ Unless stated otherwise, it is zero.
which is "<code>basic</code>", "<code>cors</code>", or "<code>opaque</code>".
Unless stated otherwise, it is "<code>basic</code>".

<p>A <a for=/>request</a> has an associated <dfn export for=request id=stale-content-mode>stale content mode</dfn>,
which is "<code>allow</code>", "<code>disallow</code>" or "<code>default</code>". Unless stated
otherwise, it is "<code>default</code>".

This comment has been minimized.

@jakearchibald

jakearchibald Feb 21, 2019

Collaborator

I'm not 100% sure, but I'm not sure this is needed. Can't the current values of "cache mode" handle this?

This comment has been minimized.

@dtapuska

dtapuska Feb 21, 2019

Author

No they can't. There is no "must-revalidate" mode in the cache-mode. Reload causes it to skip the cache.

This comment has been minimized.

@kinu

kinu Feb 27, 2019

What do we think about adding a new cache mode for that? Currently it says the 'stale content mode' change is only applied for 'default', and other cache mode says something about cache behavior too, so it feels the mode can be one of the cache mode?

This comment has been minimized.

@jakearchibald

jakearchibald Feb 27, 2019

Collaborator

If the only difference between "no-cache" and a SWR update request is the SWR update request doesn't add the Cache-Control: max-age: 0 header, should we just add a flag to prevent that step?

This comment has been minimized.

@dtapuska

dtapuska Mar 6, 2019

Author

I've added a "no-cache local only flag" and then I can use the no-cache mode. If you have a better name I can change it.

@dtapuska dtapuska force-pushed the dtapuska:master branch from 710c290 to fa44957 Feb 21, 2019

@kinu
Copy link

kinu left a comment

Some more comments. This starts to look nicer to me!

fetch.bs Outdated
will be created, and a normal request otherwise. It then updates the HTTP cache with the response.
[[!HTTP]] [[!HTTP-SEMANTICS]] [[!HTTP-COND]] [[!HTTP-CACHING]] [[!HTTP-AUTH]]
If there is a fresh response it will be used. If there is a <a>stale response inside revalidate window</a>
and the request has the <a for=request>stale content mode</a> set it will be returned

This comment has been minimized.

@kinu

kinu Feb 27, 2019

Looks like stale content mode is no longer a boolean but has a value, this text needs to be updated as well?

This comment has been minimized.

@dtapuska

dtapuska Mar 6, 2019

Author

This text is no longer present.

fetch.bs Outdated
@@ -1526,6 +1531,10 @@ Unless stated otherwise, it is zero.
which is "<code>basic</code>", "<code>cors</code>", or "<code>opaque</code>".
Unless stated otherwise, it is "<code>basic</code>".

<p>A <a for=/>request</a> has an associated <dfn export for=request id=stale-content-mode>stale content mode</dfn>,
which is "<code>allow</code>", "<code>disallow</code>" or "<code>default</code>". Unless stated
otherwise, it is "<code>default</code>".

This comment has been minimized.

@kinu

kinu Feb 27, 2019

What do we think about adding a new cache mode for that? Currently it says the 'stale content mode' change is only applied for 'default', and other cache mode says something about cache behavior too, so it feels the mode can be one of the cache mode?

fetch.bs Outdated
If there is a fresh response it will be used. If there is a <a>stale response inside revalidate window</a>
and the request has the <a for=request>stale content mode</a> set it will be returned
immediately. If there is a <a>stale response</a> a conditional request will be created and the
response will update the HTTP cache. Otherwise, a normal network request will be issued.

This comment has been minimized.

@jakearchibald

jakearchibald Feb 27, 2019

Collaborator

I still think this could be clearer (given it's non-normative).

If we had definitions like:

A fresh response is a response whose current age is within its freshness lifetime.

A stale-while-revalidate response is a response that is not a fresh response, and its current age is within its stale-while-revalidate lifetime.

A stale response is a response that is not a fresh response or a stale-while-revalidate response.

("freshness lifetime" and "stale-while-revalidate lifetime" should be linked to something which defines those calculations)

Then, the "default" can be described as:

  1. If the HTTP cache contains a matching fresh response it will be returned.
  2. If the HTTP cache contains a matching stale-while-revalidate response it will be returned, and a conditional network fetch will be made to update the entry in the HTTP cache.
  3. If the HTTP cache contains a matching stale response, a conditional network fetch will be returned.
  4. Otherwise, a non-conditional network fetch will be returned.

This comment has been minimized.

@dtapuska
fetch.bs Outdated
@@ -1526,6 +1531,10 @@ Unless stated otherwise, it is zero.
which is "<code>basic</code>", "<code>cors</code>", or "<code>opaque</code>".
Unless stated otherwise, it is "<code>basic</code>".

<p>A <a for=/>request</a> has an associated <dfn export for=request id=stale-content-mode>stale content mode</dfn>,
which is "<code>allow</code>", "<code>disallow</code>" or "<code>default</code>". Unless stated
otherwise, it is "<code>default</code>".

This comment has been minimized.

@jakearchibald

jakearchibald Feb 27, 2019

Collaborator

If the only difference between "no-cache" and a SWR update request is the SWR update request doesn't add the Cache-Control: max-age: 0 header, should we just add a flag to prevent that step?

fetch.bs Outdated
@@ -1901,6 +1910,13 @@ is a <a>filtered response</a> whose
<li><p>Return <var>newResponse</var>.
</ol>

<p>A <dfn export id=concept-stale-response>stale response</dfn> is a <a for=/>response</a> where
the freshness lifetime calculation (excluding the stale-while-revalidate directive) has not
exceeded the current age.

This comment has been minimized.

@jakearchibald

jakearchibald Feb 27, 2019

Collaborator

We shouldn't call a fresh response a "stale response".

@dtapuska dtapuska force-pushed the dtapuska:master branch from fa44957 to 8981736 Mar 6, 2019

@jakearchibald
Copy link
Collaborator

jakearchibald left a comment

Aside from the nits, I'm happy with this. I agree that "no-cache local only flag" is a confusing name, but I can't think of a better one.

Maybe "prevent no-cache cache-control header modification flag"? It's a bit long, but it says what it does.

Show resolved Hide resolved fetch.bs Outdated
Show resolved Hide resolved fetch.bs Outdated
Show resolved Hide resolved fetch.bs Outdated

@dtapuska dtapuska force-pushed the dtapuska:master branch from 8981736 to 136bfd7 Mar 7, 2019

@kinu

This comment has been minimized.

Copy link

kinu commented Mar 8, 2019

Took another look, just wanted to say that this looks really good now :) (Thanks Jake for thorough comments)

@jakearchibald

This comment has been minimized.

Copy link
Collaborator

jakearchibald commented Mar 8, 2019

@annevk us Chromefolk are happy with this now. Want to take another look?

@annevk
Copy link
Member

annevk left a comment

Thanks, this looks very different from last time I looked! I've given mostly style feedback assuming not cloning the request is an oversight and the other errors are too.

@mnot the current state of this PR seems to be that we're not adding new API surface, but we are requiring that the fetch (request, if you will) made by the HTTP cache layer follows Fetch's general rules. That is quite important for browsers to meet their security guarantees. I guess in general when new HTTP features come up it would be good to see what their end-to-end integration might end up being. At least for features aimed at browsers.

Show resolved Hide resolved fetch.bs Outdated
Show resolved Hide resolved fetch.bs
Show resolved Hide resolved fetch.bs Outdated
Show resolved Hide resolved fetch.bs Outdated
Show resolved Hide resolved fetch.bs Outdated
fetch.bs Outdated
<li><p>Set <var>response</var> to <var>storedResponse</var>.
<li><p>Set <var>response</var>'s <a for=response>cache state</a> to "<code>local</code>".
<li><p>In parallel perform <a for=main>main fetch</a> using <var>request</var> with
<ul class=brief>

This comment has been minimized.

@annevk

annevk Mar 12, 2019

Member

Indentation is wrong here.

Show resolved Hide resolved fetch.bs Outdated
fetch.bs Outdated
<var>httpRequest</var>'s <a for=request>header list</a>.
<li><p>If <var>storedResponse</var> is a <a>stale response</a>, then set the
<var>revalidatingFlag</var>.
<p>If the <var>revalidatingFlag</var> is set and <a for=request>cache mode</a>

This comment has been minimized.

@annevk

annevk Mar 12, 2019

Member

It seems this should be a new list item?

fetch.bs Outdated
<li><p>If <var>storedResponse</var> is a <a>stale response</a>, then set the
<var>revalidatingFlag</var>.
<p>If the <var>revalidatingFlag</var> is set and <a for=request>cache mode</a>
is not one of "<code>force-cache</code>", "<code>only-if-cached</code>", then:

This comment has been minimized.

@annevk

annevk Mar 12, 2019

Member

This sentence doesn't make sense? Now that there's no new enum value the original wording should be restored I think.

This comment has been minimized.

@dtapuska
fetch.bs Outdated
<var>httpRequest</var>'s <a for=request>header list</a>.
<li><p>If <var>storedResponse</var> is a <a>stale response</a>, then set the
<var>revalidatingFlag</var>.
<p>If the <var>revalidatingFlag</var> is set and <a for=request>cache mode</a>

This comment has been minimized.

@annevk

annevk Mar 12, 2019

Member

This also dropped where you got "cache mode" from. Which request?

This comment has been minimized.

@dtapuska

@dtapuska dtapuska force-pushed the dtapuska:master branch 2 times, most recently from 35d386d to d5aa21d Mar 12, 2019

@annevk
Copy link
Member

annevk left a comment

Thanks for filing those bugs. It seems there's tests too. Do we have one other interested implementer?

Show resolved Hide resolved fetch.bs Outdated
Show resolved Hide resolved fetch.bs Outdated
Show resolved Hide resolved fetch.bs Outdated
Show resolved Hide resolved fetch.bs Outdated
Add Async Stale While Revalidate Handling
Add definitions around fresh, stale-while-revalidate and stale responses.

Define a new flag that allows no-cache mode to not append Cache-Control
header. This allows stale while revalidate to use this mode and not to
force revalidation of upstream caches.

Fixes #852

@dtapuska dtapuska force-pushed the dtapuska:master branch from d5aa21d to 99c3b37 Mar 12, 2019

@uasan

This comment has been minimized.

Copy link

uasan commented Mar 15, 2019

Hello.

You are planning to implement support stale-if-error?
https://tools.ietf.org/html/rfc5861#section-4

This real cases, used in reverse proxy Nginx
https://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_cache_use_stale

Thanks.

@dtapuska

This comment has been minimized.

Copy link
Author

dtapuska commented Mar 15, 2019

@uasan I don't have a plan for it at this time.

@dtapuska

This comment has been minimized.

Copy link
Author

dtapuska commented Mar 19, 2019

@mayhemer does opening https://bugzilla.mozilla.org/show_bug.cgi?id=1536511 mean that you have interest in proceeding with this PR. Ultimately we don't want to leave the PR in limbo forever.

@mayhemer

This comment has been minimized.

Copy link

mayhemer commented Mar 19, 2019

@mayhemer does opening https://bugzilla.mozilla.org/show_bug.cgi?id=1536511 mean that you have interest in proceeding with this PR. Ultimately we don't want to leave the PR in limbo forever.

Yes, we intend to. There doesn't seem to be any problem with it immediately visible, it's beneficial and easy to implement.

As I wrote at mozilla/standards-positions#144 (comment) we should keep the HTTP-cache and fetch() parts separate. We now have two bug (see that comment.) The fetch() changes are still in development, in this thread, is that so?

@dtapuska

This comment has been minimized.

Copy link
Author

dtapuska commented Mar 19, 2019

The fetch changes are done as proposed in Chrome but to merge the PR into the formal specification WHATWG requires two browser vendors to commit to it (otherwise it remains a PR indefinitely). And that is the question I'm asking.

@mayhemer

This comment has been minimized.

Copy link

mayhemer commented Mar 19, 2019

The fetch changes are done as proposed in Chrome but to merge the PR into the formal specification WHATWG requires two browser vendors to commit to it (otherwise it remains a PR indefinitely). And that is the question I'm asking.

Aha, now it's clear to me. Can you please add here or in our (mozilla) bug https://bugzilla.mozilla.org/show_bug.cgi?id=1534698 direct reference to the final form of the specification changes for fetch that Chrome is proposing?

(In that bug's description, https://crbug.com/348877 is referring only the HTTP-cache part (mozilla mirror bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1536511). The other link is for this thread. And the last one just points at the mozilla positions discussion.)

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.