Re: [PATCH] HTTP/1.1 response caching upgrade

On 24.10.2012 10:56, Dmitry Kurochkin wrote:
> Amos Jeffries <squid3_at_treenet.co.nz> writes:
>
>> On 24.10.2012 09:11, Dmitry Kurochkin wrote:
>>> Hi Amos.
>>>
<snip>
>>> Co-Advisor results look good. No regressions. The following test
>>> cases
>>> succeed now:
>>>
>>> * shared cache MUST use fresh request-headers when validating
>>> Authorization responses with must-revalidate cache-directive
>>> * shared cache MUST use fresh request-headers when validating
>>> Authorization responses with proxy-revalidate cache-directive
>>> * shared cache MUST use fresh request-headers when validating
>>> Authorization responses with s-maxage=0 cache-directive
>>> * cache MUST NOT cache entity with no-store Cache-Control directive
>>> * shared cache MUST NOT cache entity with private Cache-Control
>>> directive
>>>
>>> And the following tests changed from violation to precondition
>>> failure:
>>>
>>> * cache MUST NOT cache listed response headers of entity with a
>>> "Cache-Control: private=list" header(s); using single listed header
>>> with one field value
>>> * cache MUST NOT cache listed response headers of entity with a
>>> "Cache-Control: private=list" header(s); using single listed header
>>> with multiple field values
>>> * cache MUST NOT cache listed response headers of entity with a
>>> "Cache-Control: private=list" header(s); using multiple listed
>>> headers
>>> * cache MUST NOT cache listed response headers of entity with a
>>> "Cache-Control: private=list" header(s); using multiple
>>> Cache-Control
>>> headers
>>
>> Hmm. Okay, what exactly does precondition failure mean for these?
>> That we should have cached them but revalidated?
>>
>
> Yes. Without the patch, these replies were cached and served from
> cache. Hence the violation. Now they are not cached, hence
> precondition failure. Which is fine, since spec does not require us
> to
> cache such replies.
>
>> There is a bit of a clash here in the WG drafts.
>> * pt 6 section 7.2.2.2 states we; MAY store the remainder of the
>> response (other than the specific headers)
>> * pt 6 section 3 states we; MUST NOT store if the directive is
>> present
>> (no mention of a parameter being relevant to storage).
>>
>> (Thank you for making me look at this, another WGLC issue :-)
>>
>
> My pleasure :)
>
>>>
>>> Note that there are similar tests for CC no-cache header that still
>>> result in violation. Perhaps they should also be fixed by this
>>> patch
>>> or
>>> a follow-up one? Let me know if you need more details.
>>
>> Yes please more details are good. You mean no-cache with parameters?
>> or
>> no-cache under authentication?
>>
>
> No-cache with parameters, no authentication.
>
>> I added "#if 0" around the Auth & no-cache cases because I was
>> unable
>> to find any specific text mentioning them in the WG drafts. If you
>> have
>> found some please point me at for reference in the WGLC issue I'm
>> composing.
>>
>>
>> For no-cache with parameters. It does not make much sense for a
>> shared
>> cache to treat it any differently in the general case than no-cache
>> without parameters due to irrelevancy of revalidating just a few
>> headers
>> instead of the whole request. It does kind of makes sense when
>> combined
>> with the stale-while-revalidate/stale-if-error from RFC 5861 to keep
>> some headers private while passing on the rest, and we might hit
>> issues
>> with stale-if-error now in the rare case when both are combined
>> (stale-while-revalidate not being supported yet).
>>
>
> Currently, Squid caches replies with CC no-cache with parameters. It
> does the same for replies with CC private with parameters, but your
> patch fixes that. The test cases are:
>
> * cache MUST NOT cache listed response headers of entity with a
> "Cache-Control: no-cache=list" header(s); using single listed header
> with one field value
> * cache MUST NOT cache listed response headers of entity with a
> "Cache-Control: no-cache=list" header(s); using single listed header
> with multiple field values
> * cache MUST NOT cache listed response headers of entity with a
> "Cache-Control: no-cache=list" header(s); using multiple listed
> headers
> * cache MUST NOT cache listed response headers of entity with a
> "Cache-Control: no-cache=list" header(s); using multiple
> Cache-Control
> headers
>
> All of these result in violation with 'cache served cached entity
> with a
> "Cache-Control: no-cache=list" header(s) and did not exclude listed
> response header(s)' reason.
>
> The tests are the same as CC private cases fixed by your patch,
> except
> for the CC header obviously. I can send you logs with all requests
> and
> replies if you would like.
>
> Note that Squid r11186 did not cache replies with CC private and
> no-cache with parameters. But it started caching them since at least
> r12294 (that is when test cases results changed from precondition
> failure to violation).

And just to confirm I'm understanding you right. The no-cache cases are
now also back to "precondition failed"?

Meaning we are still missing two CC features, but are no longer
violating badly.

Amos
Received on Tue Oct 23 2012 - 22:13:59 MDT