Cleanup connect result handling

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

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);

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<PoolableSession *>(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);

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:

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;