[squid-dev] Fix If-None-Match processing and related bug 4169

[Amos Jeffries]

On 1/12/2016 12:44 a.m., Garri Djavadyan wrote:
> On Tue, 2016-11-29 at 15:42 -0700, Alex Rousskov wrote:
>> On 11/29/2016 02:23 PM, Amos Jeffries wrote:
>>>
>>> On 30/11/2016 1:47 a.m., Garri Djavadyan wrote:
>>>>
>>>> On Tue, 2016-11-29 at 14:51 +0500, Garri Djavadyan wrote:
>>>>>
>>>>> Hello,
>>>>>
>>>>> Please review the attached patch prepared for r14958, it fixes
>>>>> the
>>>>> If-
>>>>> None-Match processing (incorrect logging [1]) and the bug [2]
>>>>> report
>>>>> 4169 depending on the incorrect (IMO) behavior.
>>>>>
>>>>> An If-None-Match request for a non-matched (ETag) but cached
>>>>> object
>>>>> should be processed as normal HIT.
>>>>>
>>>>> [1] http://lists.squid-cache.org/pipermail/squid-users/2016-Nov
>>>>> ember/013483.html
>>>>> [2] http://bugs.squid-cache.org/show_bug.cgi?id=4169
>>
>>>
>>>>
>>>> In the first version I
>>>> removed strings (not related to the fix) which mark a request as
>>>> non-
>>>> IMS, because I thought the same request could not
>>>> enter processConditional twice and the removed actions are
>>>> redundant.
>>
>> I agree that the second version is better: Unless you are absolutely
>> sure, it is best to stay focused on the problem and leave the IMS
>> deletion code "as is" even if it may not be necessary. If you still
>> suspect it is not necessary, add a TODO comment to investigate
>> separately.
>>
>>
>>>
>>> Checking this whole processConditional() sequence for compliance
>>> with
>>> RFC 7232 and 7234 the rest of the logic seems to be fine except
>>> this case:
>>>
>>> <https://tools.ietf.org/html/rfc7234#section-4.3.2>
>>> "
>>>    If the field-value is "*", or if the
>>>    field-value is a list of entity-tags and at least one of them
>>> matches
>>>    the entity-tag of the selected stored response,
>>
>>>
>>> [ e->hasIfNoneMatchEtag(r) == false --> !N == true ]
>>
>> I am not sure I am interpreting your [...] mnemonic correctly, but
>> AFAICT, the patched code works when the RFC conditions quoted above
>> are
>> _not_ meant:
>>

Arg. You are right. Double not-non-negations strikes again :-(

<snip>
> 
> Alex, Amos thanks for comments! I've attached third version of the
> patch. It is based on the proposal by Alex. The patch introduces
> following changes:
> 
> * Matched If-None-Match requests are logged as TCP_INM_HIT (proposed by
> Amos)
> * If-Modified-Since requests for modified cached objects are processed
> as normal HITs
> * Not matched If-None-Match requests for cached objects are processed
> as normal HITs
> * If-Modified-Since header is ignored if If-None-Match header exists
> (RFC7232 compliance) 
> 
> I've tested different conditional requests (including 'If-None-Match:
> *' and 'If-None-Match: "matched-tag", "not-matched-tag"') and found no
> problems. Thanks.
> 

+1. That looks better to me.

I would really like the LogTags flag to be an actual flag, but I can do
that as a followup or later when I convert the others to flags.

Amos