Re: [PATCH] HTTP/1.1 response caching upgrade

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

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.

Regards,
  Dmitry

> Amos
Received on Tue Oct 23 2012 - 20:11:46 MDT