diff --git a/include/proxy/http/HttpTransact.h b/include/proxy/http/HttpTransact.h index 64e13a991de..a2d00afd99b 100644 --- a/include/proxy/http/HttpTransact.h +++ b/include/proxy/http/HttpTransact.h @@ -512,8 +512,8 @@ class HttpTransact bool receive_chunked_response = false; bool proxy_connect_hdr = false; /// @c errno from the most recent attempt to connect. - /// zero means no failure (not attempted, succeeded). - int connect_result = 0; + /// zero means succeeded. + int connect_result = -UNKNOWN_INTERNAL_ERROR; char *name = nullptr; swoc::IPAddr name_addr; TransferEncoding_t transfer_encoding = TransferEncoding_t::NONE; @@ -539,10 +539,10 @@ class HttpTransact bool had_connect_fail() const { - return 0 != connect_result; + return 0 != connect_result && -UNKNOWN_INTERNAL_ERROR != connect_result; } void - clear_connect_fail() + set_connect_success() { connect_result = 0; } @@ -553,7 +553,7 @@ class HttpTransact { ink_zero(src_addr); ink_zero(dst_addr); - connect_result = 0; + connect_result = -UNKNOWN_INTERNAL_ERROR; } }; @@ -751,7 +751,7 @@ class HttpTransact int method = 0; bool method_metric_incremented = false; - /// The errno associated with a failed connect attempt. + /// The errno that caused the HTTP error response to be returned. /// /// This is used for logging and (in some code paths) for determing HTTP /// response reason phrases. @@ -922,22 +922,32 @@ class HttpTransact ProxyProtocol pp_info; void - set_connect_fail(int e) + set_fail(int e) { - int const original_connect_result = this->current.server->connect_result; - if (e == EUSERS) { + const auto original_connect_result = this->current.server->connect_result; + const auto connection_succeeded = (original_connect_result == 0); + if (this->next_action == StateMachineAction_t::ORIGIN_SERVER_OPEN && !connection_succeeded) { // EUSERS is used when the number of connections exceeds the configured // limit. Since this is not a network connectivity issue with the // server, we should not mark it as such. Otherwise we will incorrectly // mark the server as down. - this->current.server->connect_result = 0; - } else if (e == EIO || this->current.server->connect_result == EIO) { - this->current.server->connect_result = e; + if (e != EUSERS) { + this->current.server->connect_result = e; + Dbg(_dbg_ctl, "Setting upstream connection failure %d to %d", original_connect_result, + this->current.server->connect_result); + } } - if (e != EIO) { - this->cause_of_death_errno = e; + this->cause_of_death_errno = e; + } + + void + set_success() + { + const auto connection_succeeded = (this->current.server->connect_result == 0); + if (this->next_action == StateMachineAction_t::ORIGIN_SERVER_OPEN && !connection_succeeded) { + this->current.server->set_connect_success(); } - Dbg(_dbg_ctl, "Setting upstream connection failure %d to %d", original_connect_result, this->current.server->connect_result); + this->cause_of_death_errno = 0; } MgmtInt diff --git a/src/proxy/http/ConnectingEntry.cc b/src/proxy/http/ConnectingEntry.cc index 992602439ed..36cf75614be 100644 --- a/src/proxy/http/ConnectingEntry.cc +++ b/src/proxy/http/ConnectingEntry.cc @@ -130,7 +130,7 @@ ConnectingEntry::state_http_server_open(int event, void *data) Dbg(dbg_ctl_http_connect, "Stop %zd state machines waiting for failed origin", connect_sms.size()); this->remove_entry(); int vc_provided_cert = 0; - int lerrno = EIO; + int lerrno = -UNKNOWN_INTERNAL_ERROR; if (netvc != nullptr) { vc_provided_cert = netvc->provided_cert(); lerrno = netvc->lerrno == 0 ? lerrno : netvc->lerrno; @@ -139,7 +139,9 @@ ConnectingEntry::state_http_server_open(int event, void *data) while (!connect_sms.empty()) { auto entry = connect_sms.begin(); SCOPED_MUTEX_LOCK(lock, (*entry)->mutex, this_ethread()); - (*entry)->t_state.set_connect_fail(lerrno); + if (lerrno != -UNKNOWN_INTERNAL_ERROR) { + (*entry)->t_state.set_fail(lerrno); + } (*entry)->server_connection_provided_cert = vc_provided_cert; (*entry)->handleEvent(event, data); connect_sms.erase(entry); diff --git a/src/proxy/http/HttpSM.cc b/src/proxy/http/HttpSM.cc index 87e8c859456..77b6815d875 100644 --- a/src/proxy/http/HttpSM.cc +++ b/src/proxy/http/HttpSM.cc @@ -1112,7 +1112,7 @@ HttpSM::state_raw_http_server_open(int event, void *data) netvc->set_inactivity_timeout(get_server_inactivity_timeout()); netvc->set_active_timeout(get_server_active_timeout()); - t_state.current.server->clear_connect_fail(); + t_state.set_success(); if (get_tunnel_type() != SNIRoutingType::NONE) { tunnel.mark_tls_tunnel_active(); @@ -1125,9 +1125,9 @@ HttpSM::state_raw_http_server_open(int event, void *data) case NET_EVENT_OPEN_FAILED: if (t_state.cause_of_death_errno == -UNKNOWN_INTERNAL_ERROR) { if (event == VC_EVENT_EOS) { - t_state.set_connect_fail(EPIPE); + t_state.set_fail(EPIPE); } else { - t_state.set_connect_fail(EIO); + t_state.set_fail(EIO); } } t_state.current.state = HttpTransact::OPEN_RAW_ERROR; @@ -1822,10 +1822,6 @@ HttpSM::state_http_server_open(int event, void *data) // The buffer we create will be handed over to the eventually created server session _netvc->do_io_write(this, 1, _netvc_reader); _netvc->set_inactivity_timeout(this->get_server_connect_timeout()); - - // Pre-emptively set a server connect failure that will be cleared once a WRITE_READY is received from origin or - // bytes are received back - t_state.set_connect_fail(EIO); } else { // in the case of an intercept plugin don't to the connect timeout change SMDbg(dbg_ctl_http_connect, "not setting handler for connection handshake"); this->create_server_txn(this->create_server_session(*_netvc, _netvc_read_buffer, _netvc_reader)); @@ -1844,7 +1840,6 @@ HttpSM::state_http_server_open(int event, void *data) case CONNECT_EVENT_TXN: SMDbg(dbg_ctl_http, "Connection handshake complete via CONNECT_EVENT_TXN"); if (this->create_server_txn(static_cast(data))) { - t_state.current.server->clear_connect_fail(); handle_http_server_open(); } else { // Failed to create transaction. Maybe too many active transactions already // Try again (probably need a bounding counter here) @@ -1857,12 +1852,11 @@ HttpSM::state_http_server_open(int event, void *data) // Update the time out to the regular connection timeout. SMDbg(dbg_ctl_http_ss, "Connection handshake complete"); this->create_server_txn(this->create_server_session(*_netvc, _netvc_read_buffer, _netvc_reader)); - t_state.current.server->clear_connect_fail(); handle_http_server_open(); return 0; case VC_EVENT_INACTIVITY_TIMEOUT: case VC_EVENT_ACTIVE_TIMEOUT: - t_state.set_connect_fail(ETIMEDOUT); + t_state.set_fail(ETIMEDOUT); /* fallthrough */ case VC_EVENT_ERROR: case VC_EVENT_EOS: @@ -1871,7 +1865,7 @@ HttpSM::state_http_server_open(int event, void *data) t_state.outbound_conn_track_state.clear(); if (_netvc != nullptr) { if (event == VC_EVENT_ERROR || event == NET_EVENT_OPEN_FAILED) { - t_state.set_connect_fail(_netvc->lerrno); + t_state.set_fail(_netvc->lerrno); } this->server_connection_provided_cert = _netvc->provided_cert(); _netvc->do_io_write(nullptr, 0, nullptr); @@ -2043,7 +2037,7 @@ HttpSM::state_read_server_response_header(int event, void *data) if (allow_error == false) { SMDbg(dbg_ctl_http_seq, "Error parsing server response header"); t_state.current.state = HttpTransact::PARSE_ERROR; - t_state.set_connect_fail(EBADMSG); + t_state.set_fail(EBADMSG); // If the server closed prematurely on us, use the // server setup error routine since it will forward @@ -2268,9 +2262,6 @@ HttpSM::add_to_existing_request() // Check that entry matches sni, hostname, and cert. if (proposed_hostname == ip_iter->second->hostname && proposed_sni == ip_iter->second->sni && proposed_cert == ip_iter->second->cert_name && ip_iter->second->connect_sms.size() < 50) { - // Pre-emptively set a server connect failure that will be cleared once a - // WRITE_READY is received from origin or bytes are received back. - this->t_state.set_connect_fail(EIO); ip_iter->second->connect_sms.insert(this); Dbg(dbg_ctl_http_connect, "Add entry to connection queue. size=%zd", ip_iter->second->connect_sms.size()); retval = true; @@ -3938,19 +3929,19 @@ HttpSM::tunnel_handler_post_server(int event, HttpTunnelConsumer *c) switch (event) { case VC_EVENT_INACTIVITY_TIMEOUT: t_state.current.state = HttpTransact::INACTIVE_TIMEOUT; - t_state.set_connect_fail(ETIMEDOUT); + t_state.set_fail(ETIMEDOUT); break; case VC_EVENT_ACTIVE_TIMEOUT: t_state.current.state = HttpTransact::ACTIVE_TIMEOUT; - t_state.set_connect_fail(ETIMEDOUT); + t_state.set_fail(ETIMEDOUT); break; case VC_EVENT_EOS: t_state.current.state = HttpTransact::CONNECTION_CLOSED; - t_state.set_connect_fail(EPIPE); + t_state.set_fail(EPIPE); break; case VC_EVENT_ERROR: t_state.current.state = HttpTransact::CONNECTION_CLOSED; - t_state.set_connect_fail(server_txn->get_netvc()->lerrno); + t_state.set_fail(server_txn->get_netvc()->lerrno); break; default: break; @@ -5156,7 +5147,7 @@ HttpSM::send_origin_throttled_response() t_state.current.retry_attempts.maximize(t_state.configured_connect_attempts_max_retries()); } if (t_state.cause_of_death_errno == -UNKNOWN_INTERNAL_ERROR) { - t_state.set_connect_fail(EUSERS); // Too many users. + t_state.set_fail(EUSERS); // Too many users. } t_state.current.state = HttpTransact::OUTBOUND_CONGESTION; call_transact_and_set_next_state(HttpTransact::HandleResponse); @@ -5569,7 +5560,7 @@ HttpSM::do_http_server_open(bool raw, bool only_direct) // Eventually may want to have a queue as the origin_max_connection does to allow for a combination // of retries and errors. But at this point, we are just going to allow the error case. if (t_state.cause_of_death_errno == -UNKNOWN_INTERNAL_ERROR) { - t_state.set_connect_fail(ENFILE); // Too many open files in system. + t_state.set_fail(ENFILE); // Too many open files in system. } t_state.current.state = HttpTransact::CONNECTION_ERROR; call_transact_and_set_next_state(HttpTransact::HandleResponse); @@ -5716,7 +5707,6 @@ HttpSM::do_http_server_open(bool raw, bool only_direct) new_entry->sni = this->get_outbound_sni(); new_entry->cert_name = this->get_outbound_cert(); new_entry->is_no_plugin_tunnel = plugin_tunnel_type == HttpPluginTunnel_t::NONE; - this->t_state.set_connect_fail(EIO); new_entry->connect_sms.insert(this); ethread->connecting_pool->m_ip_pool.insert(std::make_pair(new_entry->ipaddr, new_entry)); } @@ -6044,7 +6034,7 @@ HttpSM::handle_post_failure() tunnel.reset(); // Server is down if (t_state.current.state == HttpTransact::STATE_UNDEFINED || t_state.current.state == HttpTransact::CONNECTION_ALIVE) { - t_state.set_connect_fail(server_txn->get_netvc()->lerrno); + t_state.set_fail(server_txn->get_netvc()->lerrno); t_state.current.state = HttpTransact::CONNECTION_CLOSED; } call_transact_and_set_next_state(HttpTransact::HandleResponse); @@ -6059,6 +6049,8 @@ HttpSM::handle_post_failure() void HttpSM::handle_http_server_open() { + t_state.set_success(); + // [bwyatt] applying per-transaction OS netVC options here // IFF they differ from the netVC's current options. // This should keep this from being redundant on a @@ -6166,14 +6158,14 @@ HttpSM::handle_server_setup_error(int event, void *data) switch (event) { case VC_EVENT_EOS: t_state.current.state = HttpTransact::CONNECTION_CLOSED; - t_state.set_connect_fail(EPIPE); + t_state.set_fail(EPIPE); break; case VC_EVENT_ERROR: t_state.current.state = HttpTransact::CONNECTION_ERROR; - t_state.set_connect_fail(server_txn->get_netvc()->lerrno); + t_state.set_fail(server_txn->get_netvc()->lerrno); break; case VC_EVENT_ACTIVE_TIMEOUT: - t_state.set_connect_fail(ETIMEDOUT); + t_state.set_fail(ETIMEDOUT); t_state.current.state = HttpTransact::ACTIVE_TIMEOUT; break; @@ -6183,7 +6175,7 @@ HttpSM::handle_server_setup_error(int event, void *data) // server failed // In case of TIMEOUT, the iocore sends back // server_entry->read_vio instead of the write_vio - t_state.set_connect_fail(ETIMEDOUT); + t_state.set_fail(ETIMEDOUT); if (server_entry->write_vio && server_entry->write_vio->nbytes > 0 && server_entry->write_vio->ndone == 0) { t_state.current.state = HttpTransact::CONNECTION_ERROR; } else { @@ -8228,9 +8220,6 @@ HttpSM::set_next_state() } case HttpTransact::StateMachineAction_t::ORIGIN_SERVER_RAW_OPEN: { - // Pre-emptively set a server connect failure that will be cleared once a WRITE_READY is received from origin or - // bytes are received back - t_state.set_connect_fail(EIO); HTTP_SM_SET_DEFAULT_HANDLER(&HttpSM::state_raw_http_server_open); ink_assert(server_entry == nullptr); diff --git a/src/proxy/http/HttpTransact.cc b/src/proxy/http/HttpTransact.cc index aba3e30b7e5..7a894f145e9 100644 --- a/src/proxy/http/HttpTransact.cc +++ b/src/proxy/http/HttpTransact.cc @@ -3559,7 +3559,7 @@ HttpTransact::handle_response_from_parent(State *s) switch (s->current.state) { case CONNECTION_ALIVE: TxnDbg(dbg_ctl_http_trans, "[hrfp] connection alive"); - s->current.server->connect_result = 0; + s->set_success(); SET_VIA_STRING(VIA_DETAIL_PP_CONNECT, VIA_DETAIL_PP_SUCCESS); if (s->parent_result.retry) { markParentUp(s); @@ -3714,13 +3714,13 @@ HttpTransact::handle_response_from_server(State *s) case CONNECTION_ALIVE: TxnDbg(dbg_ctl_http_trans, "[hrfs] connection alive"); SET_VIA_STRING(VIA_DETAIL_SERVER_CONNECT, VIA_DETAIL_SERVER_SUCCESS); - s->current.server->clear_connect_fail(); + s->set_success(); handle_forward_server_connection_open(s); break; case OUTBOUND_CONGESTION: TxnDbg(dbg_ctl_http_trans, "Error. congestion control -- congested."); SET_VIA_STRING(VIA_DETAIL_SERVER_CONNECT, VIA_DETAIL_SERVER_FAILURE); - s->set_connect_fail(EUSERS); // too many users + s->set_fail(EUSERS); // too many users handle_server_connection_not_open(s); break; case OPEN_RAW_ERROR: @@ -3735,13 +3735,13 @@ HttpTransact::handle_response_from_server(State *s) // This prevents the assertion failure in retry_server_connection_not_open. if (s->cause_of_death_errno == -UNKNOWN_INTERNAL_ERROR) { if (s->current.state == PARSE_ERROR || s->current.state == BAD_INCOMING_RESPONSE) { - s->set_connect_fail(EBADMSG); + s->set_fail(EBADMSG); } else if (s->current.state == CONNECTION_CLOSED) { - s->set_connect_fail(EPIPE); + s->set_fail(EPIPE); } else { // Generic fallback for OPEN_RAW_ERROR, CONNECTION_ERROR, // STATE_UNDEFINED, and any other unexpected error states. - s->set_connect_fail(EIO); + s->set_fail(EIO); } } @@ -3801,7 +3801,7 @@ HttpTransact::handle_response_from_server(State *s) case ACTIVE_TIMEOUT: TxnDbg(dbg_ctl_http_trans, "[hrfs] connection not alive"); SET_VIA_STRING(VIA_DETAIL_SERVER_CONNECT, VIA_DETAIL_SERVER_FAILURE); - s->set_connect_fail(ETIMEDOUT); + s->set_fail(ETIMEDOUT); handle_server_connection_not_open(s); break; default: diff --git a/src/proxy/http/HttpTunnel.cc b/src/proxy/http/HttpTunnel.cc index feb863cf13a..c58e8324cea 100644 --- a/src/proxy/http/HttpTunnel.cc +++ b/src/proxy/http/HttpTunnel.cc @@ -1512,7 +1512,7 @@ HttpTunnel::consumer_handler(int event, HttpTunnelConsumer *c) this->consumer_reenable(c); // Once we get a write ready from the origin, we can assume the connect to some degree succeeded if (c->vc_type == HttpTunnelType_t::HTTP_SERVER) { - sm->t_state.current.server->clear_connect_fail(); + sm->t_state.set_success(); } break;