From: Thomas Markwalder Date: Mon, 28 Oct 2019 15:19:53 +0000 (-0400) Subject: [#964,!577] Revamped to detect and close OOB Connections X-Git-Tag: Kea-1.7.2~82 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=ed13fb5c6394358ffeb1aef1f6f77781ee6a1b61;p=thirdparty%2Fkea.git [#964,!577] Revamped to detect and close OOB Connections Rather than just unregistering the socket, we now actually close the Connection. This ensures we never end up with an unregistered but open connection. src/hooks/dhcp/high_availability/ha_service.* HAService::clientConnectHandler() - modified to call HttpClient::closeIfOutOfBandwidth(). src/lib/http/client.* Connection - replaced isTransaction(int socket_fd) with isMySocket(int socket_fd) ConnectionPool - replaced isTransaction(int socket_fd) with closeIfOutOfBandwidth(int socket_fd) HttpClient - replaced isTransaction(int socket_fd) with closeIfOutOfBandwidth(int socket_fd) --- diff --git a/src/hooks/dhcp/high_availability/ha_service.cc b/src/hooks/dhcp/high_availability/ha_service.cc index ec78a5d74d..77106bef18 100644 --- a/src/hooks/dhcp/high_availability/ha_service.cc +++ b/src/hooks/dhcp/high_availability/ha_service.cc @@ -1647,10 +1647,10 @@ HAService::clientConnectHandler(const boost::system::error_code& ec, int tcp_nat void HAService::socketReadyHandler(int tcp_native_fd) { // If the socket is ready but does not belong to one of our client's - // ongoing transactions we unregister it. - if (!client_.isTransactionOngoing(tcp_native_fd)) { - IfaceMgr::instance().deleteExternalSocket(tcp_native_fd); - } + // ongoing transactions, we close it. This will unregister it from + // IfaceMgr and ensure the client starts over with a fresh connection + // if it needs to do so. + client_.closeIfOutOfBandwidth(tcp_native_fd); } void diff --git a/src/hooks/dhcp/high_availability/ha_service.h b/src/hooks/dhcp/high_availability/ha_service.h index d364fb1ddd..68129f5744 100644 --- a/src/hooks/dhcp/high_availability/ha_service.h +++ b/src/hooks/dhcp/high_availability/ha_service.h @@ -744,16 +744,14 @@ protected: /// flagged as ready to read. It is installed by the invocation to /// register the socket with IfaceMgr made in @ref clientConnectHandler. /// - /// The handler checks if the ready socket belongs to one our client's - /// ongoing transactions. If it does, it simply returns. If it does - /// not belong to an ongoing transactions, the socket descriptor is - /// unregistered from IfaceMgr. + /// The handler calls @ref HttpClient::closeIfOutOfBandwidth() to catch + /// and close any sockets that have gone ready outside of transactions. /// /// We do this in case the other peer closed the socket (e.g. idle timeout /// for example), as this will cause the socket /appear ready to read to - /// the IfaceMgr::select(). If this happens in while no transcations are + /// the IfaceMgr::select(). If this happens while no transcations are /// in progess, we won't have anything to deal with the socket event. - /// This causes IfaceMgr::select() to endlessly interrupt on this socket. + /// This causes IfaceMgr::select() to endlessly interrupt on the socket. /// /// @param tcp_native_fd socket descriptor of the ready socket void socketReadyHandler(int tcp_native_fd); diff --git a/src/lib/http/client.cc b/src/lib/http/client.cc index e056170efd..2593359ed7 100644 --- a/src/lib/http/client.cc +++ b/src/lib/http/client.cc @@ -139,11 +139,12 @@ public: /// @return true if transaction has been initiated, false otherwise. bool isTransactionOngoing() const; - /// @brief Checks if a transaction has been initiated over this connection and - /// socket descriptor. + /// @brief Checks if a socket descriptor belongs to this connection. /// - /// @return true if transaction has been initiated, false otherwise. - bool isTransactionOngoing(int socket_fd) const; + /// @param socket_fd socket descriptor to check + /// + /// @return True if the socket fd belongs to this connection. + bool isMySocket(int socket_fd) const; /// @brief Checks and logs if premature transaction timeout is suspected. /// @@ -445,21 +446,41 @@ public: queue_.clear(); } - /// @brief Deteremines if there is an ongoing transaction associated - /// with a given socket desecriptor. + /// @brief Closes a connection if it has an out-of-bandwidth socket event /// - /// @param socket_fd socket descriptor to check + /// If the pool contains a connection using the given socket and that + /// connection is currently in a transaction the method returns as this + /// indicates a normal ready event. If the connection is not in an + /// ongoing transaction, then the connection is closed. /// - /// @return True if the pool contains a connection which is using the - /// given socket descriptor for an ongoing transaction. - bool isTransactionOngoing(int socket_fd) { + /// This is method is intended to be used to detect and clean up then + /// sockets that are marked ready outside of transactions. The most comman + /// case is the other end of the socket being closed. + /// + /// @param socket_fd socket descriptor to check + void closeIfOutOfBandwidth(int socket_fd) { + // First we look for a connection with the socket. for (auto conns_it = conns_.begin(); conns_it != conns_.end(); ++conns_it) { - if (conns_it->second->isTransactionOngoing(socket_fd)) { - return (true); + + if (!conns_it->second->isMySocket(socket_fd)) { + // Not this connection. + continue; } + + if (conns_it->second->isTransactionOngoing()) { + // Matches but is in a transaction, all is well. + return; + } + + // Socket has no transaction, so any ready event is + // out-of-bandwidth (other end probably closed), so + // let's close it. Note we do not remove any queued + // requests, as this might somehow be occurring in + // between them. + conns_it->second->close(); + conns_.erase(conns_it); } - return (false); } private: @@ -631,9 +652,8 @@ Connection::isTransactionOngoing() const { } bool -Connection::isTransactionOngoing(int socket_fd) const { - return ((static_cast(current_request_)) && - (socket_.getNative() == socket_fd)); +Connection::isMySocket(int socket_fd) const { + return (socket_.getNative() == socket_fd); } bool @@ -952,9 +972,9 @@ HttpClient::asyncSendRequest(const Url& url, const HttpRequestPtr& request, request_callback, connect_callback, close_callback); } -bool -HttpClient::isTransactionOngoing(int socket_fd) const { - return (impl_->conn_pool_->isTransactionOngoing(socket_fd)); +void +HttpClient::closeIfOutOfBandwidth(int socket_fd) { + return (impl_->conn_pool_->closeIfOutOfBandwidth(socket_fd)); } void diff --git a/src/lib/http/client.h b/src/lib/http/client.h index 95f1af9779..9b9aff751f 100644 --- a/src/lib/http/client.h +++ b/src/lib/http/client.h @@ -191,11 +191,19 @@ public: /// @brief Closes all connections. void stop(); - /// @brief Deteremines if a socket is part of an ongoing transaction. + /// @brief Closes a connection if it has an out-of-bandwidth socket event /// - /// @return True if the given socket descriptor is being used by an - /// ongoing transaction - bool isTransactionOngoing(int socket_fd) const; + /// If the client owns a connection using the given socket and that + /// connection is currently in a transaction the method returns as this + /// indicates a normal ready event. If the connection is not in an + /// ongoing transaction, then the connection is closed. + /// + /// This is method is intended to be used to detect and clean up then + /// sockets that are marked ready outside of transactions. The most comman + /// case is the other end of the socket being closed. + /// + /// @param socket_fd socket descriptor to check + void closeIfOutOfBandwidth(int socket_fd); private: