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

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.

This is master commit 91870bf backported to v4.

diff --git a/src/store.cc b/src/store.cc
index 9dab8fa0bfea378e17b5204c0082e3f9c2b31c9e..854a296c4fb6ecc5ec92b54d235b908e2dc3a4bc 100644 (file)
--- a/src/store.cc
+++ b/src/store.cc
@@ -917,7 +917,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;
@@ -998,10 +997,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 || !getReply()) {
             // XXX: In bug 4131, we forgetHit() without mem_obj, so we need
             // this segfault protection, but how can we get such a HIT?
@@ -1053,7 +1048,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",
@@ -1412,7 +1407,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