Re: [PATCH] HTTP/1.1 response caching upgrade

Updated patch with requested changes.

Amos

On 16/10/2012 12:31 p.m., Amos Jeffries wrote:
> On 16.10.2012 09:56, Alex Rousskov wrote:
>> On 10/14/2012 01:17 AM, Amos Jeffries wrote:
>>> On prompting from bug 3670 which outlines how Auth transactions are now
>>> wrongly non-cacheable. I have performed a small audit of the
>>> HttpStateData::cacheableReply() method and this patch contains the
>>> result.
>>>
>>> trunk rev 11361 converted Cache-Control header from using a single mask
>>> bitmap (shared by request and response) to separate CC header
>>> objects in
>>> the request response. This conversion contained several regressions
>>> like
>>> the one bug 3670 reports.
>>>
>>> This patch (on trunk rev.12394):
>>> * documents HttpStateData::cacheableReply() clarifying the overall
>>> method action and what each individual check it doing.
>>> * resolves several visible regressions, including bug 3670.
>>> * extends the caching to handle the "no-cache" controls as per
>>> HTTP/1.1
>>> (MAY store, but MUST revalidate before use).
>>> * extends the caching for several lesser known cases of "MAY store"
>>> exemptions handling authenticated transactions.
>>>
>>> One side effect of now caching transactions utilizing "no-cache" is
>>> that
>>> hacks around Pragma:no-cache are reduced to only having any effect when
>>> Cache-Control is absent. Reducing their performance cost.
>>>
>>> This should give Squid a major boost to both its caching compliance and
>>> HIT ratios.
>>>
>>> The patch is build tested, but not yet run-time tested or HTTP/1.1
>>> compliance tested.
>>
>>
>> Hi Amos,
>>
>> Thank you for fixing this. I wish you did not re-indented the old
>> checks in the patch though -- it complicates reviewing logic changes.
>>
>>
>>> HttpReply const *rep = finalReply();
>>> +
>>> + // non-existent replies are not cacheable.
>>> + if (!rep)
>>> + return 0;
>>
>> ServerStateData::finalReply() is guaranteed to return a non-nil pointer.
>> We have other code relying on that guarantee. There is no need to double
>> check IMO.
>>
>
> Removed.
>
>>
>>> + if (!ignoreCacheControl) {
>> ...
>>> + // RFC 2068, sec 14.9.4 - MUST NOT cache any response with
>>> Authentication UNLESS certain CC controls are present
>>> + // allow HTTP violations to IGNORE those controls (ie
>>> re-block caching Auth)
>>> + if (request && (request->flags.auth ||
>>> request->flags.authSent) && !REFRESH_OVERRIDE(ignore_auth)) {
>>> + if (!rep->cache_control)
>>> + return 0;
>>
>> Guarding authentication check with ignoreCacheControl looks odd to me.
>> Should not the check be moved outside the guard, while possibly ignoring
>> cache_control inside the check? That is:
>>
>> // RFC ... MUST NOT cache any response with Authentication
>> if (response with authentication) {
>> // UNLESS certain CC controls are present
>> if (!rep->cache_control || ignoreCacheControl)
>> return 0;
>> ... see if cache controls allow caching ...
>> }
>>
>> if (!ignoreCacheControl) {
>> ...
>>
>> The authentication blob can be moved below the ignoreCacheControl blob
>> if we think authentication is, on average, more rare than conditions
>> tested by other CC checks. Since both blobs never return 1, it is OK to
>> order them as we please.
>
> You are right. Fixed.
>
>>
>>
>>> + String s = rep->header.getList(HDR_PRAGMA);
>>> + const int no_cache = strListIsMember(&s,
>>> "no-cache", ',');
>>> + s.clean();
>>> +
>>> + if (no_cache)
>>> + EBIT_SET(entry->flags, ENTRY_REVALIDATE);
>>
>> Consider using HttpHeader::hasListMember() instead of getList() plus
>> strListIsMember(). If you have to use strListIsMember(), please note
>> that String cleans up itself during destruction so the above can be
>> simplified:
>>
>> const String s = rep->header.getList(HDR_PRAGMA);
>> if (strListIsMember(&s, "no-cache", ','))
>> EBIT_SET(entry->flags, ENTRY_REVALIDATE);
>>
>
> Done.
>
>>
>> Finally, if you are rewriting so much code anyway, consider returning a
>> "const char *" reason string and make the caller log it with debugs(). A
>> nil result would indicate a cachable response. The source code comments
>> are great, but it is often difficult to understand the decision when
>> looking at a debugging log. Polishing this may be viewed outside this
>> patch scope though (and I would probably not even suggest it if you kept
>> the old indentation for the review :-).
>
> We can't return a char* because this function returns 0/1/-1 cases.
> I've threaded debugs() before each return statement though for the
> same effect.
>
>
> New patch coming after new build tests.
>
> Amos