Re: [PATCH] HTTP/1.1 response caching upgrade

Amos Jeffries <squid3_at_treenet.co.nz> writes:

> On 24.10.2012 09:11, Dmitry Kurochkin wrote:
>> Hi Amos.
>>
>> Amos Jeffries <squid3_at_treenet.co.nz> writes:
>>
>>> On 18.10.2012 04:42, Alex Rousskov wrote:
>>>> On 10/16/2012 03:24 AM, Amos Jeffries wrote:
>>>>> Updated patch with requested changes.
>>>>
>>>> Looks OK to me although I have not verified each individual CC
>>>> handling
>>>> line.
>>>>
>>>>> + if ((mayStore |= rep->cache_control->Public())) {
>>>>> + } else if ((mayStore |=
>>>>> (rep->cache_control->mustRevalidate() &&
>>>>> !REFRESH_OVERRIDE(ignore_must_revalidate)) )) {
>>>>> + } else if ((mayStore |= (rep->cache_control->noCache() &&
>>>>> !REFRESH_OVERRIDE(ignore_no_cache)) )) {
>>>>> + } else if ((mayStore |= rep->cache_control->sMaxAge())) {
>>>>> + }
>>>>
>>>> If mayStore starts as false and is modified only once, I would
>>>> replace
>>>> "|=" cleverness with a simple assignment.
>>>
>>> Okay. Done.
>>>
>>>>
>>>>> + debugs(22, 3, HERE << "YES because Authenticated and
>>>>> server reply Cache-Control:public");
>>>>> + debugs(22, 3, HERE << "YES because Authenticated and
>>>>> server reply Cache-Control:public");
>>>>> + debugs(22, 3, HERE << "YES because Authenticated and
>>>>> server reply Cache-Control:no-cache");
>>>>> + debugs(22, 3, HERE << "YES because Authenticated and
>>>>> server reply Cache-Control:s-maxage");
>>>>
>>>> These should not be "YES" because we may end up returning 0 or -1
>>>> later
>>>> anyway. We are already using "MAYBE" for -1 case. I suggest
>>>> replacing
>>>> "YES because Authenticated and ..." text with "Authenticated but
>>>> ..."
>>>> text.
>>>
>>> Sure. Done.
>>>
>>>>
>>>> These polishing touches do not require another round of review as
>>>> far
>>>> as
>>>> I am concerned.
>>>>
>>>> Do you want us to run this by Co-Advisor before the commit?
>>>
>>> From the wiki coadvisor checklist I noticed the auth and pragma
>>> cases
>>> are very under-represented and I'm only expecting 3-5 cases to
>>> change
>>> from #! or V from this despite the great leap in cacheable content
>>> (which is more based on prevalence of no-cache). If it's not going
>>> to
>>> take long that would be great. I'll hold this until tomorrow then
>>> apply
>>> and release 3.2.3 and 3.3.0.1 sometime over the weekend.
>>>
>>
>> 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).

Regards,
  Dmitry

> Amos
Received on Tue Oct 23 2012 - 21:56:29 MDT