You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@trafficserver.apache.org by zw...@apache.org on 2021/01/07 20:17:10 UTC
[trafficserver] branch 8.1.x updated: Reverting to old
negative_caching conditional behavior (#7401)
This is an automated email from the ASF dual-hosted git repository.
zwoop pushed a commit to branch 8.1.x
in repository https://gitbox.apache.org/repos/asf/trafficserver.git
The following commit(s) were added to refs/heads/8.1.x by this push:
new f2f7be7 Reverting to old negative_caching conditional behavior (#7401)
f2f7be7 is described below
commit f2f7be7ac59a25c420038a614b7df684d6ab20ff
Author: Shinya Kawano <sk...@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 <bn...@verizonmedia.com>
(cherry picked from commit 80f2c3f40133a59c00db05d8d3a6bc428b4d20f0)
---
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 0f737fa..322f04a 100644
--- a/proxy/http/HttpSM.cc
+++ b/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.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)
@@ -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.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
@@ -6579,7 +6579,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 66d96af..3801774 100644
--- a/proxy/http/HttpTransact.cc
+++ b/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->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
@@ -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->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);
@@ -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->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;
@@ -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->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 800c10e..8e618d1 100644
--- a/proxy/http/HttpTransact.h
+++ b/proxy/http/HttpTransact.h
@@ -763,23 +763,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;