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:
>
> >
> > if (r.header.has(Http::HdrType::IF_NONE_MATCH)) {
> > if (!e->hasIfNoneMatchEtag(r)) {
> > // RFC 2616: ignore IMS if If-None-Match did not match
> > r.flags.ims = false;
> > r.ims = -1;
> > r.imslen = 0;
> > r.header.delById(Http::HdrType::IF_MODIFIED_SINCE);
> > http->logType = LOG_TCP_MISS;
> > sendMoreData(result);
> > return true;
> > }
>
>
> In other words, hasIfNoneMatchEtag() returns false and, hence, the
> quoted code is executed when _none_ of the requested ETags matches
> the
> cached ETag. The "if If-None-Match did not match" comment inside that
> code agrees with this interpretation.
>
> Thus, the recommended RFC action that you found does _not_ apply to
> the
> fixed code:
>
> >
> > a cache recipient
> > SHOULD generate a 304 (Not Modified) response (using the
> > metadata of
> > the selected stored response) instead of sending that stored
> > response.
> > "
>
>
> If you were talking about processConditional() code outside the if
> statement fixed by Garri (i.e., when hasIfNoneMatchEtag() returns
> true),
> then it appears to match the quoted requirement only if there is no
> IMS
> header:
>
> >
> > if (!r.flags.ims) {
> > // RFC 2616: if If-None-Match matched and there is no IMS,
> > // reply with 304 Not Modified or 412 Precondition Failed
> > sendNotModifiedOrPreconditionFailedError();
> > return true;
> > }
> >
> > // otherwise check IMS below to decide if we reply with 304 or 412
> > matchedIfNoneMatch = true;
>
>
> If there is an IMS header, then Squid violates the following RFC 7232
> MUST:
>
> >
> > A recipient MUST ignore If-Modified-Since if the request contains
> > an
> > If-None-Match header field
>
> The HTTP requirements changed here since RFC 2616, and we probably
> should adjust the code to delete IMS header/flags after discovering
> the
> If-None-Match header:
>
> if (r.header.has(Http::HdrType::IF_NONE_MATCH)) {
> // RFC 7232: If-None-Match recipient MUST ignore IMS
> r.flags.ims = false;
> r.ims = -1;
> r.imslen = 0;
> r.header.delById(Http::HdrType::IF_MODIFIED_SINCE);
>
> if (e->hasIfNoneMatchEtag(r)) {
> sendNotModifiedOrPreconditionFailedError();
> return true;
> }
>
> // If-None-Match did not match; treat as an unconditional hit
> return false;
> }
>
> If the above sketch is correct, then the matchedIfNoneMatch variable
> may
> become unnecessary.
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.
Garri
=== modified file 'src/LogTags.cc'
--- src/LogTags.cc 2016-10-08 22:19:44 +0000
+++ src/LogTags.cc 2016-11-30 09:46:54 +0000
@@ -21,6 +21,7 @@
"TCP_REFRESH",
"TCP_CLIENT_REFRESH_MISS",
"TCP_IMS_HIT",
+ "TCP_INM_HIT",
"TCP_SWAPFAIL_MISS",
"TCP_NEGATIVE_HIT",
"TCP_MEM_HIT",
@@ -73,6 +74,7 @@
return
(oldType == LOG_TCP_HIT) ||
(oldType == LOG_TCP_IMS_HIT) ||
+ (oldType == LOG_TCP_INM_HIT) ||
(oldType == LOG_TCP_REFRESH_FAIL_OLD) ||
(oldType == LOG_TCP_REFRESH_UNMODIFIED) ||
(oldType == LOG_TCP_NEGATIVE_HIT) ||
=== modified file 'src/LogTags.h'
--- src/LogTags.h 2016-10-08 22:19:44 +0000
+++ src/LogTags.h 2016-11-30 07:11:46 +0000
@@ -28,6 +28,7 @@
LOG_TCP_REFRESH, // refresh from origin started, but still pending
LOG_TCP_CLIENT_REFRESH_MISS,
LOG_TCP_IMS_HIT,
+ LOG_TCP_INM_HIT,
LOG_TCP_SWAPFAIL_MISS,
LOG_TCP_NEGATIVE_HIT,
LOG_TCP_MEM_HIT,
=== modified file 'src/client_side.cc'
--- src/client_side.cc 2016-11-18 18:08:30 +0000
+++ src/client_side.cc 2016-11-30 07:13:38 +0000
@@ -233,6 +233,10 @@
statCounter.client_http.nearMissSvcTime.count(svc_time);
break;
+ case LOG_TCP_INM_HIT:
+ statCounter.client_http.nearMissSvcTime.count(svc_time);
+ break;
+
case LOG_TCP_HIT:
case LOG_TCP_MEM_HIT:
=== modified file 'src/client_side_reply.cc'
--- src/client_side_reply.cc 2016-11-11 04:28:06 +0000
+++ src/client_side_reply.cc 2016-11-30 11:40:07 +0000
@@ -796,40 +796,27 @@
return true;
}
- bool matchedIfNoneMatch = false;
if (r.header.has(Http::HdrType::IF_NONE_MATCH)) {
- if (!e->hasIfNoneMatchEtag(r)) {
- // RFC 2616: ignore IMS if If-None-Match did not match
+ if (e->hasIfNoneMatchEtag(r)) {
+ // RFC 7232: If-None-Match recipient MUST ignore IMS
r.flags.ims = false;
r.ims = -1;
r.imslen = 0;
r.header.delById(Http::HdrType::IF_MODIFIED_SINCE);
- http->logType = LOG_TCP_MISS;
- sendMoreData(result);
- return true;
- }
- if (!r.flags.ims) {
- // RFC 2616: if If-None-Match matched and there is no IMS,
- // reply with 304 Not Modified or 412 Precondition Failed
sendNotModifiedOrPreconditionFailedError();
return true;
}
- // otherwise check IMS below to decide if we reply with 304 or 412
- matchedIfNoneMatch = true;
+ // If-None-Match did not match; treat as an unconditional hit
+ return false;
}
if (r.flags.ims) {
// handle If-Modified-Since requests from the client
if (e->modifiedSince(r.ims, r.imslen)) {
- http->logType = LOG_TCP_IMS_HIT;
- sendMoreData(result);
-
- } else if (matchedIfNoneMatch) {
- // If-None-Match matched, reply with 304 Not Modified or
- // 412 Precondition Failed
- sendNotModifiedOrPreconditionFailedError();
+ // object modified; treat as an unconditional hit
+ return false;
} else {
// otherwise reply with 304 Not Modified
@@ -1989,7 +1976,12 @@
StoreEntry *e = http->storeEntry();
const time_t timestamp = e->timestamp;
HttpReply *const temprep = e->getReply()->make304();
- http->logType = LOG_TCP_IMS_HIT;
+ // log as TCP_INM_HIT if code 304 generated for
+ // If-None-Match request
+ if (!http->request->flags.ims)
+ http->logType = LOG_TCP_INM_HIT;
+ else
+ http->logType = LOG_TCP_IMS_HIT;
removeClientStoreReference(&sc, http);
createStoreEntry(http->request->method, RequestFlags());
e = http->storeEntry();
_______________________________________________
squid-dev mailing list
[email protected]
http://lists.squid-cache.org/listinfo/squid-dev
