trafficserver-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From masa...@apache.org
Subject [trafficserver] branch master updated: Reverting to old negative_caching conditional behavior (#7401)
Date Wed, 23 Dec 2020 04:33:10 GMT
This is an automated email from the ASF dual-hosted git repository.

masaori pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/trafficserver.git


The following commit(s) were added to refs/heads/master by this push:
     new 80f2c3f  Reverting to old negative_caching conditional behavior (#7401)
80f2c3f is described below

commit 80f2c3f40133a59c00db05d8d3a6bc428b4d20f0
Author: Shinya Kawano <skawano@yahoo-corp.jp>
AuthorDate: Wed Dec 23 13:32:54 2020 +0900

    Reverting to old negative_caching conditional behavior (#7401)
    
    https://github.com/apache/trafficserver/pull/7361 fixed negative caching
    for non-cacheable negative responses, but it broke certain logic
    concerning checks for whether a given response was cacheable because of
    negative caching configuration. This fixes the latter behavior so it now
    behaves as it did before.
    
    Co-authored-by: bneradt <bneradt@verizonmedia.com>
---
 proxy/http/HttpSM.cc       |  8 ++++----
 proxy/http/HttpTransact.cc | 10 +++++-----
 proxy/http/HttpTransact.h  | 22 +++++++---------------
 3 files changed, 16 insertions(+), 24 deletions(-)

diff --git a/proxy/http/HttpSM.cc b/proxy/http/HttpSM.cc
index 8e5f46d..427e4a0 100644
--- a/proxy/http/HttpSM.cc
+++ b/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.is_cacheable_and_negative_caching_is_enabled && p->bytes_read
== 0) {
+        if (t_state.is_cacheable_due_to_negative_caching_configuration && 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.is_cacheable_and_negative_caching_is_enabled) {
-    t_state.is_cacheable_and_negative_caching_is_enabled = false;
+  if (t_state.is_cacheable_due_to_negative_caching_configuration) {
+    t_state.is_cacheable_due_to_negative_caching_configuration = false;
   }
 
   // If we had a ground fill, check update our status
@@ -6735,7 +6735,7 @@ HttpSM::setup_server_transfer()
 
   nbytes = server_transfer_init(buf, hdr_size);
 
-  if (t_state.is_cacheable_and_negative_caching_is_enabled &&
+  if (t_state.is_cacheable_due_to_negative_caching_configuration &&
       t_state.hdr_info.server_response.status_get() == HTTP_STATUS_NO_CONTENT) {
     int s = sizeof("No Content") - 1;
     buf->write("No Content", s);
diff --git a/proxy/http/HttpTransact.cc b/proxy/http/HttpTransact.cc
index 85ea71b..ea38992 100644
--- a/proxy/http/HttpTransact.cc
+++ b/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->is_cacheable_and_negative_caching_is_enabled = cacheable && s->txn_conf->negative_caching_enabled;
+    s->is_cacheable_due_to_negative_caching_configuration = cacheable && is_negative_caching_appropriate(s);
 
     // determine the correct cache action given the original cache action,
     // cacheability of server response, and request method
@@ -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->is_cacheable_and_negative_caching_is_enabled) {
+      if (s->is_cacheable_due_to_negative_caching_configuration) {
         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->is_cacheable_and_negative_caching_is_enabled) {
-      s->is_cacheable_and_negative_caching_is_enabled = false;
+    } else if (s->is_cacheable_due_to_negative_caching_configuration) {
+      s->is_cacheable_due_to_negative_caching_configuration = 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->is_cacheable_and_negative_caching_is_enabled || !cache_info->response_get()->valid())
{
+  if (!s->is_cacheable_due_to_negative_caching_configuration || !cache_info->response_get()->valid())
{
     cache_info->response_set(response);
   }
 
diff --git a/proxy/http/HttpTransact.h b/proxy/http/HttpTransact.h
index f65d5d2..686d8b4 100644
--- a/proxy/http/HttpTransact.h
+++ b/proxy/http/HttpTransact.h
@@ -737,23 +737,15 @@ public:
     bool client_connection_enabled = true;
     bool acl_filtering_performed   = false;
 
-    /// True if negative caching is enabled and the response is cacheable.
+    /// True if the response is cacheable because of negative caching configuration.
     ///
-    /// 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:
+    /// This being true implies the following:
     ///
-    ///   * 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;
+    /// * The response code was negative.
+    /// * Negative caching is enabled.
+    /// * The response is considered cacheable because of negative caching
+    ///   configuration.
+    bool is_cacheable_due_to_negative_caching_configuration = false;
     // for authenticated content caching
     CacheAuth_t www_auth_content = CACHE_AUTH_NONE;
 


Mime
View raw message