From: DrDaveD <2129743+DrDaveD@users.noreply.github.com>
Date: Wed, 18 Mar 2020 17:34:45 +0000 (+0000)
Subject: Bug 5030: Negative responses are never cached (#566)
X-Git-Tag: 4.15-20210522-snapshot~153
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=91870bf;p=thirdparty%2Fsquid.git

Bug 5030: Negative responses are never cached (#566)

Negative caching was blocked by checkCachable().

Since 3e98df2, Squid cached ENTRY_NEGCACHED entries in memory cache
only. Back then, storeCheckSwapable() prevented what later became
ENTRY_NEGCACHED entries from going to disk. The design was obscured by
8350fe9 that renamed storeCheckSwapable() to storeCheckCachable().

Commit 97754f5 violated that (obscured) design by adding a
checkCachable() call to StoreEntry::memoryCachable(), effectively
blocking ENTRY_NEGCACHED entries from the memory cache as well. That
call should have been added, but checkCachable() should not have denied
caching rights to ENTRY_NEGCACHED -- the corresponding check should have
been moved into StoreEntry::mayStartSwapOut().

By removing ENTRY_NEGCACHED from checkCachable(), we now allow
ENTRY_NEGCACHED entries into both memory and disk caches, subject to all
the other checks. We allow ENTRY_NEGCACHED to be cached on disk because
negative responses are fundamentally no different than positive ones:
HTTP allows caching of 4xx and 5xx responses expiring in the future.
Hopefully, the increased disk cache traffic will not be a problem.
---

diff --git a/src/store.cc b/src/store.cc
index d5c60ff0da..80a38b4c09 100644
--- a/src/store.cc
+++ b/src/store.cc
@@ -899,7 +899,6 @@ struct _store_check_cachable_hist {
         int non_get;
         int not_entry_cachable;
         int wrong_content_length;
-        int negative_cached;
         int too_big;
         int too_small;
         int private_key;
@@ -979,10 +978,6 @@ StoreEntry::checkCachable()
         if (store_status == STORE_OK && EBIT_TEST(flags, ENTRY_BAD_LENGTH)) {
             debugs(20, 2, "StoreEntry::checkCachable: NO: wrong content-length");
             ++store_check_cachable_hist.no.wrong_content_length;
-        } else if (EBIT_TEST(flags, ENTRY_NEGCACHED)) {
-            debugs(20, 3, "StoreEntry::checkCachable: NO: negative cached");
-            ++store_check_cachable_hist.no.negative_cached;
-            return 0;           /* avoid release call below */
         } else if (!mem_obj) {
             // XXX: In bug 4131, we forgetHit() without mem_obj, so we need
             // this segfault protection, but how can we get such a HIT?
@@ -1034,7 +1029,7 @@ storeCheckCachableStats(StoreEntry *sentry)
     storeAppendPrintf(sentry, "no.wrong_content_length\t%d\n",
                       store_check_cachable_hist.no.wrong_content_length);
     storeAppendPrintf(sentry, "no.negative_cached\t%d\n",
-                      store_check_cachable_hist.no.negative_cached);
+                      0); // TODO: Remove this backward compatibility hack.
     storeAppendPrintf(sentry, "no.missing_parts\t%d\n",
                       store_check_cachable_hist.no.missing_parts);
     storeAppendPrintf(sentry, "no.too_big\t%d\n",
@@ -1374,7 +1369,10 @@ StoreEntry::negativeCache()
 #else
         expires = squid_curtime;
 #endif
-    EBIT_SET(flags, ENTRY_NEGCACHED);
+    if (expires > squid_curtime) {
+        EBIT_SET(flags, ENTRY_NEGCACHED);
+        debugs(20, 6, "expires = " << expires << " +" << (expires-squid_curtime) << ' ' << *this);
+    }
 }
 
 void