Re: [whatwg/fetch] HTTP cache partitioning (#943) from Domenic Denicola on 2020-02-27 (public-webapps-github@w3.org from February 2020)

From: Domenic Denicola <notifications@github.com>
Date: Thu, 27 Feb 2020 14:19:55 -0800
To: whatwg/fetch <fetch@noreply.github.com>
Cc: Subscribed <subscribed@noreply.github.com>
Message-ID: <whatwg/fetch/pull/943/review/366035588@github.com>
domenic requested changes on this pull request.



> @@ -1417,42 +1417,45 @@ Unless stated otherwise, it is unset.
 "<code>no-cache</code>", "<code>force-cache</code>", or
 "<code>only-if-cached</code>". Unless stated otherwise, it is "<code>default</code>".
 
+<p>Let <var>httpCache</var> be the result of <a>determining the HTTP cache partition</a> for

This step doesn't seem to be located in the correct place. What uses this httpCache variable? It needs to be inside the fetch algorithm, not standing alone at top level. Then you can pass it another variable representing the current request as an argument, instead of passing it the general concept of a request.

>  <div class="note no-backref">
  <dl>
   <dt>"<code>default</code>"
-  <dd><a for=/>Fetch</a> will inspect the HTTP cache on the way to the network. If the HTTP cache
-  contains a matching <a>fresh response</a> it will be returned. If the HTTP cache contains a
-  matching <a>stale-while-revalidate response</a> it will be returned, and a conditional network
-  fetch will be made to update the entry in the HTTP cache. If the HTTP cache contains a matching
-  <a>stale response</a>, a conditional network fetch will be returned to update the entry in
-  the HTTP cache. Otherwise, a non-conditional network fetch will be returned to update the entry
-  in the HTTP cache. [[!HTTP]] [[!HTTP-SEMANTICS]] [[!HTTP-COND]] [[!HTTP-CACHING]] [[!HTTP-AUTH]]
-  [[!STALE-WHILE-REVALIDATE]]
+  <dd><a for=/>Fetch</a> will inspect the <var>httpCache</var> on the way to the network. If the
+  <var>httpCache</var> contains a matching <a>fresh response</a> it will be returned. If the

These non-normative explanations of different cache modes should not refer to a variable. Probably they should be reverted; they were reasonable before.

> @@ -2049,6 +2052,34 @@ clearly stipulates that <a>connections</a> are keyed on
 <!-- See https://github.com/whatwg/fetch/issues/114#issuecomment-143500095 for when we make
      WebSocket saner -->
 
+<h3 id=http-cache-partitions>HTTP cache partitions</h3>
+
+<p>For <dfn export>determining the HTTP cache partition</dfn>, given <var>request</var>,
+run these steps:

Instead of "for determining the HTTP cache partition", it should be "to determine the HTTP cache partition".

> @@ -2049,6 +2052,34 @@ clearly stipulates that <a>connections</a> are keyed on
 <!-- See https://github.com/whatwg/fetch/issues/114#issuecomment-143500095 for when we make
      WebSocket saner -->
 
+<h3 id=http-cache-partitions>HTTP cache partitions</h3>
+
+<p>For <dfn export>determining the HTTP cache partition</dfn>, given <var>request</var>,
+run these steps:
+
+<ol>
+ <li>
+  <p>Let <var>environmentSettings</var> be <var>request</var>'s <a for=request>client</a>, if
+  present, otherwise return null.</p></li>

This is a bit confusing. I would just have a second step that says

1. If _environmentSettings_ is null, then return null.

> @@ -2049,6 +2052,34 @@ clearly stipulates that <a>connections</a> are keyed on
 <!-- See https://github.com/whatwg/fetch/issues/114#issuecomment-143500095 for when we make
      WebSocket saner -->
 
+<h3 id=http-cache-partitions>HTTP cache partitions</h3>
+
+<p>For <dfn export>determining the HTTP cache partition</dfn>, given <var>request</var>,
+run these steps:
+
+<ol>
+ <li>
+  <p>Let <var>environmentSettings</var> be <var>request</var>'s <a for=request>client</a>, if
+  present, otherwise return null.</p></li>
+
+ <li>
+  <p>Let <var>topLevelOrigin</var> be <var>environmentSettings</var>'s top-level origin and
+  <var>origin</var> be <a for="environment settings object">origin</a>.</p></li>

```suggestion
  <var>origin</var> be its <a for="environment settings object">origin</a>.</p></li>
```

> +
+<p>For <dfn export>determining the HTTP cache partition</dfn>, given <var>request</var>,
+run these steps:
+
+<ol>
+ <li>
+  <p>Let <var>environmentSettings</var> be <var>request</var>'s <a for=request>client</a>, if
+  present, otherwise return null.</p></li>
+
+ <li>
+  <p>Let <var>topLevelOrigin</var> be <var>environmentSettings</var>'s top-level origin and
+  <var>origin</var> be <a for="environment settings object">origin</a>.</p></li>
+
+ <li>
+  <p>Let <var>topLevelOriginAgentClusterKey</var> be the result of <span
+   data-x="obtain-agent-cluster-key">obtaining an agent cluster key</span>, given

To cross link this, use `<a spec="HTML">obtaining an agent cluster key</a>`. Then we can make changes on the HTML side to allow you to remove the `spec="HTML"` (which is a way of overriding the fact that HTML currently doesn't export this term).

> +
+<p>For <dfn export>determining the HTTP cache partition</dfn>, given <var>request</var>,
+run these steps:
+
+<ol>
+ <li>
+  <p>Let <var>environmentSettings</var> be <var>request</var>'s <a for=request>client</a>, if
+  present, otherwise return null.</p></li>
+
+ <li>
+  <p>Let <var>topLevelOrigin</var> be <var>environmentSettings</var>'s top-level origin and
+  <var>origin</var> be <a for="environment settings object">origin</a>.</p></li>
+
+ <li>
+  <p>Let <var>topLevelOriginAgentClusterKey</var> be the result of <span
+   data-x="obtain-agent-cluster-key">obtaining an agent cluster key</span>, given

@annevk may have editorial feedback on this---e.g. in the past he's wanted to create a first-class "site" concept, instead of using the term "agent cluster key". But, we can worry about that after we get the processing model set up.

>  
-<p class="note no-backref">The user agent does not update the entry in the HTTP cache for a
-<a for=/>request</a> if request's cache mode is "no-store" or a
-`<code>Cache-Control: no-store</code>` header appears in the response.
+<p class="note no-backref">The user agent does not update the entry in the HTTP cache if request's
+cache mode is "no-store" or a `<code>Cache-Control: no-store</code>` header appears in the response.

Why make these two changes?

> @@ -1417,42 +1417,45 @@ Unless stated otherwise, it is unset.
 "<code>no-cache</code>", "<code>force-cache</code>", or
 "<code>only-if-cached</code>". Unless stated otherwise, it is "<code>default</code>".
 
+<p>Let <var>httpCache</var> be the result of <a>determining the HTTP cache partition</a> for

Ah, I see, you do include it below. Yeah, then just remove all changes to this section. This section is defining concepts and doesn't need to get into these details.

> @@ -4303,14 +4332,19 @@ Range Requests</cite>. [[HTTP-RANGE]] However, this is not widely supported by b
     <var>httpRequest</var>'s
     <a for=request>credentials mode</a>.
 
+   <li>
+    <p>Let <var>httpCache</var> be the result of <a>determining the HTTP cache partition</a> for
+    <var>httpRequest</var>. If <var>httpCache</var> is null, fetch behaves as if there is no HTTP cache
+    at all and the <a>cache mode</a> is considered "<code>no-cache</code>".

"fetch behaves as if there is no HTTP cache at all and the <a>cache mode</a> is considered "<code>no-cache</code>"." is not precise enough. You should add a second step which says something like

```html
If <var>httpCache</var> is null, then set <var>httpRequest</var>'s <a for=request>cache mode</a> to "<code>no-cache</code>".
```

>     <li>
     <p>If <var>httpRequest</var>'s <a for=request>cache mode</a> is neither "<code>no-store</code>"
     nor "<code>reload</code>", then:
 
     <ol>
      <li>
-      <p>Set <var>storedResponse</var> to the result of selecting a response from the HTTP cache,
-      possibly needing validation, as per the
+      <p>Set <var>storedResponse</var> to the result of selecting a response from the
+      <var>httpCache</var>, possibly needing validation, as per the

This step will be reached even if the mode is no-cache. Is that desired? Maybe no-cache is not correct, and it should be no-store instead?

Your goal is to prevent any steps that reference _httpCache_ from being reached if _httpCache_ is null, because that would cause a null pointer exception.

> @@ -4414,8 +4448,8 @@ Range Requests</cite>. [[HTTP-RANGE]] However, this is not widely supported by b
    <li><p>If <var>httpRequest</var>'s <a for=request>method</a> is
    <a href=https://tools.ietf.org/html/rfc7231#safe.methods>unsafe</a> and
    <var>forwardResponse</var>'s <a for=response>status</a> is in the range <code>200</code> to
-   <code>399</code>, inclusive, 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
+   <code>399</code>, inclusive, invalidate appropriate stored responses in the <var>httpCache</var>,

```suggestion
   <code>399</code>, inclusive, invalidate appropriate stored responses in <var>httpCache</var>,
```

> @@ -4442,8 +4476,8 @@ Range Requests</cite>. [[HTTP-RANGE]] However, this is not widely supported by b
      <li><p>Set <var>response</var> to <var>forwardResponse</var>.
 
      <li>
-      <p>Store <var>httpRequest</var> and <var>forwardResponse</var> in the HTTP cache, as per the
-      "<a href=https://tools.ietf.org/html/rfc7234#section-3>Storing Responses in Caches</a>"
+      <p>Store <var>httpRequest</var> and <var>forwardResponse</var> in the <var>httpCache</var>, as

```suggestion
      <p>Store <var>httpRequest</var> and <var>forwardResponse</var> in <var>httpCache</var>, as
```

> @@ -4692,7 +4726,7 @@ Range Requests</cite>. [[HTTP-RANGE]] However, this is not widely supported by b
    <li><p>If <var>response</var> is not a
    <a>network error</a> and <var>request</var>'s
    <a for=request>cache mode</a> is not "<code>no-store</code>",
-   update <var>response</var> in the HTTP cache for <var>request</var>.
+   update <var>response</var> in the <var>httpCache</var> for <var>request</var>.

```suggestion
   update <var>response</var> in <var>httpCache</var> for <var>request</var>.
```

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/whatwg/fetch/pull/943#pullrequestreview-366035588
Received on Thursday, 27 February 2020 22:20:09 UTC