Removed %IDENT format code with Ident protocol support.
+
Squid no longer revalidates responses to collapsed requests, treating
+ all such responses as fresh. This change follows IETF HTTP Working Group
+ advice (in an HTTP gray area) and prevents arguably excessive freshness
+ checks for responses to collapsed requests. This change does not prevent
+ freshness checks for responses that were, at the time of a hit request,
+ either fully cached or still receiving response body bytes.
+
Instead of ignoring quick_abort_pct settings that would, together with other conditions, abort a pending download of a 99-byte or diff --git a/src/StoreClient.h b/src/StoreClient.h index b0a83c34624..d534af970a0 100644 --- a/src/StoreClient.h +++ b/src/StoreClient.h @@ -35,6 +35,7 @@ class StoreEntry; class ACLFilledChecklist; class LogTags; +// TODO: Merge store_client into StoreClient. /// a storeGetPublic*() caller class StoreClient: public Acl::ChecklistFiller { @@ -58,6 +59,9 @@ class StoreClient: public Acl::ChecklistFiller bool mayInitiateCollapsing() const { return onCollapsingPath(); } /// whether Squid configuration allows collapsing for this transaction bool onCollapsingPath() const; + + /// whether startCollapsingOn() was called and returned true + mutable bool didCollapse = false; }; #if USE_DELAY_POOLS diff --git a/src/cf.data.pre b/src/cf.data.pre index d6e03f5fd3b..c9da24e317b 100644 --- a/src/cf.data.pre +++ b/src/cf.data.pre @@ -6448,7 +6448,9 @@ LOC: Config.accessList.sendHit DOC_START Responses denied by this directive will not be served from the cache (but may still be cached, see store_miss). This directive has no - effect on the responses it allows and on the cached objects. + effect on the responses it allows and on the cached objects. This + directive is applied to both regular from-cache responses and responses + reused by collapsed requests (see collapsed_forwarding). Please see the "cache" directive for a summary of differences among store_miss, send_hit, and cache directives. @@ -7237,6 +7239,11 @@ DOC_START requests hitting a stale cached object. Revalidation collapsing is currently disabled for Squid instances containing SMP-aware disk or memory caches and for Vary-controlled cached objects. + + A response reused by the collapsed request is deemed fresh in that + request processing context -- Squid does not apply refresh_pattern and + internal freshness validation checks to collapsed transactions. Squid + does apply send_hit rules. DOC_END NAME: collapsed_forwarding_access diff --git a/src/client_side_reply.cc b/src/client_side_reply.cc index 66c53916111..5f589b7d20c 100644 --- a/src/client_side_reply.cc +++ b/src/client_side_reply.cc @@ -627,7 +627,7 @@ clientReplyContext::cacheHit(const StoreIOBuffer result) http->updateLoggingTags(LOG_TCP_MISS); processMiss(); return; - } else if (!r->flags.internal && refreshCheckHTTP(e, r)) { + } else if (!r->flags.internal && !didCollapse && refreshCheckHTTP(e, r)) { debugs(88, 5, "clientCacheHit: in refreshCheck() block"); /* * We hold a stale copy; it needs to be validated diff --git a/src/htcp.cc b/src/htcp.cc index e91851aa28d..a87af95d3fd 100644 --- a/src/htcp.cc +++ b/src/htcp.cc @@ -981,10 +981,10 @@ htcpSpecifier::checkHit() debugs(31, 3, "NO; public object not found"); } else if (!e->validToSend()) { debugs(31, 3, "NO; entry not valid to send" ); - } else if (refreshCheckHTCP(e, checkHitRequest.getRaw())) { - debugs(31, 3, "NO; cached response is stale"); } else if (e->hittingRequiresCollapsing() && !startCollapsingOn(*e, false)) { debugs(31, 3, "NO; prohibited CF hit: " << *e); + } else if (!didCollapse && refreshCheckHTCP(e, checkHitRequest.getRaw())) { + debugs(31, 3, "NO; cached response is stale"); } else { debugs(31, 3, "YES!?"); hit = e; diff --git a/src/icp_v2.cc b/src/icp_v2.cc index f759e89cb12..c1c3474d021 100644 --- a/src/icp_v2.cc +++ b/src/icp_v2.cc @@ -170,10 +170,10 @@ ICPState::confirmAndPrepHit(const StoreEntry &e) const if (!e.validToSend()) return false; - if (!Config.onoff.icp_hit_stale && refreshCheckICP(&e, request)) + if (e.hittingRequiresCollapsing() && !startCollapsingOn(e, false)) return false; - if (e.hittingRequiresCollapsing() && !startCollapsingOn(e, false)) + if (!Config.onoff.icp_hit_stale && !didCollapse && refreshCheckICP(&e, request)) return false; return true; diff --git a/src/refresh.cc b/src/refresh.cc index 81be28ae4b4..0e8db3bf201 100644 --- a/src/refresh.cc +++ b/src/refresh.cc @@ -523,6 +523,10 @@ refreshIsCachable(const StoreEntry * entry) * minimum_expiry_time seconds delta (defaults to 60 seconds), to * avoid objects which expire almost immediately, and which can't * be refreshed. + * + * No hittingRequiresCollapsing() or didCollapse concerns here: This + * incoming response is fresh now, but we want to check whether it can be + * refreshed Config.minimum_expiry_time seconds later. */ int reason = refreshCheck(entry, nullptr, Config.minimum_expiry_time); ++ refreshCounts[rcStore].total; @@ -569,6 +573,10 @@ refreshIsStaleIfHit(const int reason) * * \retval 1 if STALE * \retval 0 if FRESH + * + * Do not call this when StoreClient::didCollapse is true. XXX: Callers should + * not have to remember to check didCollapse. TODO: Refactor by adding something + * like pure virtual StoreClient::refreshCheck() with protocol specializations? */ int refreshCheckHTTP(const StoreEntry * entry, HttpRequest * request) @@ -577,7 +585,6 @@ refreshCheckHTTP(const StoreEntry * entry, HttpRequest * request) ++ refreshCounts[rcHTTP].total; ++ refreshCounts[rcHTTP].status[reason]; request->flags.staleIfHit = refreshIsStaleIfHit(reason); - // TODO: Treat collapsed responses as fresh but second-hand. return (Config.onoff.offline || reason < 200) ? 0 : 1; } diff --git a/src/store_client.cc b/src/store_client.cc index 12b730fc4fc..e24a6e192cd 100644 --- a/src/store_client.cc +++ b/src/store_client.cc @@ -80,6 +80,7 @@ StoreClient::startCollapsingOn(const StoreEntry &e, const bool doingRevalidation tags->collapsingHistory.otherCollapses++; } + didCollapse = true; debugs(85, 5, e << " doingRevalidation=" << doingRevalidation); return true; } diff --git a/src/store_digest.cc b/src/store_digest.cc index 3cf348ad4eb..3fb1c76f019 100644 --- a/src/store_digest.cc +++ b/src/store_digest.cc @@ -260,6 +260,10 @@ storeDigestAddable(const StoreEntry * e) } /* still here? check staleness */ + // Include hittingRequiresCollapsing() entries: They are fresh _now_, but we + // check future freshness. They should not have enough info to judge future + // freshness since we are still waiting for their response headers, but + // admins might configure Squid to consider such entries fresh anyway. /* Note: We should use the time of the next rebuild, not (cur_time+period) */ if (refreshCheckDigest(e, Config.digest.rebuild_period)) { debugs(71, 6, "storeDigestAdd: entry expires within " << Config.digest.rebuild_period << " secs, ignoring"); diff --git a/src/urn.cc b/src/urn.cc index ff88aae4d54..934d023712e 100644 --- a/src/urn.cc +++ b/src/urn.cc @@ -282,6 +282,8 @@ urnHandleReply(void *data, StoreIOBuffer result) return; } + // XXX: Missing reply freshness checks (e.g., calling refreshCheckHTTP()). + urls = urnParseReply(urnState->parsingBuffer.toSBuf(), urnState->request->method); if (!urls) { /* unknown URN error */ diff --git a/test-suite/test-functionality.sh b/test-suite/test-functionality.sh index eb191bc0cc9..d325e764bcd 100755 --- a/test-suite/test-functionality.sh +++ b/test-suite/test-functionality.sh @@ -220,6 +220,7 @@ main() { upgrade-protocols cache-response proxy-collapsed-forwarding + hit-revalidation busy-restart truncated-responses malformed-request