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 2019/07/22 22:51:26 UTC
[trafficserver] branch 8.0.x updated: This fixes issue #5642
Origin_max_connections can create state machine loops when using parent
selection to select a parent for redundancy and/or load balancing.
This is an automated email from the ASF dual-hosted git repository.
bcall pushed a commit to branch 8.0.x
in repository https://gitbox.apache.org/repos/asf/trafficserver.git
The following commit(s) were added to refs/heads/8.0.x by this push:
new 514cab5 This fixes issue #5642 Origin_max_connections can create state machine loops when using parent selection to select a parent for redundancy and/or load balancing.
514cab5 is described below
commit 514cab5e0e55b952aac6555a81faa3017e3c4b34
Author: John Rushford <jr...@apache.org>
AuthorDate: Tue Jun 18 17:30:39 2019 +0000
This fixes issue #5642 Origin_max_connections can create state
machine loops when using parent selection to select a parent
for redundancy and/or load balancing.
The parent selection handling in the state machine did not
handle the case when origin max connections is reached or
exceeded when using throttling. This led to state machine
looping and a crash when the stack is overflowed due to the
looping. Now this is handled by either trying a new uncongested
origin or sending a negative 503 response if all available
origins are congested.
---
proxy/http/HttpDebugNames.cc | 2 +
proxy/http/HttpSM.cc | 8 +-
proxy/http/HttpTransact.cc | 236 ++++++++++++++++++++++++-------------------
proxy/http/HttpTransact.h | 3 +-
4 files changed, 143 insertions(+), 106 deletions(-)
diff --git a/proxy/http/HttpDebugNames.cc b/proxy/http/HttpDebugNames.cc
index 8f270a8..31c8f9f 100644
--- a/proxy/http/HttpDebugNames.cc
+++ b/proxy/http/HttpDebugNames.cc
@@ -56,6 +56,8 @@ HttpDebugNames::get_server_state_name(HttpTransact::ServerState_t state)
return "TRANSACTION_COMPLETE";
case HttpTransact::PARENT_RETRY:
return "PARENT_RETRY";
+ case HttpTransact::OUTBOUND_CONGESTION:
+ return "OUTBOUND_CONGESTION";
}
return ("unknown state name");
diff --git a/proxy/http/HttpSM.cc b/proxy/http/HttpSM.cc
index b6d03b5..1008fd8 100644
--- a/proxy/http/HttpSM.cc
+++ b/proxy/http/HttpSM.cc
@@ -4620,8 +4620,12 @@ HttpSM::do_cache_prepare_action(HttpCacheSM *c_sm, CacheHTTPInfo *object_read_in
void
HttpSM::send_origin_throttled_response()
{
- t_state.current.attempts = t_state.txn_conf->connect_attempts_max_retries;
- t_state.current.state = HttpTransact::CONNECTION_ERROR;
+ // if the request is to a parent proxy, do not reset
+ // t_state.current.attempts so that another parent may be tried.
+ if (t_state.current.request_to != HttpTransact::PARENT_PROXY) {
+ t_state.current.attempts = t_state.txn_conf->connect_attempts_max_retries;
+ }
+ t_state.current.state = HttpTransact::OUTBOUND_CONGESTION;
call_transact_and_set_next_state(HttpTransact::HandleResponse);
}
diff --git a/proxy/http/HttpTransact.cc b/proxy/http/HttpTransact.cc
index 10b5548..de03df6 100644
--- a/proxy/http/HttpTransact.cc
+++ b/proxy/http/HttpTransact.cc
@@ -263,8 +263,14 @@ find_server_and_update_current_info(HttpTransact::State *s)
TxnDebug("http_trans", "request is from localhost, so bypass parent");
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, s->txn_conf->parent_fail_threshold,
- s->txn_conf->parent_retry_time);
+ if (s->parent_result.result == PARENT_SPECIFIED) {
+ s->parent_params->nextParent(&s->request_data, &s->parent_result, s->txn_conf->parent_fail_threshold,
+ s->txn_conf->parent_retry_time);
+ } else {
+ s->parent_params->findParent(&s->request_data, &s->parent_result, s->txn_conf->parent_fail_threshold,
+ s->txn_conf->parent_retry_time);
+ }
+
if (!s->parent_result.is_some() || s->parent_result.is_api_result() || s->parent_result.parent_is_proxy()) {
TxnDebug("http_trans", "request not cacheable, so bypass parent");
s->parent_result.result = PARENT_DIRECT;
@@ -277,8 +283,13 @@ find_server_and_update_current_info(HttpTransact::State *s)
// we are assuming both child and parent have similar configuration
// 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, s->txn_conf->parent_fail_threshold,
- s->txn_conf->parent_retry_time);
+ if (s->parent_result.result == PARENT_SPECIFIED) {
+ s->parent_params->nextParent(&s->request_data, &s->parent_result, s->txn_conf->parent_fail_threshold,
+ s->txn_conf->parent_retry_time);
+ } else {
+ s->parent_params->findParent(&s->request_data, &s->parent_result, s->txn_conf->parent_fail_threshold,
+ s->txn_conf->parent_retry_time);
+ }
if (!s->parent_result.is_some() || s->parent_result.is_api_result() || s->parent_result.parent_is_proxy()) {
TxnDebug("http_trans", "request not cacheable, so bypass parent");
s->parent_result.result = PARENT_DIRECT;
@@ -3072,6 +3083,8 @@ HttpTransact::OriginServerRawOpen(State *s)
/* fall through */
case CONNECTION_CLOSED:
/* fall through */
+ case OUTBOUND_CONGESTION:
+ /* fall through */
handle_server_died(s);
ink_assert(s->cache_info.action == CACHE_DO_NO_ACTION);
@@ -3272,6 +3285,7 @@ HttpTransact::HandleStatPage(State *s)
void
HttpTransact::handle_response_from_parent(State *s)
{
+ LookingUp_t next_lookup = UNDEFINED_LOOKUP;
TxnDebug("http_trans", "[handle_response_from_parent] (hrfp)");
HTTP_RELEASE_ASSERT(s->current.server == &s->parent_info);
@@ -3294,122 +3308,120 @@ HttpTransact::handle_response_from_parent(State *s)
}
handle_forward_server_connection_open(s);
break;
- default: {
- LookingUp_t next_lookup = UNDEFINED_LOOKUP;
-
- if (s->current.state == PARENT_RETRY) {
- if (s->current.retry_type == PARENT_RETRY_SIMPLE) {
- if (s->current.simple_retry_attempts >= s->parent_result.max_retries(PARENT_RETRY_SIMPLE)) {
- TxnDebug("http_trans", "PARENT_RETRY_SIMPLE: retried all parents, send error to client.");
- s->current.retry_type = PARENT_RETRY_NONE;
- } else {
- s->current.simple_retry_attempts++;
- TxnDebug("http_trans", "PARENT_RETRY_SIMPLE: try another parent.");
- s->current.retry_type = PARENT_RETRY_NONE;
- next_lookup = find_server_and_update_current_info(s);
- }
- } else if (s->current.retry_type == PARENT_RETRY_UNAVAILABLE_SERVER) {
- if (s->current.unavailable_server_retry_attempts >= s->parent_result.max_retries(PARENT_RETRY_UNAVAILABLE_SERVER)) {
- TxnDebug("http_trans", "PARENT_RETRY_UNAVAILABLE_SERVER: retried all parents, send error to client.");
- s->current.retry_type = PARENT_RETRY_NONE;
- } else {
- s->current.unavailable_server_retry_attempts++;
- TxnDebug("http_trans", "PARENT_RETRY_UNAVAILABLE_SERVER: marking parent down and trying another.");
- s->current.retry_type = PARENT_RETRY_NONE;
- HTTP_INCREMENT_DYN_STAT(http_total_parent_marked_down_count);
- s->parent_params->markParentDown(&s->parent_result, s->txn_conf->parent_fail_threshold, s->txn_conf->parent_retry_time);
- next_lookup = find_server_and_update_current_info(s);
- }
+ case PARENT_RETRY:
+ if (s->current.retry_type == PARENT_RETRY_SIMPLE) {
+ if (s->current.simple_retry_attempts >= s->parent_result.max_retries(PARENT_RETRY_SIMPLE)) {
+ TxnDebug("http_trans", "PARENT_RETRY_SIMPLE: retried all parents, send error to client.");
+ s->current.retry_type = PARENT_RETRY_NONE;
+ } else {
+ s->current.simple_retry_attempts++;
+ TxnDebug("http_trans", "PARENT_RETRY_SIMPLE: try another parent.");
+ s->current.retry_type = PARENT_RETRY_NONE;
+ next_lookup = find_server_and_update_current_info(s);
}
- } else {
- TxnDebug("http_trans", "[hrfp] connection not alive");
- SET_VIA_STRING(VIA_DETAIL_PP_CONNECT, VIA_DETAIL_PP_FAILURE);
+ } else if (s->current.retry_type == PARENT_RETRY_UNAVAILABLE_SERVER) {
+ if (s->current.unavailable_server_retry_attempts >= s->parent_result.max_retries(PARENT_RETRY_UNAVAILABLE_SERVER)) {
+ TxnDebug("http_trans", "PARENT_RETRY_UNAVAILABLE_SERVER: retried all parents, send error to client.");
+ s->current.retry_type = PARENT_RETRY_NONE;
+ } else {
+ s->current.unavailable_server_retry_attempts++;
+ TxnDebug("http_trans", "PARENT_RETRY_UNAVAILABLE_SERVER: marking parent down and trying another.");
+ s->current.retry_type = PARENT_RETRY_NONE;
+ HTTP_INCREMENT_DYN_STAT(http_total_parent_marked_down_count);
+ s->parent_params->markParentDown(&s->parent_result, s->txn_conf->parent_fail_threshold, s->txn_conf->parent_retry_time);
+ next_lookup = find_server_and_update_current_info(s);
+ }
+ }
+ break;
+ default:
+ TxnDebug("http_trans", "[hrfp] connection not alive");
+ SET_VIA_STRING(VIA_DETAIL_PP_CONNECT, VIA_DETAIL_PP_FAILURE);
- ink_assert(s->hdr_info.server_request.valid());
+ ink_assert(s->hdr_info.server_request.valid());
- s->current.server->connect_result = ENOTCONN;
- // only mark the parent down in hostdb if the configuration allows it,
- // see proxy.config.http.parent_proxy.mark_down_hostdb in records.config.
- if (s->txn_conf->parent_failures_update_hostdb) {
- s->state_machine->do_hostdb_update_if_necessary();
- }
+ s->current.server->connect_result = ENOTCONN;
+ // only mark the parent down in hostdb if the configuration allows it and the parent
+ // is not congested, see proxy.config.http.parent_proxy.mark_down_hostdb in records.config.
+ if (s->txn_conf->parent_failures_update_hostdb && s->current.state != OUTBOUND_CONGESTION) {
+ s->state_machine->do_hostdb_update_if_necessary();
+ }
- char addrbuf[INET6_ADDRSTRLEN];
- TxnDebug("http_trans", "[%d] failed to connect to parent %s", s->current.attempts,
- ats_ip_ntop(&s->current.server->dst_addr.sa, addrbuf, sizeof(addrbuf)));
+ char addrbuf[INET6_ADDRSTRLEN];
+ TxnDebug("http_trans", "[%d] failed to connect to parent %s", s->current.attempts,
+ ats_ip_ntop(&s->current.server->dst_addr.sa, addrbuf, sizeof(addrbuf)));
- // If the request is not retryable, just give up!
- if (!is_request_retryable(s)) {
+ // If the request is not retryable, just give up!
+ if (!is_request_retryable(s)) {
+ // do not mark the parent down if it is only congested.
+ if (s->current.state != OUTBOUND_CONGESTION) {
HTTP_INCREMENT_DYN_STAT(http_total_parent_marked_down_count);
s->parent_params->markParentDown(&s->parent_result, s->txn_conf->parent_fail_threshold, s->txn_conf->parent_retry_time);
- s->parent_result.result = PARENT_FAIL;
- handle_parent_died(s);
- return;
}
+ s->parent_result.result = PARENT_FAIL;
+ handle_parent_died(s);
+ return;
+ }
- if (s->current.attempts < s->txn_conf->parent_connect_attempts) {
- HTTP_INCREMENT_DYN_STAT(http_total_parent_retries_stat);
- s->current.attempts++;
-
- // Are we done with this particular parent?
- if ((s->current.attempts - 1) % s->txn_conf->per_parent_connect_attempts != 0) {
- // No we are not done with this parent so retry
- HTTP_INCREMENT_DYN_STAT(http_total_parent_switches_stat);
- s->next_action = how_to_open_connection(s);
- TxnDebug("http_trans", "%s Retrying parent for attempt %d, max %" PRId64, "[handle_response_from_parent]",
- s->current.attempts, s->txn_conf->per_parent_connect_attempts);
- return;
- } else {
- TxnDebug("http_trans", "%s %d per parent attempts exhausted", "[handle_response_from_parent]", s->current.attempts);
- HTTP_INCREMENT_DYN_STAT(http_total_parent_retries_exhausted_stat);
-
- // Only mark the parent down if we failed to connect
- // to the parent otherwise slow origin servers cause
- // us to mark the parent down
- if (s->current.state == CONNECTION_ERROR) {
- HTTP_INCREMENT_DYN_STAT(http_total_parent_marked_down_count);
- s->parent_params->markParentDown(&s->parent_result, s->txn_conf->parent_fail_threshold, s->txn_conf->parent_retry_time);
- }
- // We are done so look for another parent if any
- next_lookup = find_server_and_update_current_info(s);
- }
+ if (s->current.attempts < s->txn_conf->parent_connect_attempts) {
+ HTTP_INCREMENT_DYN_STAT(http_total_parent_retries_stat);
+ s->current.attempts++;
+
+ // Are we done with this particular parent?
+ if ((s->current.attempts - 1) % s->txn_conf->per_parent_connect_attempts != 0) {
+ // No we are not done with this parent so retry
+ HTTP_INCREMENT_DYN_STAT(http_total_parent_switches_stat);
+ s->next_action = how_to_open_connection(s);
+ TxnDebug("http_trans", "%s Retrying parent for attempt %d, max %" PRId64, "[handle_response_from_parent]",
+ s->current.attempts, s->txn_conf->per_parent_connect_attempts);
+ return;
} else {
- // Done trying parents... fail over to origin server if that is
- // appropriate
+ TxnDebug("http_trans", "%s %d per parent attempts exhausted", "[handle_response_from_parent]", s->current.attempts);
HTTP_INCREMENT_DYN_STAT(http_total_parent_retries_exhausted_stat);
- TxnDebug("http_trans", "[handle_response_from_parent] Error. No more retries.");
+
+ // Only mark the parent down if we failed to connect
+ // to the parent otherwise slow origin servers cause
+ // us to mark the parent down
if (s->current.state == CONNECTION_ERROR) {
HTTP_INCREMENT_DYN_STAT(http_total_parent_marked_down_count);
s->parent_params->markParentDown(&s->parent_result, s->txn_conf->parent_fail_threshold, s->txn_conf->parent_retry_time);
}
- s->parent_result.result = PARENT_FAIL;
- next_lookup = find_server_and_update_current_info(s);
+ // We are done so look for another parent if any
+ next_lookup = find_server_and_update_current_info(s);
}
+ } else {
+ // Done trying parents... fail over to origin server if that is
+ // appropriate
+ HTTP_INCREMENT_DYN_STAT(http_total_parent_retries_exhausted_stat);
+ TxnDebug("http_trans", "[handle_response_from_parent] Error. No more retries.");
+ if (s->current.state == CONNECTION_ERROR) {
+ HTTP_INCREMENT_DYN_STAT(http_total_parent_marked_down_count);
+ s->parent_params->markParentDown(&s->parent_result, s->txn_conf->parent_fail_threshold, s->txn_conf->parent_retry_time);
+ }
+ s->parent_result.result = PARENT_FAIL;
+ next_lookup = HOST_NONE;
}
-
- // We have either tried to find a new parent or failed over to the
- // origin server
- switch (next_lookup) {
- case PARENT_PROXY:
- ink_assert(s->current.request_to == PARENT_PROXY);
- TRANSACT_RETURN(SM_ACTION_DNS_LOOKUP, PPDNSLookup);
- break;
- case ORIGIN_SERVER:
- // Next lookup is Origin Server, try DNS for Origin Server
- TRANSACT_RETURN(SM_ACTION_DNS_LOOKUP, OSDNSLookup);
- break;
- case HOST_NONE:
- handle_parent_died(s);
- break;
- default:
- // This handles:
- // UNDEFINED_LOOKUP
- // INCOMING_ROUTER
- break;
- }
-
break;
}
+
+ // We have either tried to find a new parent or failed over to the
+ // origin server
+ switch (next_lookup) {
+ case PARENT_PROXY:
+ ink_assert(s->current.request_to == PARENT_PROXY);
+ TRANSACT_RETURN(SM_ACTION_DNS_LOOKUP, PPDNSLookup);
+ break;
+ case ORIGIN_SERVER:
+ // Next lookup is Origin Server, try DNS for Origin Server
+ TRANSACT_RETURN(SM_ACTION_DNS_LOOKUP, OSDNSLookup);
+ break;
+ case HOST_NONE:
+ handle_parent_died(s);
+ break;
+ default:
+ // This handles:
+ // UNDEFINED_LOOKUP
+ // INCOMING_ROUTER
+ break;
}
}
@@ -3450,6 +3462,12 @@ HttpTransact::handle_response_from_server(State *s)
s->current.server->clear_connect_fail();
handle_forward_server_connection_open(s);
break;
+ case OUTBOUND_CONGESTION:
+ TxnDebug("http_trans", "[handle_response_from_server] Error. congestion control -- congested.");
+ SET_VIA_STRING(VIA_DETAIL_SERVER_CONNECT, VIA_DETAIL_SERVER_FAILURE);
+ s->current.server->set_connect_fail(EUSERS); // too many users
+ handle_server_connection_not_open(s);
+ break;
case OPEN_RAW_ERROR:
/* fall through */
case CONNECTION_ERROR:
@@ -6306,7 +6324,8 @@ HttpTransact::is_response_valid(State *s, HTTPHdr *incoming_response)
if (s->current.state != CONNECTION_ALIVE) {
ink_assert((s->current.state == CONNECTION_ERROR) || (s->current.state == OPEN_RAW_ERROR) ||
(s->current.state == PARSE_ERROR) || (s->current.state == CONNECTION_CLOSED) ||
- (s->current.state == INACTIVE_TIMEOUT) || (s->current.state == ACTIVE_TIMEOUT));
+ (s->current.state == INACTIVE_TIMEOUT) || (s->current.state == ACTIVE_TIMEOUT) ||
+ s->current.state == OUTBOUND_CONGESTION);
s->hdr_info.response_error = CONNECTION_OPEN_FAILED;
return false;
@@ -7350,7 +7369,12 @@ HttpTransact::handle_parent_died(State *s)
{
ink_assert(s->parent_result.result == PARENT_FAIL);
- build_error_response(s, HTTP_STATUS_BAD_GATEWAY, "Next Hop Connection Failed", "connect#failed_connect");
+ // congestion to a parent multi-site origin server
+ if (s->current.state == OUTBOUND_CONGESTION) {
+ build_error_response(s, HTTP_STATUS_SERVICE_UNAVAILABLE, "Next Hop Congested", "congestion#retryAfter");
+ } else {
+ build_error_response(s, HTTP_STATUS_BAD_GATEWAY, "Next Hop Connection Failed", "connect#failed_connect");
+ }
TRANSACT_RETURN(SM_ACTION_SEND_ERROR_CACHE_NOOP, nullptr);
}
@@ -7409,6 +7433,12 @@ HttpTransact::handle_server_died(State *s)
reason = "Invalid HTTP Response";
body_type = "response#bad_response";
break;
+ case OUTBOUND_CONGESTION:
+ status = HTTP_STATUS_SERVICE_UNAVAILABLE;
+ reason = "Origin server congested";
+ body_type = "congestion#retryAfter";
+ s->hdr_info.response_error = TOTAL_RESPONSE_ERROR_TYPES;
+ break;
case STATE_UNDEFINED:
case TRANSACTION_COMPLETE:
default: /* unknown death */
diff --git a/proxy/http/HttpTransact.h b/proxy/http/HttpTransact.h
index e3ee61b..1178f41 100644
--- a/proxy/http/HttpTransact.h
+++ b/proxy/http/HttpTransact.h
@@ -349,7 +349,8 @@ public:
OPEN_RAW_ERROR,
PARSE_ERROR,
TRANSACTION_COMPLETE,
- PARENT_RETRY
+ PARENT_RETRY,
+ OUTBOUND_CONGESTION
};
enum CacheWriteStatus_t {