Add negative caching tests and fixes.

doc/admin-guide/files/records.config.en.rst

@@ -1506,11 +1506,9 @@ Negative Response Caching
    ====================== =====================================================
    ``204``                No Content
    ``305``                Use Proxy
-   ``400``                Bad Request
    ``403``                Forbidden
    ``404``                Not Found
    ``414``                URI Too Long
-   ``405``                Method Not Allowed
    ``500``                Internal Server Error
    ``501``                Not Implemented
    ``502``                Bad Gateway
@@ -1528,7 +1526,7 @@ Negative Response Caching
    How long (in seconds) Traffic Server keeps the negative responses  valid in cache. This value only affects negative
    responses that do NOT have explicit ``Expires:`` or ``Cache-Control:`` lifetimes set by the server.
 
-.. ts:cv:: CONFIG proxy.config.http.negative_caching_list STRING 204 305 403 404 405 414 500 501 502 503 504
+.. ts:cv:: CONFIG proxy.config.http.negative_caching_list STRING 204 305 403 404 414 500 501 502 503 504
    :reloadable:
 
    The HTTP status code for negative caching. Default values are mentioned above. The unwanted status codes can be

doc/admin-guide/performance/index.en.rst

@@ -493,7 +493,7 @@ Error responses from origins are conistent and costly
 If error responses are costly for your origin server to generate, you may elect
 to have |TS| cache these responses for a period of time. The default behavior is
 to consider all of these responses to be uncacheable, which will lead to every
-client request to result in an origin request.
+client request resulting in an origin request.
 
 This behavior is controlled by both enabling the feature via
 :ts:cv:`proxy.config.http.negative_caching_enabled` and setting the cache time
