Re: [PATCH] HTTP/1.1 response caching upgrade

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?

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 :-)

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

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).

Amos
Received on Tue Oct 23 2012 - 21:30:13 MDT