[#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 392826ac70404ceb0f5a34ffc7e86064aeb5de65..52fc6d53b5ef1e838a66354d9805ca6f996f21c1 100644 (file)
--- 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 <cstddef>
 #include <log/message_types.h>
@@ -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 631290b1b37936f86e86cccdb0bf8807af9e3040..14337ab4bbc807ff50535825b21a964121fdfd13 100644 (file)
--- 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 85ba97bea46eae692f8c9655512d6bc40b092a16..83a9231378bd3dbc510fbb0af61890f6195910b8 100644 (file)
--- 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 3a9f74513fbe726a9036baa683ddb1741d30a56e..7dafa7f17fcb902f099d1213217f17c914203f3c 100644 (file)
--- 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<n> 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 c930e782b52373172b0178f42fd5312212daef5c..a45236fb7e1af227c9b59473a9a4504a96107912 100644 (file)
--- 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 79a3cb346af9cd85a5f3da5b475fdbfca791b43b..e603f6f650971d85b5ca637c6c8c0d03163e4043 100644 (file)
--- 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 <cstddef>
 #include <log/message_types.h>
@@ -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 658b0810885f74cdfbbaf68b0041e98565039285..ac00399bc0880a21f4c6baac6ab9d59b6f381a85 100644 (file)
--- 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 de00e539096cbfde7e6968dd0e6902f53d548a22..a6325e010af2522e5c8c4da45aabb7747312a540 100644 (file)
--- 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