@@ -502,7 +502,7 @@ status code for negative caching can be set with :ts:cv:`proxy.config.http.negat
 
     CONFIG proxy.config.http.negative_caching_enabled INT 1
     CONFIG proxy.config.http.negative_caching_lifetime INT 10
-    CONFIG proxy.config.http.negative_caching_list STRING 204 305 403 404 405 414 500 501 502 503 504
+    CONFIG proxy.config.http.negative_caching_list STRING 204 305 403 404 414 500 501 502 503 504
 
 SSL-Specific Options
 ~~~~~~~~~~~~~~~~~~~~

mgmt/RecordsConfig.cc

@@ -499,7 +499,7 @@ static const RecordElement RecordsConfig[] =
   ,
   {RECT_CONFIG, "proxy.config.http.negative_caching_lifetime", RECD_INT, "1800", RECU_DYNAMIC, RR_NULL, RECC_NULL, nullptr, RECA_NULL}
   ,
-  {RECT_CONFIG, "proxy.config.http.negative_caching_list", RECD_STRING, "204 305 403 404 405 414 500 501 502 503 504", RECU_DYNAMIC, RR_NULL, RECC_NULL, nullptr, RECA_NULL}
+  {RECT_CONFIG, "proxy.config.http.negative_caching_list", RECD_STRING, "204 305 403 404 414 500 501 502 503 504", RECU_DYNAMIC, RR_NULL, RECC_NULL, nullptr, RECA_NULL}
   ,
 
   //        #########################

proxy/http/HttpConfig.cc

@@ -910,7 +910,7 @@ set_negative_caching_list(const char *name, RecDataT dtype, RecData data, HttpCo
   HttpStatusBitset set;
   // values from proxy.config.http.negative_caching_list
   if (0 == strcasecmp("proxy.config.http.negative_caching_list", name) && RECD_STRING == dtype && data.rec_string) {
-    // parse the list of status code
+    // parse the list of status codes
     ts::TextView status_list(data.rec_string, strlen(data.rec_string));
     auto is_sep{[](char c) { return isspace(c) || ',' == c || ';' == c; }};
     while (!status_list.ltrim_if(is_sep).empty()) {

proxy/http/HttpSM.cc

@@ -2941,7 +2941,7 @@ HttpSM::tunnel_handler_server(int event, HttpTunnelProducer *p)
       // the reason string being written to the client and a bad CL when reading from cache.
       // I didn't find anywhere this appended reason is being used, so commenting it out.
       /*
-        if (t_state.negative_caching && p->bytes_read == 0) {
+        if (t_state.is_cacheable_and_negative_caching_is_enabled && p->bytes_read == 0) {
         int reason_len;
         const char *reason = t_state.hdr_info.server_response.reason_get(&reason_len);
         if (reason == NULL)
@@ -2996,8 +2996,8 @@ HttpSM::tunnel_handler_server(int event, HttpTunnelProducer *p)
   }
 
   // turn off negative caching in case there are multiple server contacts
-  if (t_state.negative_caching) {
-    t_state.negative_caching = false;
+  if (t_state.is_cacheable_and_negative_caching_is_enabled) {
+    t_state.is_cacheable_and_negative_caching_is_enabled = false;
   }
 
   // If we had a ground fill, check update our status
@@ -6579,7 +6579,8 @@ HttpSM::setup_server_transfer()
 
   nbytes = server_transfer_init(buf, hdr_size);
 
-  if (t_state.negative_caching && t_state.hdr_info.server_response.status_get() == HTTP_STATUS_NO_CONTENT) {
+  if (t_state.is_cacheable_and_negative_caching_is_enabled &&
+      t_state.hdr_info.server_response.status_get() == HTTP_STATUS_NO_CONTENT) {
     int s = sizeof("No Content") - 1;
     buf->write("No Content", s);
     nbytes += s;

proxy/http/HttpTransact.cc

@@ -4202,7 +4202,7 @@ HttpTransact::handle_cache_operation_on_forward_server_response(State *s)
     client_response_code = server_response_code;
     base_response        = &s->hdr_info.server_response;
 
-    s->negative_caching = is_negative_caching_appropriate(s) && cacheable;
+    s->is_cacheable_and_negative_caching_is_enabled = cacheable && s->txn_conf->negative_caching_enabled;
 
     // determine the correct cache action given the original cache action,
     // cacheability of server response, and request method
@@ -4237,7 +4237,7 @@ HttpTransact::handle_cache_operation_on_forward_server_response(State *s)
       }
 
     } else if (s->cache_info.action == CACHE_DO_WRITE) {
-      if (!cacheable && !s->negative_caching) {
+      if (!cacheable) {
         s->cache_info.action = CACHE_DO_NO_ACTION;
       } else if (s->method == HTTP_WKSIDX_HEAD) {
         s->cache_info.action = CACHE_DO_NO_ACTION;
@@ -4264,7 +4264,7 @@ HttpTransact::handle_cache_operation_on_forward_server_response(State *s)
     //   before issuing a 304
     if (s->cache_info.action == CACHE_DO_WRITE || s->cache_info.action == CACHE_DO_NO_ACTION ||
         s->cache_info.action == CACHE_DO_REPLACE) {
-      if (s->negative_caching) {
+      if (s->is_cacheable_and_negative_caching_is_enabled) {
         HTTPHdr *resp;
         s->cache_info.object_store.create();
         s->cache_info.object_store.request_set(&s->hdr_info.client_request);
@@ -4300,8 +4300,8 @@ HttpTransact::handle_cache_operation_on_forward_server_response(State *s)
           SET_VIA_STRING(VIA_PROXY_RESULT, VIA_PROXY_SERVER_REVALIDATED);
         }
       }
-    } else if (s->negative_caching) {
-      s->negative_caching = false;
+    } else if (s->is_cacheable_and_negative_caching_is_enabled) {
+      s->is_cacheable_and_negative_caching_is_enabled = false;
     }
 
     break;
@@ -4711,7 +4711,7 @@ HttpTransact::set_headers_for_cache_write(State *s, HTTPInfo *cache_info, HTTPHd
      sites yields no insight. So the assert is removed and we keep the behavior that if the response
      in @a cache_info is already set, we don't override it.
   */
-  if (!s->negative_caching || !cache_info->response_get()->valid()) {
+  if (!s->is_cacheable_and_negative_caching_is_enabled || !cache_info->response_get()->valid()) {
     cache_info->response_set(response);
   }
 
@@ -6163,24 +6163,24 @@ HttpTransact::is_response_cacheable(State *s, HTTPHdr *request, HTTPHdr *respons
     }
   }
 
-  // default cacheability
-  if (!s->txn_conf->negative_caching_enabled) {
-    if ((response_code == HTTP_STATUS_OK) || (response_code == HTTP_STATUS_NOT_MODIFIED) ||
-        (response_code == HTTP_STATUS_NON_AUTHORITATIVE_INFORMATION) || (response_code == HTTP_STATUS_MOVED_PERMANENTLY) ||
-        (response_code == HTTP_STATUS_MULTIPLE_CHOICES) || (response_code == HTTP_STATUS_GONE)) {
-      TxnDebug("http_trans", "[is_response_cacheable] YES by default ");
-      return true;
-    } else {
-      TxnDebug("http_trans", "[is_response_cacheable] NO by default");
-      return false;
-    }
+  if ((response_code == HTTP_STATUS_OK) || (response_code == HTTP_STATUS_NOT_MODIFIED) ||
+      (response_code == HTTP_STATUS_NON_AUTHORITATIVE_INFORMATION) || (response_code == HTTP_STATUS_MOVED_PERMANENTLY) ||
+      (response_code == HTTP_STATUS_MULTIPLE_CHOICES) || (response_code == HTTP_STATUS_GONE)) {
+    TxnDebug("http_trans", "[is_response_cacheable] YES response code seems fine");
+    return true;
   }
+  // Notice that the following are not overridable by negative caching.
   if (response_code == HTTP_STATUS_SEE_OTHER || response_code == HTTP_STATUS_UNAUTHORIZED ||
       response_code == HTTP_STATUS_PROXY_AUTHENTICATION_REQUIRED) {
     return false;
   }
-  // let is_negative_caching_approriate decide what to do
-  return true;
+  // The response code does not look appropriate for caching. Check, however,
+  // whether the user has specified it should be cached via negative response
+  // caching configuration.
+  if (is_negative_caching_appropriate(s)) {
+    return true;
+  }
+  return false;
   /* Since we weren't caching response obtained with
      Authorization (the cache control stuff was commented out previously)
      I've moved this check to is_request_cache_lookupable().

proxy/http/HttpTransact.h

@@ -763,8 +763,23 @@ class HttpTransact
     bool client_connection_enabled = true;
     bool acl_filtering_performed   = false;
 
-    // for negative caching
-    bool negative_caching = false;
+    /// True if negative caching is enabled and the response is cacheable.
+    ///
+    /// Note carefully that this being true does not necessarily imply that the
+    /// response code was negative. It means that (a) the response was
+    /// cacheable apart from response code considerations, and (b) concerning
+    /// the response code one of the following was true:
+    ///
+    ///   * The response was a negative response code configured cacheable
+    ///   by the user via negative response caching configuration, or ...
+    ///
+    ///   * The response code was an otherwise cacheable positive repsonse
+    ///   value (such as a 200 response, for example).
+    ///
+    /// TODO: We should consider refactoring this variable and its use. For now
+    /// I'm giving it an awkwardly long name to make sure the meaning of it is
+    /// clear in its various contexts.
+    bool is_cacheable_and_negative_caching_is_enabled = false;
     // for authenticated content caching
     CacheAuth_t www_auth_content = CACHE_AUTH_NONE;