From: Thomas Markwalder <tmark@isc.org>
Date: Fri, 25 Oct 2019 19:49:34 +0000 (-0400)
Subject: [#964,!577] Added external socket ready handler to HAService
X-Git-Tag: Kea-1.7.2~83
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=16f37ca0de9dd20057418698c1e3dbb7e67afe87;p=thirdparty%2Fkea.git

[#964,!577] Added external socket ready handler to HAService

src/hooks/dhcp/high_availability/ha_service.*
    HAService::socketReadyHandler(int tcp_native_fd) - new handler for external socket ready
    callback. It detects out-of-transaction ready socket and unregisters it.

src/lib/http/client.*
    isTransactionOngoing(int socket_fd) - new method to return true if any of the clients
    connections are using the socket in an ongoing transaction
---

diff --git a/src/hooks/dhcp/high_availability/ha_service.cc b/src/hooks/dhcp/high_availability/ha_service.cc
index 0cb08bf658..ec78a5d74d 100644
--- a/src/hooks/dhcp/high_availability/ha_service.cc
+++ b/src/hooks/dhcp/high_availability/ha_service.cc
@@ -1646,12 +1646,11 @@ HAService::clientConnectHandler(const boost::system::error_code& ec, int tcp_nat
 
 void
 HAService::socketReadyHandler(int tcp_native_fd) {
-    std::cout << "HAService::socketReadyHandler - ready socket:" << tcp_native_fd << std::endl;
-    // If the socket is not usable/or in a transaction
-    // we'll unregister it
-    // if (socket bad) /{
-    //      IfaceMgr::instance().deleteExternalSocket(tcp_native_fd, 0);
-    // }
+    // 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);
+    }
 }
 
 void
diff --git a/src/hooks/dhcp/high_availability/ha_service.h b/src/hooks/dhcp/high_availability/ha_service.h
index 48efda89fe..d364fb1ddd 100644
--- a/src/hooks/dhcp/high_availability/ha_service.h
+++ b/src/hooks/dhcp/high_availability/ha_service.h
@@ -744,9 +744,16 @@ protected:
     /// flagged as ready to read.   It is installed by the invocation to
     /// register the socket with IfaceMgr made in @ref clientConnectHandler.
     ///
-    /// @todo add logic to determine if the
-    /// socket is involved in an ongoing transcation or not. If not, it
-    /// will be unregistered from IfaceMgr.
+    /// 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.
+    ///
+    /// 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
+    /// in progess, we won't have anything to deal with the socket event.
+    /// This causes IfaceMgr::select() to endlessly interrupt on this 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 50a581fcfb..e056170efd 100644
--- a/src/lib/http/client.cc
+++ b/src/lib/http/client.cc
@@ -139,6 +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.
+    ///
+    /// @return true if transaction has been initiated, false otherwise.
+    bool isTransactionOngoing(int socket_fd) const;
+
     /// @brief Checks and logs if premature transaction timeout is suspected.
     ///
     /// There are cases when the premature timeout occurs, e.g. as a result of
@@ -439,6 +445,23 @@ public:
         queue_.clear();
     }
 
+    /// @brief Deteremines if there is an ongoing transaction associated
+    /// with a given socket desecriptor.
+    ///
+    /// @param socket_fd socket descriptor to check
+    ///
+    /// @return True if the pool contains a connection which is using the
+    /// given socket descriptor for an ongoing transaction.
+    bool isTransactionOngoing(int socket_fd) {
+        for (auto conns_it = conns_.begin(); conns_it != conns_.end();
+             ++conns_it) {
+            if (conns_it->second->isTransactionOngoing(socket_fd)) {
+                return (true);
+            }
+        }
+        return (false);
+    }
+
 private:
 
     /// @brief Holds reference to the IO service.
@@ -607,6 +630,12 @@ Connection::isTransactionOngoing() const {
     return (static_cast<bool>(current_request_));
 }
 
+bool
+Connection::isTransactionOngoing(int socket_fd) const {
+    return ((static_cast<bool>(current_request_)) &&
+            (socket_.getNative() == socket_fd));
+}
+
 bool
 Connection::checkPrematureTimeout(const uint64_t transid) {
     // If there is no transaction but the handlers are invoked it means
@@ -923,6 +952,11 @@ 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::stop() {
     impl_->conn_pool_->closeAll();
diff --git a/src/lib/http/client.h b/src/lib/http/client.h
index b72605222a..95f1af9779 100644
--- a/src/lib/http/client.h
+++ b/src/lib/http/client.h
@@ -191,6 +191,12 @@ public:
     /// @brief Closes all connections.
     void stop();
 
+    /// @brief Deteremines if a socket is part of an ongoing transaction.
+    ///
+    /// @return True if the given socket descriptor is being used by an
+    /// ongoing transaction
+    bool isTransactionOngoing(int socket_fd) const;
+
 private:
 
     /// @brief Pointer to the HTTP client implementation.