Re: [PATCH] HTTP/1.1 response caching upgrade

Amos Jeffries <squid3_at_treenet.co.nz> writes:

> 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"?
>

No, your patch changed CC private cases back to "precondition failed"
but CC no-cache cases still result in violation.

Regards,
  Dmitry

> Meaning we are still missing two CC features, but are no longer
> violating badly.
>
> Amos
Received on Tue Oct 23 2012 - 22:19:01 MDT