You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@trafficserver.apache.org by bc...@apache.org on 2020/11/30 18:34:22 UTC
[trafficserver] branch 8.1.x updated: Add negative caching tests
and fixes. (#7359)
This is an automated email from the ASF dual-hosted git repository.
bcall 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 ac7ed95 Add negative caching tests and fixes. (#7359)
ac7ed95 is described below
commit ac7ed9580331b8fd65eccd732f651b14f95f71f0
Author: Brian Neradt <br...@gmail.com>
AuthorDate: Mon Nov 30 12:34:14 2020 -0600
Add negative caching tests and fixes. (#7359)
This adds test coverage for the negative caching feature and makes some
fixes as a result of the test's findings.
---
doc/admin-guide/files/records.config.en.rst | 4 +--
doc/admin-guide/performance/index.en.rst | 4 +--
mgmt/RecordsConfig.cc | 2 +-
proxy/http/HttpConfig.cc | 2 +-
proxy/http/HttpSM.cc | 9 ++++---
proxy/http/HttpTransact.cc | 38 ++++++++++++++---------------
proxy/http/HttpTransact.h | 19 +++++++++++++--
7 files changed, 46 insertions(+), 32 deletions(-)
diff --git a/doc/admin-guide/files/records.config.en.rst b/doc/admin-guide/files/records.config.en.rst
index 684c5fb..29e983f 100644
--- a/doc/admin-guide/files/records.config.en.rst
+++ b/doc/admin-guide/files/records.config.en.rst
@@ -1506,11 +1506,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
@@ -1528,7 +1526,7 @@ Negative Response Caching
How long (in seconds) Traffic Server 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
diff --git a/doc/admin-guide/performance/index.en.rst b/doc/admin-guide/performance/index.en.rst
index 18f05dc..87e999c 100644
--- a/doc/admin-guide/performance/index.en.rst
+++ b/doc/admin-guide/performance/index.en.rst
@@ -493,7 +493,7 @@ Error responses from origins are conistent 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
@@ -502,7 +502,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
~~~~~~~~~~~~~~~~~~~~
diff --git a/mgmt/RecordsConfig.cc b/mgmt/RecordsConfig.cc
index c31362b..b55cd1b 100644
--- a/mgmt/RecordsConfig.cc
+++ b/mgmt/RecordsConfig.cc
@@ -499,7 +499,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}
,
// #########################
diff --git a/proxy/http/HttpConfig.cc b/proxy/http/HttpConfig.cc
index 475e029..0a99ca1 100644
--- a/proxy/http/HttpConfig.cc
+++ b/proxy/http/HttpConfig.cc
@@ -910,7 +910,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()) {
diff --git a/proxy/http/HttpSM.cc b/proxy/http/HttpSM.cc
index 31e62e4..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.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)
@@ -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.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
@@ -6579,7 +6579,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;
diff --git a/proxy/http/HttpTransact.cc b/proxy/http/HttpTransact.cc
index e6c481e..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->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
@@ -4237,7 +4237,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;
@@ -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->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);
@@ -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->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;
@@ -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->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);
}
@@ -6163,24 +6163,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().
diff --git a/proxy/http/HttpTransact.h b/proxy/http/HttpTransact.h
index dfcd579..800c10e 100644
--- a/proxy/http/HttpTransact.h
+++ b/proxy/http/HttpTransact.h
@@ -763,8 +763,23 @@ public:
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;