From: Thomas Markwalder Date: Wed, 26 Jun 2019 14:23:24 +0000 (-0400) Subject: [#691,!395] Review comments 2 X-Git-Tag: Kea-1.6.0-beta2~196 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=cb11b477da1d2d3a8796f843e2ac2878921774c9;p=thirdparty%2Fkea.git [#691,!395] Review comments 2 src/hooks/dhcp/high_availability/ha_messages.mes HA_SERVICE_CONNECT_INVALID_SOCKET - new message src/hooks/dhcp/high_availability/ha_service.cc HAService::clientConnectHandler() - added negative fd logic src/lib/http/client.cc Connection::closeCallback() - new method that wraps invocation of close callback in try-catch. src/lib/http/http_messages.mes HTTP_CONNECTION_CLOSE_CALLBACK_FAILED - new message --- diff --git a/src/hooks/dhcp/high_availability/ha_messages.cc b/src/hooks/dhcp/high_availability/ha_messages.cc index 392826ac70..52fc6d53b5 100644 --- a/src/hooks/dhcp/high_availability/ha_messages.cc +++ b/src/hooks/dhcp/high_availability/ha_messages.cc @@ -1,4 +1,4 @@ -// File created from ../../../../src/hooks/dhcp/high_availability/ha_messages.mes on Mon Jun 24 2019 17:05 +// File created from ../../../../src/hooks/dhcp/high_availability/ha_messages.mes on Wed Jun 26 2019 09:25 #include #include @@ -60,6 +60,7 @@ extern const isc::log::MessageID HA_LOCAL_DHCP_DISABLE = "HA_LOCAL_DHCP_DISABLE" extern const isc::log::MessageID HA_LOCAL_DHCP_ENABLE = "HA_LOCAL_DHCP_ENABLE"; extern const isc::log::MessageID HA_MISSING_CONFIGURATION = "HA_MISSING_CONFIGURATION"; extern const isc::log::MessageID HA_SCOPES_HANDLER_FAILED = "HA_SCOPES_HANDLER_FAILED"; +extern const isc::log::MessageID HA_SERVICE_CONNECT_INVALID_SOCKET = "HA_SERVICE_CONNECT_INVALID_SOCKET"; extern const isc::log::MessageID HA_SERVICE_STARTED = "HA_SERVICE_STARTED"; extern const isc::log::MessageID HA_STATE_MACHINE_CONTINUED = "HA_STATE_MACHINE_CONTINUED"; extern const isc::log::MessageID HA_STATE_MACHINE_PAUSED = "HA_STATE_MACHINE_PAUSED"; @@ -129,6 +130,7 @@ const char* values[] = { "HA_LOCAL_DHCP_ENABLE", "local DHCP service is enabled while the %1 is in the %2 state", "HA_MISSING_CONFIGURATION", "high-availability parameter not specified for High Availability hooks library", "HA_SCOPES_HANDLER_FAILED", "ha-scopes command failed: %1", + "HA_SERVICE_CONNECT_INVALID_SOCKET", "Attempted to register an invalid socket, error code: %1", "HA_SERVICE_STARTED", "started high availability service in %1 mode as %2 server", "HA_STATE_MACHINE_CONTINUED", "state machine is un-paused", "HA_STATE_MACHINE_PAUSED", "state machine paused in state %1", diff --git a/src/hooks/dhcp/high_availability/ha_messages.h b/src/hooks/dhcp/high_availability/ha_messages.h index 631290b1b3..14337ab4bb 100644 --- a/src/hooks/dhcp/high_availability/ha_messages.h +++ b/src/hooks/dhcp/high_availability/ha_messages.h @@ -1,4 +1,4 @@ -// File created from ../../../../src/hooks/dhcp/high_availability/ha_messages.mes on Mon Jun 24 2019 17:05 +// File created from ../../../../src/hooks/dhcp/high_availability/ha_messages.mes on Wed Jun 26 2019 09:25 #ifndef HA_MESSAGES_H #define HA_MESSAGES_H @@ -61,6 +61,7 @@ extern const isc::log::MessageID HA_LOCAL_DHCP_DISABLE; extern const isc::log::MessageID HA_LOCAL_DHCP_ENABLE; extern const isc::log::MessageID HA_MISSING_CONFIGURATION; extern const isc::log::MessageID HA_SCOPES_HANDLER_FAILED; +extern const isc::log::MessageID HA_SERVICE_CONNECT_INVALID_SOCKET; extern const isc::log::MessageID HA_SERVICE_STARTED; extern const isc::log::MessageID HA_STATE_MACHINE_CONTINUED; extern const isc::log::MessageID HA_STATE_MACHINE_PAUSED; diff --git a/src/hooks/dhcp/high_availability/ha_messages.mes b/src/hooks/dhcp/high_availability/ha_messages.mes index 85ba97bea4..83a9231378 100644 --- a/src/hooks/dhcp/high_availability/ha_messages.mes +++ b/src/hooks/dhcp/high_availability/ha_messages.mes @@ -116,6 +116,11 @@ failure. This informational message indicates that the High Availability hooks library has been unloaded successfully. +% HA_SERVICE_CONNECT_INVALID_SOCKET Attempted to register an invalid socket, error code: %1 +This is an error message that indicates that the server attempted to +register an invalid socket with the Interface Manager. This is an internal +server error and a bug report should be created. + % HA_DHCP4_START_SERVICE_FAILED failed to start DHCPv4 HA service in dhcp4_srv_configured callout: %1 This error message is issued when an attempt to start High Availability service for the DHCPv4 server failed in the dhcp4_srv_configured callout. This diff --git a/src/hooks/dhcp/high_availability/ha_service.cc b/src/hooks/dhcp/high_availability/ha_service.cc index 3a9f74513f..7dafa7f17f 100644 --- a/src/hooks/dhcp/high_availability/ha_service.cc +++ b/src/hooks/dhcp/high_availability/ha_service.cc @@ -1621,7 +1621,19 @@ 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) { + if (!ec || (ec.value() == boost::asio::error::in_progress)) { + if (ec && (ec.value() == boost::asio::error::in_progress)) { + std::cout << "connect in_progress : " << tcp_native_fd << std::endl; + } + + if (tcp_native_fd < 0) { + // This really should not be possible, but just in case. + LOG_ERROR(ha_logger, HA_SERVICE_CONNECT_INVALID_SOCKET) + .arg(ec.value()); + return (false); + } + + // Register the socket with Interface Manager. // 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(). @@ -1629,7 +1641,7 @@ HAService::clientConnectHandler(const boost::system::error_code& ec, int tcp_nat } // If ec.value() == boost::asio::error::already_connected, we should already - // be registered, so nothing to do. If it is any other value, than connect + // be registered, so nothing to do. If it is any other value, then connect // failed and Connection logic should handle that, not us, so no matter // what happens we're returning true. return (true); diff --git a/src/lib/http/client.cc b/src/lib/http/client.cc index c930e782b5..a45236fb7e 100644 --- a/src/lib/http/client.cc +++ b/src/lib/http/client.cc @@ -233,6 +233,17 @@ private: /// @brief Local callback invoked when request timeout occurs. void timerCallback(); + /// @brief Local callback invoked when the connection is closed. + /// + /// Invokes the close callback (if one), passing in the socket's + /// descriptor, when the connection's socket about to be closed. + /// The callback invocation is wrapped in a try-catch to ensure + /// exception safety. + /// + /// @param clear dictates whether or not the callback is discarded + /// after invocation. Defaults to false. + void closeCallback(const bool clear = false); + /// @brief Pointer to the connection pool owning this connection. /// /// This is a weak pointer to avoid circular dependency between the @@ -502,6 +513,23 @@ Connection::resetState() { current_callback_ = HttpClient::RequestHandler(); } + +void +Connection::closeCallback(const bool clear) { + if (close_callback_) { + try { + close_callback_(socket_.getNative()); + } catch (...) { + LOG_ERROR(http_logger, HTTP_CONNECTION_CLOSE_CALLBACK_FAILED); + } + } + + if (clear) { + close_callback_ = HttpClient::CloseHandler(); + } +} + + void Connection::doTransaction(const HttpRequestPtr& request, const HttpResponsePtr& response, @@ -529,9 +557,7 @@ Connection::doTransaction(const HttpRequestPtr& request, // data over this socket, when the peer may close the connection. In this // case we'll need to re-transmit but we don't handle it here. if (socket_.getASIOSocket().is_open() && !socket_.isUsable()) { - if (close_callback_) { - close_callback_(socket_.getNative()); - } + closeCallback(); socket_.close(); } @@ -568,11 +594,8 @@ Connection::doTransaction(const HttpRequestPtr& request, void Connection::close() { - if (close_callback_) { - close_callback_(socket_.getNative()); - close_callback_ = HttpClient::CloseHandler(); - } - + closeCallback(true); + timer_.cancel(); socket_.close(); resetState(); @@ -667,7 +690,7 @@ Connection::terminate(const boost::system::error_code& ec, ConnectionPoolPtr conn_pool = conn_pool_.lock(); if (conn_pool && conn_pool->getNextRequest(url_, request, response, request_timeout, callback, connect_callback, close_callback)) { - doTransaction(request, response, request_timeout, callback, + doTransaction(request, response, request_timeout, callback, connect_callback, close_callback); } } diff --git a/src/lib/http/http_messages.cc b/src/lib/http/http_messages.cc index 79a3cb346a..e603f6f650 100644 --- a/src/lib/http/http_messages.cc +++ b/src/lib/http/http_messages.cc @@ -1,4 +1,4 @@ -// File created from ../../../src/lib/http/http_messages.mes on Mon May 13 2019 17:08 +// File created from ../../../src/lib/http/http_messages.mes on Wed Jun 26 2019 10:16 #include #include @@ -16,6 +16,7 @@ extern const isc::log::MessageID HTTP_CLIENT_REQUEST_RECEIVED_DETAILS = "HTTP_CL extern const isc::log::MessageID HTTP_CLIENT_REQUEST_SEND = "HTTP_CLIENT_REQUEST_SEND"; extern const isc::log::MessageID HTTP_CLIENT_REQUEST_SEND_DETAILS = "HTTP_CLIENT_REQUEST_SEND_DETAILS"; extern const isc::log::MessageID HTTP_CLIENT_REQUEST_TIMEOUT_OCCURRED = "HTTP_CLIENT_REQUEST_TIMEOUT_OCCURRED"; +extern const isc::log::MessageID HTTP_CONNECTION_CLOSE_CALLBACK_FAILED = "HTTP_CONNECTION_CLOSE_CALLBACK_FAILED"; extern const isc::log::MessageID HTTP_CONNECTION_STOP = "HTTP_CONNECTION_STOP"; extern const isc::log::MessageID HTTP_CONNECTION_STOP_FAILED = "HTTP_CONNECTION_STOP_FAILED"; extern const isc::log::MessageID HTTP_DATA_RECEIVED = "HTTP_DATA_RECEIVED"; @@ -42,6 +43,7 @@ const char* values[] = { "HTTP_CLIENT_REQUEST_SEND", "sending HTTP request %1 to %2", "HTTP_CLIENT_REQUEST_SEND_DETAILS", "detailed information about request sent to %1:\n%2", "HTTP_CLIENT_REQUEST_TIMEOUT_OCCURRED", "HTTP request timeout occurred when communicating with %1", + "HTTP_CONNECTION_CLOSE_CALLBACK_FAILED", "Connection close callback threw an exception", "HTTP_CONNECTION_STOP", "stopping HTTP connection from %1", "HTTP_CONNECTION_STOP_FAILED", "stopping HTTP connection failed", "HTTP_DATA_RECEIVED", "received %1 bytes from %2", diff --git a/src/lib/http/http_messages.h b/src/lib/http/http_messages.h index 658b081088..ac00399bc0 100644 --- a/src/lib/http/http_messages.h +++ b/src/lib/http/http_messages.h @@ -1,4 +1,4 @@ -// File created from ../../../src/lib/http/http_messages.mes on Mon May 13 2019 17:08 +// File created from ../../../src/lib/http/http_messages.mes on Wed Jun 26 2019 10:16 #ifndef HTTP_MESSAGES_H #define HTTP_MESSAGES_H @@ -17,6 +17,7 @@ extern const isc::log::MessageID HTTP_CLIENT_REQUEST_RECEIVED_DETAILS; extern const isc::log::MessageID HTTP_CLIENT_REQUEST_SEND; extern const isc::log::MessageID HTTP_CLIENT_REQUEST_SEND_DETAILS; extern const isc::log::MessageID HTTP_CLIENT_REQUEST_TIMEOUT_OCCURRED; +extern const isc::log::MessageID HTTP_CONNECTION_CLOSE_CALLBACK_FAILED; extern const isc::log::MessageID HTTP_CONNECTION_STOP; extern const isc::log::MessageID HTTP_CONNECTION_STOP_FAILED; extern const isc::log::MessageID HTTP_DATA_RECEIVED; diff --git a/src/lib/http/http_messages.mes b/src/lib/http/http_messages.mes index de00e53909..a6325e010a 100644 --- a/src/lib/http/http_messages.mes +++ b/src/lib/http/http_messages.mes @@ -63,6 +63,11 @@ This debug message is issued when the HTTP request timeout has occurred and the server is going to send a response with Http Request timeout status code. +% HTTP_CONNECTION_CLOSE_CALLBACK_FAILED Connection close callback threw an exception +This is an error message emitted when the close connection callback +registered on the connection failed unexpectedly. This is a programmatic +error that should be submitted as a bug. + % HTTP_CONNECTION_STOP stopping HTTP connection from %1 This debug message is issued when one of the HTTP connections is stopped. The connection can be stopped as a result of an error or after the