You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@trafficserver.apache.org by jp...@apache.org on 2016/05/02 18:55:54 UTC
[trafficserver] 06/06: TS-4388: TSHttpTxnParentProxySet crashes in
parent selection.
This is an automated email from the ASF dual-hosted git repository.
jpeach pushed a commit to branch master
in repository https://git-dual.apache.org/repos/asf/trafficserver.git
commit 7a590edc76843233524f6ba093e57e523d560ba3
Author: James Peach <jp...@apache.org>
AuthorDate: Wed Apr 27 15:16:32 2016 -0700
TS-4388: TSHttpTxnParentProxySet crashes in parent selection.
Fix ParentResult to handle the case where the parent is specified
by the TSHttpTxnParentProxySet API. Encapsulate the internals of
the result better so that it is easier for the HTTP state machine
to do the right thing.
---
proxy/InkAPITest.cc | 6 ---
proxy/ParentConsistentHash.cc | 4 +-
proxy/ParentRoundRobin.cc | 4 +-
proxy/ParentSelection.cc | 2 +-
proxy/ParentSelection.h | 87 +++++++++++++++++++++++++++++++++++++++++--
proxy/http/HttpTransact.cc | 49 ++++++++++++------------
proxy/http/HttpTransact.h | 8 ++--
7 files changed, 117 insertions(+), 43 deletions(-)
diff --git a/proxy/InkAPITest.cc b/proxy/InkAPITest.cc
index 342e824..e635219 100644
--- a/proxy/InkAPITest.cc
+++ b/proxy/InkAPITest.cc
@@ -5982,12 +5982,6 @@ parent_proxy_handler(TSCont contp, TSEvent event, void *edata)
EXCLUSIVE_REGRESSION_TEST(SDK_API_HttpParentProxySet)(RegressionTest *test, int level, int *pstatus)
{
- // Don't enable this test by default until it passes.
- if (level < REGRESSION_TEST_FATAL) {
- *pstatus = REGRESSION_TEST_NOT_RUN;
- return;
- }
-
*pstatus = REGRESSION_TEST_INPROGRESS;
TSCont cont = TSContCreate(parent_proxy_handler, TSMutexCreate());
diff --git a/proxy/ParentConsistentHash.cc b/proxy/ParentConsistentHash.cc
index 1b5e396..e145cbc 100644
--- a/proxy/ParentConsistentHash.cc
+++ b/proxy/ParentConsistentHash.cc
@@ -236,7 +236,7 @@ ParentConsistentHash::markParentDown(const ParentSelectionPolicy *policy, Parent
}
// If we were set through the API we currently have not failover
// so just return fail
- if (result->rec == extApiRecord) {
+ if (result->is_api_result()) {
return;
}
@@ -312,7 +312,7 @@ ParentConsistentHash::markParentUp(ParentResult *result)
}
// If we were set through the API we currently have not failover
// so just return fail
- if (result->rec == extApiRecord) {
+ if (result->is_api_result()) {
ink_assert(0);
return;
}
diff --git a/proxy/ParentRoundRobin.cc b/proxy/ParentRoundRobin.cc
index 0716d78..c3ecf6d 100644
--- a/proxy/ParentRoundRobin.cc
+++ b/proxy/ParentRoundRobin.cc
@@ -198,7 +198,7 @@ ParentRoundRobin::markParentDown(const ParentSelectionPolicy *policy, ParentResu
}
// If we were set through the API we currently have not failover
// so just return fail
- if (result->rec == extApiRecord) {
+ if (result->is_api_result()) {
return;
}
@@ -257,7 +257,7 @@ ParentRoundRobin::markParentUp(ParentResult *result)
}
// If we were set through the API we currently have not failover
// so just return fail
- if (result->rec == extApiRecord) {
+ if (result->is_api_result()) {
ink_assert(0);
return;
}
diff --git a/proxy/ParentSelection.cc b/proxy/ParentSelection.cc
index bbedbaf..705fa4c 100644
--- a/proxy/ParentSelection.cc
+++ b/proxy/ParentSelection.cc
@@ -195,7 +195,7 @@ ParentConfigParams::nextParent(HttpRequestData *rdata, ParentResult *result)
}
// If we were set through the API we currently have not failover
// so just return fail
- if (result->rec == extApiRecord) {
+ if (result->is_api_result()) {
Debug("parent_select", "Retry result for %s was %s", rdata->get_host(), ParentResultStr[result->result]);
result->result = PARENT_FAIL;
return;
diff --git a/proxy/ParentSelection.h b/proxy/ParentSelection.h
index 975b07a..ccee7d9 100644
--- a/proxy/ParentSelection.h
+++ b/proxy/ParentSelection.h
@@ -115,8 +115,8 @@ class ParentRecord : public ControlBase
public:
ParentRecord()
: parents(NULL), secondary_parents(NULL), num_parents(0), num_secondary_parents(0), ignore_query(false), rr_next(0),
- go_direct(true), parent_is_proxy(true), selection_strategy(NULL), unavailable_server_retry_responses(NULL), parent_retry(0),
- max_simple_retries(1), max_unavailable_server_retries(1)
+ go_direct(true), parent_is_proxy(true), selection_strategy(NULL), unavailable_server_retry_responses(NULL),
+ parent_retry(PARENT_RETRY_NONE), max_simple_retries(1), max_unavailable_server_retries(1)
{
}
@@ -146,7 +146,7 @@ public:
bool parent_is_proxy;
ParentSelectionStrategy *selection_strategy;
UnavailableServerResponseCodes *unavailable_server_retry_responses;
- int parent_retry;
+ ParentRetry_t parent_retry;
int max_simple_retries;
int max_unavailable_server_retries;
};
@@ -173,6 +173,82 @@ struct ParentResult {
result = PARENT_UNDEFINED;
}
+ bool
+ is_api_result() const
+ {
+ return rec == extApiRecord;
+ }
+
+ // Do we have some result?
+ bool
+ is_some() const
+ {
+ if (rec == NULL) {
+ // If we don't have a result, we either haven't done a parent
+ // lookup yet (PARENT_UNDEFINED), or the lookup didn't match
+ // anything (PARENT_DIRECT).
+ ink_assert(result == PARENT_UNDEFINED || result == PARENT_DIRECT);
+ return false;
+ }
+
+ return true;
+ }
+
+ bool
+ parent_is_proxy() const
+ {
+ // Parents set by the TSHttpTxnParentProxySet API are always considered proxies rather than origins.
+ return is_api_result() ? true : rec->parent_is_proxy;
+ }
+
+ unsigned
+ retry_type() const
+ {
+ return is_api_result() ? PARENT_RETRY_NONE : rec->parent_retry;
+ }
+
+ unsigned
+ max_retries(ParentRetry_t method) const
+ {
+ // There's no API for specifying the retries, so you get 0.
+ if (is_api_result()) {
+ return 0;
+ }
+
+ switch (method) {
+ case PARENT_RETRY_NONE:
+ return 0;
+ case PARENT_RETRY_SIMPLE:
+ return rec->max_simple_retries;
+ case PARENT_RETRY_UNAVAILABLE_SERVER:
+ return rec->max_unavailable_server_retries;
+ case PARENT_RETRY_BOTH:
+ return std::max(rec->max_unavailable_server_retries, rec->max_simple_retries);
+ }
+
+ return 0;
+ }
+
+ bool
+ response_is_retryable(HTTPStatus response_code) const
+ {
+ return (retry_type() & PARENT_RETRY_UNAVAILABLE_SERVER) && rec->unavailable_server_retry_responses->contains(response_code);
+ }
+
+ bool
+ bypass_ok() const
+ {
+ if (is_api_result()) {
+ return false;
+ } else {
+ // Caller should check for a valid result beforehand.
+ ink_assert(result != PARENT_UNDEFINED);
+ ink_assert(is_some());
+ return rec->bypass_ok();
+ }
+ }
+
+private:
// Internal use only
// Not to be modified by HTTP
int line_number;
@@ -181,6 +257,11 @@ struct ParentResult {
uint32_t start_parent;
bool wrap_around;
int last_lookup; // state for for consistent hash.
+
+ friend class ParentConsistentHash;
+ friend class ParentRoundRobin;
+ friend class ParentConfigParams;
+ friend class ParentRecord;
};
struct ParentSelectionPolicy {
diff --git a/proxy/http/HttpTransact.cc b/proxy/http/HttpTransact.cc
index 5a333c3..1985bd3 100644
--- a/proxy/http/HttpTransact.cc
+++ b/proxy/http/HttpTransact.cc
@@ -68,21 +68,20 @@ static const char local_host_ip_str[] = "127.0.0.1";
inline static void
simple_or_unavailable_server_retry(HttpTransact::State *s)
{
- int server_response = 0;
-
- HTTP_RELEASE_ASSERT(!s->parent_result.rec->parent_is_proxy);
+ HTTP_RELEASE_ASSERT(!s->parent_result.parent_is_proxy());
// reponse is from a parent origin server.
- server_response = http_hdr_status_get(s->hdr_info.server_response.m_http);
+ HTTPStatus server_response = http_hdr_status_get(s->hdr_info.server_response.m_http);
DebugTxn("http_trans", "[simple_or_unavailabe_server_retry] server_response = %d, simple_retry_attempts: %d, numParents:%d \n",
server_response, s->current.simple_retry_attempts, s->parent_params->numParents(&s->parent_result));
// simple retry is enabled, 0x1
- if ((s->parent_result.rec->parent_retry & HttpTransact::PARENT_ORIGIN_SIMPLE_RETRY) &&
- (s->current.simple_retry_attempts < s->parent_result.rec->max_simple_retries && server_response == HTTP_STATUS_NOT_FOUND)) {
+ if ((s->parent_result.retry_type() & HttpTransact::PARENT_ORIGIN_SIMPLE_RETRY) &&
+ s->current.simple_retry_attempts < s->parent_result.max_retries(PARENT_RETRY_SIMPLE) &&
+ server_response == HTTP_STATUS_NOT_FOUND) {
DebugTxn("parent_select", "RECEIVED A SIMPLE RETRY RESPONSE");
- if (s->current.simple_retry_attempts < (int)s->parent_params->numParents(&s->parent_result)) {
+ if (s->current.simple_retry_attempts < s->parent_params->numParents(&s->parent_result)) {
s->current.state = HttpTransact::PARENT_ORIGIN_RETRY;
s->current.retry_type = HttpTransact::PARENT_ORIGIN_SIMPLE_RETRY;
return;
@@ -92,11 +91,11 @@ simple_or_unavailable_server_retry(HttpTransact::State *s)
}
}
// unavailable server retry is enabled 0x2
- else if ((s->parent_result.rec->parent_retry & HttpTransact::PARENT_ORIGIN_UNAVAILABLE_SERVER_RETRY) &&
- (s->current.unavailable_server_retry_attempts < s->parent_result.rec->max_unavailable_server_retries &&
- s->parent_result.rec->unavailable_server_retry_responses->contains(server_response))) {
+ else if ((s->parent_result.retry_type() & HttpTransact::PARENT_ORIGIN_UNAVAILABLE_SERVER_RETRY) &&
+ s->current.unavailable_server_retry_attempts < s->parent_result.max_retries(PARENT_RETRY_UNAVAILABLE_SERVER) &&
+ s->parent_result.response_is_retryable(server_response)) {
DebugTxn("parent_select", "RECEIVED A PARENT_ORIGIN_UNAVAILABLE_SERVER_RETRY RESPONSE");
- if (s->current.unavailable_server_retry_attempts < (int)s->parent_params->numParents(&s->parent_result)) {
+ if (s->current.unavailable_server_retry_attempts < s->parent_params->numParents(&s->parent_result)) {
s->current.state = HttpTransact::PARENT_ORIGIN_RETRY;
s->current.retry_type = HttpTransact::PARENT_ORIGIN_UNAVAILABLE_SERVER_RETRY;
return;
@@ -254,7 +253,7 @@ find_server_and_update_current_info(HttpTransact::State *s)
s->parent_result.result = PARENT_DIRECT;
} else if (s->method == HTTP_WKSIDX_CONNECT && s->http_config_param->disable_ssl_parenting) {
s->parent_params->findParent(&s->request_data, &s->parent_result);
- if (s->parent_result.rec == NULL || s->parent_result.rec->parent_is_proxy) {
+ if (!s->parent_result.is_some() || s->parent_result.is_api_result() || s->parent_result.parent_is_proxy()) {
DebugTxn("http_trans", "request not cacheable, so bypass parent");
s->parent_result.result = PARENT_DIRECT;
}
@@ -267,7 +266,7 @@ find_server_and_update_current_info(HttpTransact::State *s)
// with respect to whether a request is cacheable or not.
// For example, the cache_urls_that_look_dynamic variable.
s->parent_params->findParent(&s->request_data, &s->parent_result);
- if (s->parent_result.rec == NULL || s->parent_result.rec->parent_is_proxy) {
+ if (!s->parent_result.is_some() || s->parent_result.is_api_result() || s->parent_result.parent_is_proxy()) {
DebugTxn("http_trans", "request not cacheable, so bypass parent");
s->parent_result.result = PARENT_DIRECT;
}
@@ -291,11 +290,11 @@ find_server_and_update_current_info(HttpTransact::State *s)
case PARENT_FAIL:
// Check to see if should bypass the parent and go direct
// We can only do this if
- // 1) the parent was not set from API
+ // 1) the config permitted us to dns the origin server
// 2) the config permits us
- // 3) the config permitted us to dns the origin server
- if (!s->parent_params->apiParentExists(&s->request_data) && s->parent_result.rec->bypass_ok() &&
- s->http_config_param->no_dns_forward_to_parent == 0 && s->parent_result.rec->parent_is_proxy) {
+ // 3) the parent was not set from API
+ if (s->http_config_param->no_dns_forward_to_parent == 0 && s->parent_result.bypass_ok() &&
+ s->parent_result.parent_is_proxy() && !s->parent_params->apiParentExists(&s->request_data)) {
s->parent_result.result = PARENT_DIRECT;
}
break;
@@ -3549,9 +3548,9 @@ HttpTransact::handle_response_from_parent(State *s)
// response is from a parent origin server.
if (is_response_valid(s, &s->hdr_info.server_response) && s->current.request_to == HttpTransact::PARENT_PROXY &&
- !s->parent_result.rec->parent_is_proxy) {
+ !s->parent_result.parent_is_proxy()) {
// check for a retryable response if simple or unavailable server retry are enabled.
- if (s->parent_result.rec->parent_retry & (PARENT_ORIGIN_SIMPLE_RETRY | PARENT_ORIGIN_UNAVAILABLE_SERVER_RETRY)) {
+ if (s->parent_result.retry_type() & (PARENT_ORIGIN_SIMPLE_RETRY | PARENT_ORIGIN_UNAVAILABLE_SERVER_RETRY)) {
simple_or_unavailable_server_retry(s);
}
}
@@ -3591,7 +3590,7 @@ HttpTransact::handle_response_from_parent(State *s)
// try a simple retry if we received a simple retryable response from the parent.
if (s->current.retry_type == PARENT_ORIGIN_SIMPLE_RETRY || s->current.retry_type == PARENT_ORIGIN_UNAVAILABLE_SERVER_RETRY) {
if (s->current.retry_type == PARENT_ORIGIN_SIMPLE_RETRY) {
- if (s->current.simple_retry_attempts >= s->parent_result.rec->max_simple_retries) {
+ if (s->current.simple_retry_attempts >= s->parent_result.max_retries(PARENT_RETRY_SIMPLE)) {
DebugTxn("http_trans", "PARENT_ORIGIN_SIMPLE_RETRY: retried all parents, send error to client.\n");
s->current.retry_type = PARENT_ORIGIN_UNDEFINED_RETRY;
} else {
@@ -3601,7 +3600,7 @@ HttpTransact::handle_response_from_parent(State *s)
next_lookup = find_server_and_update_current_info(s);
}
} else { // try unavailable server retry if we have a unavailable server retry response from the parent.
- if (s->current.unavailable_server_retry_attempts >= s->parent_result.rec->max_unavailable_server_retries) {
+ if (s->current.unavailable_server_retry_attempts >= s->parent_result.max_retries(PARENT_RETRY_UNAVAILABLE_SERVER)) {
DebugTxn("http_trans", "PARENT_ORIGIN_UNAVAILABLE_SERVER_RETRY: retried all parents, send error to client.\n");
s->current.retry_type = PARENT_ORIGIN_UNDEFINED_RETRY;
} else {
@@ -3694,7 +3693,7 @@ HttpTransact::handle_response_from_server(State *s)
{
DebugTxn("http_trans", "[handle_response_from_server] (hrfs)");
HTTP_RELEASE_ASSERT(s->current.server == &s->server_info);
- int max_connect_retries = 0;
+ unsigned max_connect_retries = 0;
// plugin call
s->server_info.state = s->current.state;
@@ -3831,7 +3830,7 @@ HttpTransact::delete_server_rr_entry(State *s, int max_retries)
//
///////////////////////////////////////////////////////////////////////////////
void
-HttpTransact::retry_server_connection_not_open(State *s, ServerState_t conn_state, int max_retries)
+HttpTransact::retry_server_connection_not_open(State *s, ServerState_t conn_state, unsigned max_retries)
{
ink_assert(s->current.state != CONNECTION_ALIVE);
ink_assert(s->current.state != ACTIVE_TIMEOUT);
@@ -7840,13 +7839,13 @@ HttpTransact::build_request(State *s, HTTPHdr *base_request, HTTPHdr *outgoing_r
// If we're going to a parent proxy, make sure we pass host and port
// in the URL even if we didn't get them (e.g. transparent proxy)
if (s->current.request_to == PARENT_PROXY) {
- if (!outgoing_request->is_target_in_url() && s->parent_result.rec->parent_is_proxy) {
+ if (!outgoing_request->is_target_in_url() && s->parent_result.parent_is_proxy()) {
DebugTxn("http_trans", "[build_request] adding target to URL for parent proxy");
// No worry about HTTP/0.9 because we reject forward proxy requests that
// don't have a host anywhere.
outgoing_request->set_url_target_from_host_field();
- } else if (s->current.request_to == PARENT_PROXY && !s->parent_result.rec->parent_is_proxy &&
+ } else if (s->current.request_to == PARENT_PROXY && !s->parent_result.parent_is_proxy() &&
outgoing_request->is_target_in_url()) {
// If the parent is an origin server remove the hostname from the url.
DebugTxn("http_trans", "[build_request] removing target from URL for a parent origin.");
diff --git a/proxy/http/HttpTransact.h b/proxy/http/HttpTransact.h
index 8f77363..831046b 100644
--- a/proxy/http/HttpTransact.h
+++ b/proxy/http/HttpTransact.h
@@ -686,9 +686,9 @@ public:
ConnectionAttributes *server;
ink_time_t now;
ServerState_t state;
- int attempts;
- int simple_retry_attempts;
- int unavailable_server_retry_attempts;
+ unsigned attempts;
+ unsigned simple_retry_attempts;
+ unsigned unavailable_server_retry_attempts;
ParentOriginRetry_t retry_type;
_CurrentInfo()
@@ -1114,7 +1114,7 @@ public:
static void handle_response_from_parent(State *s);
static void handle_response_from_server(State *s);
static void delete_server_rr_entry(State *s, int max_retries);
- static void retry_server_connection_not_open(State *s, ServerState_t conn_state, int max_retries);
+ static void retry_server_connection_not_open(State *s, ServerState_t conn_state, unsigned max_retries);
static void handle_server_connection_not_open(State *s);
static void handle_forward_server_connection_open(State *s);
static void handle_cache_operation_on_forward_server_response(State *s);
--
To stop receiving notification emails like this one, please contact
"commits@trafficserver.apache.org" <co...@trafficserver.apache.org>.