Re: [PATCH] support parameters for no-cache and private

On 23/02/2013 10:20 a.m., Alex Rousskov wrote:
> On 02/21/2013 06:26 PM, Amos Jeffries wrote:
>
>> Adjusted patch to drop the odd NP, rework CC:private operation on broken
>> parameters, and fix the segfault.
> Hi Amos,
>
> Thank you for addressing most of my concerns.
>
>> + } else if (/* p &&*/ !httpHeaderParseQuotedString(p, (ilen-nlen-1), &private_)) {
>> + debugs(65, 2, "cc: invalid private= specs near '" << item << "'");
>> + clearPrivate();
>> + }
> This still does not work correctly when there are multiple valid
> CC:private=".." header fields in a message header because the
> httpHeaderParseQuotedString() clears the previous private_ contents, right?
>
> You can fix this by parsing into a local String variable and then
> appending that variable contents to the previously stored headers using
> the now-fixed Private() method. This approach will also eliminate the
> need to call clearPrivate() on failures and then immediately undo that
> with setMask(true,true).
>
> Same concern for no-cache="..." parsing.

Hmm. Right and these two are different from the other options in that
the others are singleton options. Will do.

>
>
>> + } else {
>> + debugs(65, 2, "cc: invalid no-cache= specs near '" << item << "'");
>> + clearNoCache();
>> + }
> I still do not think we should ignore the whole CC:no-cache just because
> we cannot parse something, especially when there was a perfectly valid
> parameterless CC:no-cache before we failed to parse something. Instead,
> we should conservatively treat the whole thing as CC:no-cache.

In the no-cache cases the recovery action is not critical like private
might be. All we risk with this is a cache HIT vs REFRESH.
Still I'm not partial either way. Will do.

> In other words,
>
> Cache-control: no-cache
> Foo: bar
> Cache-control: no-cache="foo
>
> should be treated as
>
> Cache-control: no-cache
> Foo: bar
>
> rather than
>
> Foo: bar
>
>
> You obviously disagree. Hopefully somebody will break this tie.

All these permutations have been worrying me a little.

>
>> // NP: request CC:no-cache only means cache READ is forbidden. STORE is permitted.
>> + if (rep->cache_control && rep->cache_control->hasNoCache() && rep->cache_control->noCache().defined()) {
>> + /* TODO: we are allowed to cache when no-cache= has parameters.
>> + * Provided we strip away any of the listed headers unless they are revalidated
>> + * successfully (ie, must revalidate AND these headers are prohibited on stale replies).
>> + * That is a bit tricky for squid right now so we avoid caching entirely.
>> + */
>> + debugs(22, 3, HERE << "NO because server reply Cache-Control:no-cache has parameters");
>> + return 0;
>> + }
>> +
>> // NP: request CC:private is undefined. We ignore.
>> // NP: other request CC flags are limiters on HIT/MISS. We don't care about here.
> Please move the new code below all three NPs. As rendered now, the first
> NP seems to apply to the if-statement, which confused me a lot. It is
> also good to keep request CC code/comments together so that "other
> request CC flags" comment makes more sense.
>
>
> On a separate note, the code became more confusing (IMO) because patched
> Squid now handles CC:no-cache in two places: Here, we handle the
> parameter case. The caller handles both parameter and parameterless
> cases by setting ENTRY_REVALIDATE. Fixing that is difficult, but we
> should at least clarify what is going on. I suggest using the following
> comment instead of the above TODO:
>
> /* We are allowed to store CC:no-cache. When CC:no-cache has no
> parameters, we must revalidate. The caller does that by setting
> ENTRY_REVALIDATE. TODO: When CC:no-cache has parameters, strip away the
> listed headers instead of revalidating (or when revalidation fails). For
> now, avoid caching entirely because we do not strip on failed
> revalidations. */
>
> This will point to the other critical piece of code AND explain why
> parameter case is treated "worse" than the parameterless one despite
> being "better" from protocol intent point of view.
>
> If my "because we do not strip on failed revalidations" explanation is
> not correct, please let me know.

Okay.

>
>>> Since we now support HTTP/1.1 storage and revalidation of
>>> Cache-Control:no-cache it is important that we at least detect the
>>> cases where no-cache= and private= contain parameters and handle them
>>> properly whenever possible.
>>>
>>> AFAIK these are still rare occurances due to the historic lack of
>>> support. So for now Squid just detects and exempts these responses
>>> from the caching performed. The basic framework for adding future
>>> support of these HTTP/1.1 features is made available
>>>
>> Please run this past Co-Advisor to confirm the private="..." and
>> no-cache="..." cases are now all "Precondition Failed".
> Confirmed. Please note that CC:private="..." cases were failing
> precondition before the patch as well. Apparently, somebody fixed
> CC:private handling some time after r12394 and before your patch.

Yes I know.

Amos
Received on Sat Feb 23 2013 - 01:56:38 MST