[#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 fc5cb59e0efd11638507684bcd7ad883b2e25fb1..3a9f74513fbe726a9036baa683ddb1741d30a56e 100644 (file)
--- a/src/hooks/dhcp/high_availability/ha_service.cc
+++ b/src/hooks/dhcp/high_availability/ha_service.cc
@@ -12,6 +12,7 @@
 #include <ha_service_states.h>
 #include <cc/command_interpreter.h>
 #include <cc/data.h>
+#include <config/timeouts.h>
 #include <dhcp/iface_mgr.h>
 #include <dhcpsrv/lease_mgr.h>
 #include <dhcpsrv/lease_mgr_factory.h>
@@ -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<n> 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<n> 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 770b7add985753ca73d6a82943e00baa05230a28..b89a1055e421e8116f376927f61ddb5844ee73aa 100644 (file)
--- 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 d20de9a7284fdd1412eb37d9c8be16dc2fc05c38..56b6a1120094a00b6c01119a57311dd58c7835af 100644 (file)
--- 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 96eef3685e4b277dd2d084ebb0c5e3bea4842936..d66bf0fcbf1ba2870eb06ed52c64f34599e5451c 100644 (file)
--- a/src/lib/dhcp/iface_mgr.cc
+++ b/src/lib/dhcp/iface_mgr.cc
@@ -351,7 +351,7 @@ IfaceMgr::purgeBadSockets() {
     std::vector<int> 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 46ec5c86159e6af97a57fe04f47d5f803d293b88..90c64820af06cd5ecc235366c86364c77d4e8a36 100644 (file)
--- 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.