diff --git a/doc/release-notes/release-7.sgml.in b/doc/release-notes/release-7.sgml.in
index f33c9992d67..690a8db00e4 100644
--- a/doc/release-notes/release-7.sgml.in
+++ b/doc/release-notes/release-7.sgml.in
@@ -194,6 +194,14 @@ This section gives an account of those changes in three categories:
external_acl_type
Removed %IDENT format code with Ident protocol support.
+ collapsed_forwarding
+
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.
+
Removed directives
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 fbd74460399..d99533c79ef 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.
@@ -7235,6 +7237,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 6288b0dcbf8..bbc6e590ed8 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