Http cache #496

mnot · 2017-03-01T04:47:30Z

For #373.

Also should help #336 (since HTTP clearly says this, and there's a direct reference now).

I think it addresses #307 (although Anne may want a different approach).

Touches #144.

Preview | Diff

annevk

This looks great. Mostly minor nits. Do we have tests for this already somewhere?

annevk · 2017-03-01T17:49:35Z

fetch.bs

@@ -3342,86 +3345,149 @@ steps:

 <li><p>Let <var>response</var> be null.

+ <li><p>Let the <var>revalidating</var> and <var>partial</var> flags be unset.


I think we should either make these booleans or pull flag into their name.

annevk · 2017-03-01T17:51:32Z

fetch.bs


-    <p class=note>As mandated by HTTP, this still takes the `<code>Vary</code>`
-    <a>header</a> into account.
+    <p class=note>The cache key used to locate a stored response must include the value of the


We cannot use must in notes. I think the right solution here is probably to not make this a note, apart from maybe the bit that is explaining the requirement.

annevk · 2017-03-01T17:56:57Z

fetch.bs


 <li>
+  <!-- If response is still null, we require a forward request. -->


Minor nit: "forwarded request" seems to be the term of art?

Yes. Should I factor that out, explain it, or?

I meant to change from "forward" to "forwarded".

annevk · 2017-03-01T18:05:57Z

fetch.bs

+   <a for=response>status</a> begins with <code>2</code> or <code>3</code>, invalidate appropriate
+   stored responses in the HTTP cache, as per the 
+   "<a href=https://tools.ietf.org/html/rfc7234#section-4.4>Invalidation</a>" chapter of 
+   <cite>HTTP Caching</cite> [[!HTTP-CACHING]], and set <var>storedResponse</var> to null.


This step looks a little scary. 1 "unsafe" can change over time. Are we expecting implementations to change their result over time? If so we should probably reference "unsafe". I guess we also want tests for this.

Also, I think I'm mostly treating status as an integer, so a range check would make more sense, e.g., in the range 200 to 399, inclusive.

Hm. I can reference the part of HTTP that defines the registry:
http://httpwg.org/specs/rfc7231.html#method.registry
... or I can reference the registry directly:
http://www.iana.org/assignments/http-methods/http-methods.xhtml

Preference?

I think http://httpwg.org/specs/rfc7231.html#safe.methods would be the most logical. I mostly meant that the term was not referenced. That it's variable is also somewhat problematic, but maybe okay.

annevk · 2017-03-01T18:10:11Z

fetch.bs

-    "<code>force-cache</code>" or "<code>only-if-cached</code>", then set
-    <var>response</var> to the <a for=/>response</a> in the HTTP cache for
-    <var>httpRequest</var>.
+    <p>Let <var>storedResponse</var> be the result of selecting a response from the HTTP cache,


I think it would be good to declare storedResponse upfront just like partial/revalidating since you end up using it outside these substeps.

And then here you just say "Set storedResponse" to the result of selecting a ..., if any." (meaning it keeps its initial value otherwise).

Where is "if any"? Phrase ends with "or null if no stored response is selected."

I meant that if you declare storedResponse upfront outside these substeps as "Let storedResponse be null" (to make it clearer what its scope is) you can rephrase this too.

annevk · 2017-03-01T18:12:40Z

fetch.bs

-   chapter of <cite>HTTP Caching</cite>. [[!HTTP-CACHING]]
+     <li><p>Set <var>response</var> to <var>storedResponse</var>.
+
+     <p class="note">This updates the stored response in cache as well.


It seems this note is more fitting for the paragraph above? Also, the formatting is a little off. If you have multiple <p> descendants, they need to be on their own line and indented by one.

annevk · 2017-03-01T18:13:10Z

fetch.bs

+
+     <li><p>Set <var>response</var> to <var>storedResponse</var>.
+
+     <p class="note">This updates the stored response in cache as well.


Same here. It seems this note should be for one step earlier.

annevk · 2017-03-01T18:13:48Z

fetch.bs

+    </ol>
+    <!-- TODO: account for partial request failure -->
+
+    <li><p>If <var>response</var> is null, run these substeps:


<p> needs to be on a new line due to the sibling <ol>.

annevk · 2017-03-01T18:17:15Z

fetch.bs

+
+      <p class=note>The cache key used to store the response must include the value of the
+      <var>credentials</var> flag, to ensure that a stored response to a request with credentials is
+      not used to satisfy a request without credentials.


Same comment as earlier, no requirements in notes. Also a formatting glitch here due to multiple <p> inside the <li>.

mnot · 2017-03-02T00:31:00Z

