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:02:30 UTC
[trafficserver] branch 7.1.x updated: Add negative caching tests
and fixes. (#7357)
This is an automated email from the ASF dual-hosted git repository.
bcall pushed a commit to branch 7.1.x
in repository https://gitbox.apache.org/repos/asf/trafficserver.git
The following commit(s) were added to refs/heads/7.1.x by this push:
new 1b3948c Add negative caching tests and fixes. (#7357)
1b3948c is described below
commit 1b3948cca589a6e31ff8c9c39feceaa7f8ac796b
Author: Brian Neradt <br...@gmail.com>
AuthorDate: Mon Nov 30 12:02:20 2020 -0600
Add negative caching tests and fixes. (#7357)
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 | 3 +-
doc/admin-guide/performance/index.en.rst | 2 +-
proxy/http/HttpSM.cc | 9 +++--
proxy/http/HttpTransact.cc | 60 ++++++++++++++---------------
proxy/http/HttpTransact.h | 21 ++++++++--
5 files changed, 54 insertions(+), 41 deletions(-)
diff --git a/doc/admin-guide/files/records.config.en.rst b/doc/admin-guide/files/records.config.en.rst
index 7694915..1669649 100644
--- a/doc/admin-guide/files/records.config.en.rst
+++ b/doc/admin-guide/files/records.config.en.rst
@@ -1566,10 +1566,9 @@ Negative Response Caching
====================== =====================================================
``204`` No Content
``305`` Use Proxy
- ``400`` Bad Request
``403`` Forbidden
``404`` Not Found
- ``405`` Method Not Allowed
+ ``414`` URI Too Long
``500`` Internal Server Error
``501`` Not Implemented
``502`` Bad Gateway
diff --git a/doc/admin-guide/performance/index.en.rst b/doc/admin-guide/performance/index.en.rst
index 93f9b75..7d6d5e4 100644
--- a/doc/admin-guide/performance/index.en.rst
+++ b/doc/admin-guide/performance/index.en.rst
@@ -492,7 +492,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
diff --git a/proxy/http/HttpSM.cc b/proxy/http/HttpSM.cc
index a8ecedf..8d93262 100644
--- a/proxy/http/HttpSM.cc
+++ b/proxy/http/HttpSM.cc
@@ -3053,7 +3053,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)
@@ -3108,8 +3108,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
@@ -6665,7 +6665,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 0988507..8deee50 100644
--- a/proxy/http/HttpTransact.cc
+++ b/proxy/http/HttpTransact.cc
@@ -228,18 +228,16 @@ is_negative_caching_appropriate(HttpTransact::State *s)
}
switch (s->hdr_info.server_response.status_get()) {
- case HTTP_STATUS_NO_CONTENT:
- case HTTP_STATUS_USE_PROXY:
- case HTTP_STATUS_BAD_REQUEST:
- case HTTP_STATUS_FORBIDDEN:
- case HTTP_STATUS_NOT_FOUND:
- case HTTP_STATUS_METHOD_NOT_ALLOWED:
- case HTTP_STATUS_REQUEST_URI_TOO_LONG:
- case HTTP_STATUS_INTERNAL_SERVER_ERROR:
- case HTTP_STATUS_NOT_IMPLEMENTED:
- case HTTP_STATUS_BAD_GATEWAY:
- case HTTP_STATUS_SERVICE_UNAVAILABLE:
- case HTTP_STATUS_GATEWAY_TIMEOUT:
+ case HTTP_STATUS_NO_CONTENT: /* 204: No Content */
+ case HTTP_STATUS_USE_PROXY: /* 305: Use Proxy */
+ case HTTP_STATUS_FORBIDDEN: /* 403: Forbidden */
+ case HTTP_STATUS_NOT_FOUND: /* 404: Not Found */
+ case HTTP_STATUS_REQUEST_URI_TOO_LONG: /* 414: URI Too Long */
+ case HTTP_STATUS_INTERNAL_SERVER_ERROR: /* 500: Internal Server Error */
+ case HTTP_STATUS_NOT_IMPLEMENTED: /* 501: Not Implemented */
+ case HTTP_STATUS_BAD_GATEWAY: /* 502: Bad Gateway */
+ case HTTP_STATUS_SERVICE_UNAVAILABLE: /* 503: Service Unavailable */
+ case HTTP_STATUS_GATEWAY_TIMEOUT: /* 504: Gateway Timeout */
return true;
default:
break;
@@ -4494,7 +4492,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
@@ -4528,7 +4526,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;
@@ -4555,7 +4553,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);
@@ -4590,8 +4588,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;
@@ -5004,7 +5002,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);
}
@@ -6428,24 +6426,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)) {
- DebugTxn("http_trans", "[is_response_cacheable] YES by default ");
- return true;
- } else {
- DebugTxn("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)) {
+ DebugTxn("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 3f8bd67..7a240e4 100644
--- a/proxy/http/HttpTransact.h
+++ b/proxy/http/HttpTransact.h
@@ -921,8 +921,23 @@ public:
bool client_connection_enabled;
bool acl_filtering_performed;
- // for negative caching
- bool negative_caching;
+ /// 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;
// for authenticated content caching
CacheAuth_t www_auth_content;
@@ -1046,7 +1061,7 @@ public:
state_machine_id(0),
client_connection_enabled(true),
acl_filtering_performed(false),
- negative_caching(false),
+ is_cacheable_and_negative_caching_is_enabled(false),
www_auth_content(CACHE_AUTH_NONE),
remap_plugin_instance(0),
fp_tsremap_os_response(NULL),