Add negative caching tests and fixes.

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

@@ -1664,11 +1664,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
@@ -1686,7 +1684,7 @@ Negative Response Caching
    How long (in seconds) |TS| 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

@@ -495,7 +495,7 @@ Error responses from origins are consistent 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
@@ -504,7 +504,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

@@ -503,7 +503,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

@@ -1038,7 +1038,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

@@ -3055,7 +3055,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)
@@ -3111,8 +3111,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
@@ -6736,7 +6736,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

@@ -4402,7 +4402,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
@@ -4437,7 +4437,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;
@@ -4464,7 +4464,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);
@@ -4500,8 +4500,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;
@@ -4911,7 +4911,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);
   }
 
@@ -6296,24 +6296,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

@@ -738,8 +738,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;
 

tests/gold_tests/autest-site/verifier_server.test.ext

@@ -51,6 +51,8 @@ def _configure_server(obj, process, name, replay_path, http_ports=None, https_po
     if http_ports is None:
         get_port(process, "http_port")
         http_ports = [process.Variables.http_port]
+    else:
+        process.Variables['http_port'] = http_ports[0]
 
     if len(http_ports) > 0:
         command += "--listen "
@@ -60,6 +62,8 @@ def _configure_server(obj, process, name, replay_path, http_ports=None, https_po
     if https_ports is None:
         get_port(process, "https_port")
         https_ports = [process.Variables.https_port]
+    else:
+        process.Variables['https_port'] = https_ports[0]
 
     if len(https_ports) > 0:
         command += '--listen-https '

tests/gold_tests/cache/negative-caching.test.py

@@ -0,0 +1,163 @@
+'''
+Test negative caching.
+'''
+#  Licensed to the Apache Software Foundation (ASF) under one
+#  or more contributor license agreements.  See the NOTICE file
+#  distributed with this work for additional information
+#  regarding copyright ownership.  The ASF licenses this file
+#  to you under the Apache License, Version 2.0 (the
+#  "License"); you may not use this file except in compliance
+#  with the License.  You may obtain a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+#  Unless required by applicable law or agreed to in writing, software
+#  distributed under the License is distributed on an "AS IS" BASIS,
+#  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+#  See the License for the specific language governing permissions and
+#  limitations under the License.
+
+Test.Summary = '''
+Test negative caching.
+'''
+
+#
+# Negative caching disabled.
+#
+ts = Test.MakeATSProcess("ts-disabled")
+replay_file = "replay/negative-caching-disabled.replay.yaml"
+server = Test.MakeVerifierServerProcess("server-disabled", replay_file)
+ts.Disk.records_config.update({
+    'proxy.config.diags.debug.enabled': 1,
+    'proxy.config.diags.debug.tags': 'http',
+    'proxy.config.http.insert_age_in_response': 0,
+
+    'proxy.config.http.negative_caching_enabled': 0
+})
+ts.Disk.remap_config.AddLine(
+    'map / http://127.0.0.1:{0}'.format(server.Variables.http_port)
+)
+tr = Test.AddTestRun("Verify correct behavior without negative caching enabled.")
+tr.Processes.Default.StartBefore(server)
+tr.Processes.Default.StartBefore(ts)
+tr.AddVerifierClientProcess("client-disabled", replay_file, http_ports=[ts.Variables.port])
+
+#
+# Negative caching enabled with otherwise default configuration.
+#
+ts = Test.MakeATSProcess("ts-default")
+replay_file = "replay/negative-caching-default.replay.yaml"
+server = Test.MakeVerifierServerProcess("server-default", replay_file)
+ts.Disk.records_config.update({
+    'proxy.config.diags.debug.enabled': 1,
+    'proxy.config.diags.debug.tags': 'http',
+    'proxy.config.http.insert_age_in_response': 0,
+
+    'proxy.config.http.negative_caching_enabled': 1
+})
+ts.Disk.remap_config.AddLine(
+    'map / http://127.0.0.1:{0}'.format(server.Variables.http_port)
+)
+tr = Test.AddTestRun("Verify default negative caching behavior")
+tr.Processes.Default.StartBefore(server)
+tr.Processes.Default.StartBefore(ts)
+tr.AddVerifierClientProcess("client-default", replay_file, http_ports=[ts.Variables.port])
+
+#
+# Customized response caching for negative caching configuration.
+#
+ts = Test.MakeATSProcess("ts-customized")
+replay_file = "replay/negative-caching-customized.replay.yaml"
+server = Test.MakeVerifierServerProcess("server-customized", replay_file)
+ts.Disk.records_config.update({
+    'proxy.config.diags.debug.enabled': 1,
+    'proxy.config.diags.debug.tags': 'http',
+    'proxy.config.http.insert_age_in_response': 0,
+
+    'proxy.config.http.negative_caching_enabled': 1,
+    'proxy.config.http.negative_caching_list': "400"
+})
+ts.Disk.remap_config.AddLine(
+    'map / http://127.0.0.1:{0}'.format(server.Variables.http_port)
+)
+tr = Test.AddTestRun("Verify customized negative caching list")
+tr.Processes.Default.StartBefore(server)
+tr.Processes.Default.StartBefore(ts)
+tr.AddVerifierClientProcess("client-customized", replay_file, http_ports=[ts.Variables.port])
+
+#
+# Verify correct proxy.config.http.negative_caching_lifetime behavior.
+#
+ts = Test.MakeATSProcess("ts-lifetime")
+ts.Disk.records_config.update({
+    'proxy.config.diags.debug.enabled': 1,
+    'proxy.config.diags.debug.tags': 'http',
+    'proxy.config.http.insert_age_in_response': 0,
+
+    'proxy.config.http.negative_caching_enabled': 1,
+    'proxy.config.http.negative_caching_lifetime': 2
+})
+# This should all behave the same as the default enabled case above.
+tr = Test.AddTestRun("Add a 404 response to the cache")
+replay_file = "replay/negative-caching-default.replay.yaml"
+server = tr.AddVerifierServerProcess("server-lifetime-no-cc", replay_file)
+# Use the same port across the two servers so that the remap config will work
+# across both.
+server_port = server.Variables.http_port
+tr.AddVerifierClientProcess("client-lifetime-no-cc", replay_file, http_ports=[ts.Variables.port])
+ts.Disk.remap_config.AddLine(
+    'map / http://127.0.0.1:{0}'.format(server_port)
+)
+tr.Processes.Default.StartBefore(ts)
+tr.StillRunningAfter = ts
+
+# Wait enough time that the item should be aged out of the cache.
+tr = Test.AddTestRun("Wait for cached object to be stale.")
+tr.Processes.Default.Command = "sleep 4"
+tr.StillRunningAfter = ts
+
+# Verify the item is retrieved from the server instead of the cache.
+replay_file = "replay/negative-caching-timeout.replay.yaml"
+tr = Test.AddTestRun("Make sure object is stale")
+tr.AddVerifierServerProcess("server-timeout", replay_file, http_ports=[server_port])
+tr.AddVerifierClientProcess("client-timeout", replay_file, http_ports=[ts.Variables.port])
+tr.StillRunningAfter = ts
+
+#
+# Verify that the server's Cache-Control overrides the
+# proxy.config.http.negative_caching_lifetime.
+#
+ts = Test.MakeATSProcess("ts-lifetime-2")
+ts.Disk.records_config.update({
+    'proxy.config.diags.debug.enabled': 1,
+    'proxy.config.diags.debug.tags': 'http',
+    'proxy.config.http.insert_age_in_response': 0,
+
+    'proxy.config.http.negative_caching_enabled': 1,
+    'proxy.config.http.negative_caching_lifetime': 2
+})
+tr = Test.AddTestRun("Add a 404 response with explicit max-age=300 to the cache")
+replay_file = "replay/negative-caching-300-second-timeout.replay.yaml"
+server = tr.AddVerifierServerProcess("server-lifetime-cc", replay_file)
+# Use the same port across the two servers so that the remap config will work
+# across both.
+server_port = server.Variables.http_port
+tr.AddVerifierClientProcess("client-lifetime-cc", replay_file, http_ports=[ts.Variables.port])
+ts.Disk.remap_config.AddLine(
+    'map / http://127.0.0.1:{0}'.format(server_port)
+)
+tr.Processes.Default.StartBefore(ts)
+tr.StillRunningAfter = ts
+
+# Wait enough time that the item should be aged out of the cache if
+# proxy.config.http.negative_caching_lifetime is incorrectly used.
+tr = Test.AddTestRun("Wait for cached object to be stale if lifetime is incorrectly used.")
+tr.Processes.Default.Command = "sleep 4"
+tr.StillRunningAfter = ts
+
+# Verify the item is retrieved from the cache instead of going to the origin.
+replay_file = "replay/negative-caching-no-timeout.replay.yaml"
+tr = Test.AddTestRun("Make sure object is fresh")
+tr.AddVerifierServerProcess("server-no-timeout", replay_file, http_ports=[server_port])
+tr.AddVerifierClientProcess("client-no-timeout", replay_file, http_ports=[ts.Variables.port])
+tr.StillRunningAfter = ts