I think all feedback is addressed. Tests are next; planning to start with the direct requirements added here, then work on those incorporated by reference from the RFCs.

annevk · 2017-03-03T13:12:26Z

That is great, while you work on tests, would you mind clicking the checkbox that enables me to push to your branch (should be in the sidebar)? I was mainly thinking of collapsing your commits into one and maybe adding one or more follow up cleanup commits for you to review.

mnot · 2017-03-06T00:44:31Z

Box ticked.

In particular: * Be more specific about terminology * Detail more clearly how requests are to be modified * Make sure credentials flag is taken into account Fixes #373, fixes #336, and fixes #307. Helps with #144.

annevk · 2017-03-06T10:11:55Z

For some reason the box got unticked after I force pushed your collapsed changes with a revised commit message. Can you tick it again as I have a bunch of changes I wanted to push in a new commit? I'm guessing you created this branch well before GitHub introduced this feature, leading to this somewhat weird situation.

mnot · 2017-03-06T23:19:16Z

Weird. Done.

annevk · 2017-03-07T08:24:26Z

Thanks, fixup pushed.

annevk · 2017-03-07T08:24:54Z

Curiously enough it seems again unchecked. I'll contact GitHub support about it now.

annevk · 2017-03-07T16:10:59Z

(Sigh, if you want to troubleshoot the push access revocation issue, you need to contact GitHub yourself. I gave them a rather detailed explanation of what went down over several emails, but that didn't cut it...)

mnot · 2017-03-08T01:20:09Z

Any preference on where to put the tests -- e.g., http/cache, fetch/cache, or? Assuming I'll be testing with fetch, not XHR.

mnot · 2017-03-08T05:52:40Z

As per testing, doesn't seem to be implemented, and this is optional in HTTP anyway.

mnot · 2017-03-08T07:49:49Z

Fixed my tests for invalidation; support is much better than I thought.

Firefox and Safari TP invalidate everything correctly, but they over-invalidate (i.e., they do so even when the response is unsuccessful - 500 in this case).

Safari also shows a bit of variance when hitting 'reload', but I see that in other tests too.

Chrome only invalidates on POST and DELETE, regardless of whether the response is successful.

See:
https://github.com/mnot/web-platform-tests/tree/http-cache

annevk · 2017-03-08T08:11:20Z

fetch/cache or fetch/http-cache seems okay to me. I'd prefer fetch/ over http/ since some of the requirements are outlined here and we're testing the integration.

mnot · 2017-03-14T07:00:09Z

I think my tests are done. Before I do a pull, one question: I have an additional page of tests for partial content (i.e., range requests), but as far as I can tell, they're not supported by any browser, except Safari TP -- and its support is extremely basic.

My inclination is to pull the specification of partial content support in the Fetch HTTP cache; it's an optional feature of HTTP caching, and I don't think we're going to convince other browsers to implement (unless someone suddenly gets VERY interested).

Thoughts? See also #144 (which I'll update in a moment).

annevk · 2017-03-14T07:29:51Z

It seems reasonable to omit processing requirements, but we should at least have a note indicating we are aware of the feature and that it's simply not sufficiently supported or some such.

Having the tests somewhere seems good though in case browsers get interested at some point. Especially given the surprise you got from mcmanus.

mnot · 2017-03-16T05:02:02Z

I think tests are ready (sorry for the last minute additions). I'm thinking we should remove the incorporation of credentials into the cache key from this pull, at least until #307 progresses. Make sense?

annevk · 2017-03-16T08:30:15Z

I guess that's fair.

mnot · 2017-03-16T08:57:58Z

Done.

mnot · 2017-03-20T00:06:00Z

Firefox bugs:

mnot · 2017-03-20T00:19:17Z

Chrome bugs:

mnot · 2017-03-20T00:42:05Z

Edge bugs:

annevk · 2017-03-20T08:12:22Z

Can you click the checkbox again so I can push to your branch?

I think this is all good now. Are the tests ready to land as well?

yoavweiss · 2017-03-21T12:57:02Z

fetch.bs

- <li>
-  <p>If <var>response</var> is null, run these substeps:
+   <li>
+    <p>If the <var>revalidatingFlag</var> is set, then:


If I'm reading this correctly, nothing assigns storedResponse to response if revalidation is not needed, and the cache-mode is not force-cache or only-if-cached.
Am I reading this correctly? If so, is this intentional? I'd expect such assignment to happen if no revalidation is needed.

Ah, thanks - that got acccidentally removed in 2ae9df0. Will fix.

mnot · 2017-03-23T06:34:34Z

Checkbox clicked. Yes, I think this is ready, and so are tests; doubtless there will be more of each, but it's good for now.

Related Fetch change: whatwg/fetch#496.

annevk · 2017-03-23T13:51:37Z

🍸

youennf · 2017-03-23T20:22:03Z

That is a lot of test :)
Is WebKit all fine?

