Re: [PATCH] HTTP/1.1 response caching upgrade

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
Received on Mon Oct 15 2012 - 23:31:28 MDT