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 21:20:21 UTC
[trafficserver] branch 8.1.x updated: Revert "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 8b75139 Revert "Reverting to old negative_caching conditional behavior (#7401)"
8b75139 is described below
commit 8b751390c9b00c97253b48c27355403ef907ab98
Author: Leif Hedstrom <zw...@apache.org>
AuthorDate: Thu Jan 7 14:19:24 2021 -0700
Revert "Reverting to old negative_caching conditional behavior (#7401)"
This reverts commit f2f7be7ac59a25c420038a614b7df684d6ab20ff.
This does not apply, as is, on 8.1.x it seems.
---
proxy/http/HttpSM.cc | 8 ++++----
proxy/http/HttpTransact.cc | 10 +++++-----
proxy/http/HttpTransact.h | 22 +++++++++++++++-------
3 files changed, 24 insertions(+), 16 deletions(-)
diff --git a/proxy/http/HttpSM.cc b/proxy/http/HttpSM.cc
index 322f04a..0f737fa 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_due_to_negative_caching_configuration && 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.is_cacheable_due_to_negative_caching_configuration) {
- t_state.is_cacheable_due_to_negative_caching_configuration = 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,7 @@ HttpSM::setup_server_transfer()
nbytes = server_transfer_init(buf, hdr_size);
- if (t_state.is_cacheable_due_to_negative_caching_configuration &&
+ 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);
diff --git a/proxy/http/HttpTransact.cc b/proxy/http/HttpTransact.cc
index 3801774..66d96af 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_due_to_negative_caching_configuration = cacheable && is_negative_caching_appropriate(s);
+ 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
@@ -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_due_to_negative_caching_configuration) {
+ 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->is_cacheable_due_to_negative_caching_configuration) {
- s->is_cacheable_due_to_negative_caching_configuration = 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->is_cacheable_due_to_negative_caching_configuration || !cache_info->response_get()->valid()) {
+ if (!s->is_cacheable_and_negative_caching_is_enabled || !cache_info->response_get()->valid()) {
cache_info->response_set(response);
}
diff --git a/proxy/http/HttpTransact.h b/proxy/http/HttpTransact.h
index 8e618d1..800c10e 100644
--- a/proxy/http/HttpTransact.h
+++ b/proxy/http/HttpTransact.h
@@ -763,15 +763,23 @@ public:
bool client_connection_enabled = true;
bool acl_filtering_performed = false;
- /// True if the response is cacheable because of negative caching configuration.
+ /// True if negative caching is enabled and the response is cacheable.
///
- /// This being true implies the following:
+ /// 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 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;
+ /// * 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;