mnot · 2017-03-24T00:53:45Z

AIUI Webkit doesn't do the HTTP caching layer -- or has this changed? I tested Safari TP, see here:
https://www.mnot.net/blog/2017/03/16/browser-caching
In a nutshell, pretty well, a few places to look. Can file bugs if you point me to the right place to do it.

youennf · 2017-03-24T01:06:13Z

WebKit has a bunch of HTTP caches, most notably https://trac.webkit.org/raw-attachment/wiki/March%202015%20Meeting/Disk%20cache.pdf and memory cache.

This is interesting even if it is related to the underlying HTTP library.
https://bugs.webkit.org is the usual place for feedback.

annevk · 2017-03-24T05:53:00Z

@mnot https://bugs.webkit.org/enter_bug.cgi?product=WebKit in particular (random component, prefix subject with "HTTP cache: ").

mnot · 2017-04-11T04:54:43Z

Safari TP bugs:

annevk reviewed Mar 1, 2017

View reviewed changes

annevk added the needs tests Moving the issue forward requires someone to write tests label Mar 3, 2017

mnot and others added 2 commits March 6, 2017 10:54

Rewrite HTTP cache logic

b322a93

In particular: * Be more specific about terminology * Detail more clearly how requests are to be modified * Make sure credentials flag is taken into account Fixes #373, fixes #336, and fixes #307. Helps with #144.

minor editorial nits and indentation

0b52aa9

annevk mentioned this pull request Mar 7, 2017

Make it clearer that cache mode "default" follows normal logic #340

Closed

mnot mentioned this pull request Mar 8, 2017

Vary HTTP cache on credentials mode #307

Open

Don't use stale cached response on network err

4aa2432

As per testing, doesn't seem to be implemented, and this is optional in HTTP anyway.

Take out partial content caching.

2ae9df0

mnot mentioned this pull request Mar 14, 2017

Http cache web-platform-tests/wpt#5137

Merged

remove credentials from the cache key (for now)

7bcec8f

annevk removed the needs tests Moving the issue forward requires someone to write tests label Mar 20, 2017

annevk mentioned this pull request Mar 20, 2017

Use of response's cache state #376

Closed

yoavweiss reviewed Mar 21, 2017

View reviewed changes

set response to storedResponse

51179a3

final nits

edef79f

annevk pushed a commit to web-platform-tests/wpt that referenced this pull request Mar 23, 2017

Fetch: HTTP cache tests

13062c6

Related Fetch change: whatwg/fetch#496.

annevk merged commit cbca2c2 into whatwg:master Mar 23, 2017

		@@ -3342,86 +3345,149 @@ steps:

		<li><p>Let <var>response</var> be null.

		<li><p>Let the <var>revalidating</var> and <var>partial</var> flags be unset.


		<li>
		<!-- If response is still null, we require a forward request. -->


		<li><p>Set <var>response</var> to <var>storedResponse</var>.

		<p class="note">This updates the stored response in cache as well.

Http cache #496

Http cache #496

Conversation

mnot commented Mar 1, 2017 • edited by pr-preview bot Loading

annevk 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

mnot commented Mar 2, 2017

annevk commented Mar 3, 2017

mnot commented Mar 6, 2017

annevk commented Mar 6, 2017

mnot commented Mar 6, 2017

annevk commented Mar 7, 2017

annevk commented Mar 7, 2017

annevk commented Mar 7, 2017

mnot commented Mar 8, 2017

mnot commented Mar 8, 2017 • edited Loading

Invalidation

Credentials in the Cache Key

Using Stale Responses on a Network Error

mnot commented Mar 8, 2017

annevk commented Mar 8, 2017

mnot commented Mar 14, 2017

annevk commented Mar 14, 2017

mnot commented Mar 16, 2017

annevk commented Mar 16, 2017

mnot commented Mar 16, 2017

mnot commented Mar 20, 2017

mnot commented Mar 20, 2017

mnot commented Mar 20, 2017

annevk commented Mar 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnot commented Mar 23, 2017

annevk commented Mar 23, 2017

youennf commented Mar 23, 2017

mnot commented Mar 24, 2017

youennf commented Mar 24, 2017

annevk commented Mar 24, 2017

mnot commented Apr 11, 2017

mnot commented Mar 1, 2017 •

edited by pr-preview bot

Loading

mnot commented Mar 8, 2017 •

edited

Loading