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>
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 @@
// 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 @@
}
// 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 @@
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 @@
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 @@
// 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 @@
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 @@
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 @@
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;
