From: Thomas Markwalder Date: Wed, 26 Jun 2019 13:06:26 +0000 (-0400) Subject: [#691,!395] Addressed review comments 1 X-Git-Tag: Kea-1.6.0-beta2~197 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=de3af2b8ec92200f78a73f82cef3e9d4aceffa97;p=thirdparty%2Fkea.git [#691,!395] Addressed review comments 1 Added TIMEOUT_DEFAULT_HTTP_CLIENT_REQUEST Removed virtual from callback declarations Added commentary to http/client.h --- diff --git a/src/hooks/dhcp/high_availability/ha_service.cc b/src/hooks/dhcp/high_availability/ha_service.cc index fc5cb59e0e..3a9f74513f 100644 --- a/src/hooks/dhcp/high_availability/ha_service.cc +++ b/src/hooks/dhcp/high_availability/ha_service.cc @@ -12,6 +12,7 @@ #include #include #include +#include #include #include #include @@ -854,7 +855,7 @@ HAService::asyncSendLeaseUpdate(const QueryPtrType& query, runModel(HA_LEASE_UPDATES_COMPLETE_EVT); } }, - HttpClient::RequestTimeout(10000), + HttpClient::RequestTimeout(TIMEOUT_DEFAULT_HTTP_CLIENT_REQUEST), boost::bind(&HAService::clientConnectHandler, this, _1, _2), boost::bind(&HAService::clientCloseHandler, this, _1) ); @@ -1057,7 +1058,7 @@ HAService::asyncSendHeartbeat() { startHeartbeat(); runModel(HA_HEARTBEAT_COMPLETE_EVT); }, - HttpClient::RequestTimeout(10000), + HttpClient::RequestTimeout(TIMEOUT_DEFAULT_HTTP_CLIENT_REQUEST), boost::bind(&HAService::clientConnectHandler, this, _1, _2), boost::bind(&HAService::clientCloseHandler, this, _1) ); @@ -1147,7 +1148,7 @@ HAService::asyncDisableDHCPService(HttpClient& http_client, error_message); } }, - HttpClient::RequestTimeout(10000), + HttpClient::RequestTimeout(TIMEOUT_DEFAULT_HTTP_CLIENT_REQUEST), boost::bind(&HAService::clientConnectHandler, this, _1, _2), boost::bind(&HAService::clientCloseHandler, this, _1) ); @@ -1218,7 +1219,7 @@ HAService::asyncEnableDHCPService(HttpClient& http_client, error_message); } }, - HttpClient::RequestTimeout(10000), + HttpClient::RequestTimeout(TIMEOUT_DEFAULT_HTTP_CLIENT_REQUEST), boost::bind(&HAService::clientConnectHandler, this, _1, _2), boost::bind(&HAService::clientCloseHandler, this, _1) ); @@ -1621,12 +1622,10 @@ HAService::verifyAsyncResponse(const HttpResponsePtr& response) { bool HAService::clientConnectHandler(const boost::system::error_code& ec, int tcp_native_fd) { if (!ec || ec.value() == boost::asio::error::in_progress) { - IfaceMgr::instance().addExternalSocket(tcp_native_fd, - [this]() { - // Callback is a NOP. Ready events handlers are run by an explicit - // call IOService ready in kea-dhcp code. We are registering - // the socket to interrupt main-thread select(). - }); + // External socket callback is a NOP. Ready events handlers are run by an + // explicit call IOService ready in kea-dhcp code. We are registering + // the socket only to interrupt main-thread select(). + IfaceMgr::instance().addExternalSocket(tcp_native_fd, 0); } // If ec.value() == boost::asio::error::already_connected, we should already diff --git a/src/hooks/dhcp/high_availability/ha_service.h b/src/hooks/dhcp/high_availability/ha_service.h index 770b7add98..b89a1055e4 100644 --- a/src/hooks/dhcp/high_availability/ha_service.h +++ b/src/hooks/dhcp/high_availability/ha_service.h @@ -736,7 +736,7 @@ protected: /// @param tcp_native_fd socket descriptor to register /// @param returns true. Registeration cannot fail, and if ec indicates a real /// error we want Connection logic to process it. - virtual bool clientConnectHandler(const boost::system::error_code& ec, int tcp_native_fd); + bool clientConnectHandler(const boost::system::error_code& ec, int tcp_native_fd); /// @brief HttpClient close callback handler /// @@ -745,7 +745,7 @@ protected: /// main-thread select()). /// /// @param tcp_native_fd socket descriptor to register - virtual void clientCloseHandler(int tcp_native_fd); + void clientCloseHandler(int tcp_native_fd); /// @brief Pointer to the IO service object shared between this hooks /// library and the DHCP server. diff --git a/src/lib/config/timeouts.h b/src/lib/config/timeouts.h index d20de9a728..56b6a11200 100644 --- a/src/lib/config/timeouts.h +++ b/src/lib/config/timeouts.h @@ -30,6 +30,14 @@ constexpr long TIMEOUT_AGENT_IDLE_CONNECTION_TIMEOUT = 30000; /// to generate large responses, e.g. dump whole lease databse. constexpr long TIMEOUT_AGENT_FORWARD_COMMAND = 60000; +/// @brief Timeout for the HTTP clients awaiting a response to a request. +/// +/// This value is high to ensure that the client waits long enough +/// for the fulfilling server to generate a response. Specified +/// milliseconds. +constexpr long TIMEOUT_DEFAULT_HTTP_CLIENT_REQUEST = 10000; + + } // end of namespace isc::config } // end of namespace isc diff --git a/src/lib/dhcp/iface_mgr.cc b/src/lib/dhcp/iface_mgr.cc index 96eef3685e..d66bf0fcbf 100644 --- a/src/lib/dhcp/iface_mgr.cc +++ b/src/lib/dhcp/iface_mgr.cc @@ -351,7 +351,7 @@ IfaceMgr::purgeBadSockets() { std::vector bad_fds; BOOST_FOREACH(SocketCallbackInfo s, callbacks_) { errno = 0; - if (fcntl(s.socket_, F_GETFD) < 0 && errno == EBADF) { + if (fcntl(s.socket_, F_GETFD) < 0 && (errno == EBADF)) { bad_fds.push_back(s.socket_); } } diff --git a/src/lib/http/client.h b/src/lib/http/client.h index 46ec5c8615..90c64820af 100644 --- a/src/lib/http/client.h +++ b/src/lib/http/client.h @@ -59,6 +59,12 @@ class HttpClientImpl; /// a request by trying to read from the socket (with message peeking). If /// the socket is usable the client uses it to transmit the request. /// +/// This classes exposes the underlying TCP socket's descriptor for each +/// connection via connect and close callbacks. This is done to permit the +/// sockets to be monitored for IO readiness by external code that something +/// other than boost::asio (e.g.select() or epoll()), and would thus otherwise +/// starve the client's IOService and cause a backlog of ready event handlers. +/// /// All errors are reported to the caller via the callback function supplied /// to the @ref HttpClient::asyncSendRequest. The IO errors are communicated /// via the @c boost::system::error code value. The response parsing errors @@ -86,9 +92,9 @@ public: /// /// Returned boolean value indicates whether the client should continue /// connecting to the server (if true) or not (false). - /// It is passed the IO error code along with the native socket handle of - /// the connection's TCP socket. This always the socket's event readiness - /// to be monitored via select() or epoll. + /// It is passed the IO error code along with the native socket handle of + /// the connection's TCP socket. The passed socket descriptor may be used + /// to monitor the readiness of the events via select() or epoll(). /// /// @note Beware that the IO error code can be set to "in progress" /// so a not null error code does not always mean the connect failed.