[#964,!577] Revamped to detect and close OOB Connections

Rather than just unregistering the socket, we now actually close
the Connection. This ensures we never end up with an unregistered
but open connection.

src/hooks/dhcp/high_availability/ha_service.*
    HAService::clientConnectHandler() - modified to call
    HttpClient::closeIfOutOfBandwidth().

src/lib/http/client.*
    Connection - replaced isTransaction(int socket_fd) with
    isMySocket(int socket_fd)

    ConnectionPool - replaced isTransaction(int socket_fd) with
    closeIfOutOfBandwidth(int socket_fd)

    HttpClient - replaced isTransaction(int socket_fd) with
    closeIfOutOfBandwidth(int socket_fd)

diff --git a/src/hooks/dhcp/high_availability/ha_service.cc b/src/hooks/dhcp/high_availability/ha_service.cc
index ec78a5d74d0f59c199335e0f6ff8f8744d65bfe9..77106bef18e7ac78abaf3233133a14677323c7f7 100644 (file)
--- a/src/hooks/dhcp/high_availability/ha_service.cc
+++ b/src/hooks/dhcp/high_availability/ha_service.cc
@@ -1647,10 +1647,10 @@ HAService::clientConnectHandler(const boost::system::error_code& ec, int tcp_nat
 void
 HAService::socketReadyHandler(int tcp_native_fd) {
     // 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);
-    }
+    // ongoing transactions, we close it.  This will unregister it from
+    // IfaceMgr and ensure the client starts over with a fresh connection
+    // if it needs to do so.
+    client_.closeIfOutOfBandwidth(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 d364fb1ddd534914fc75e357993a755c2ca2b027..68129f574419f1b6f268deb48a1009717f38876e 100644 (file)
--- a/src/hooks/dhcp/high_availability/ha_service.h
+++ b/src/hooks/dhcp/high_availability/ha_service.h
@@ -744,16 +744,14 @@ protected:
     /// flagged as ready to read.   It is installed by the invocation to
     /// register the socket with IfaceMgr made in @ref clientConnectHandler.
     ///
-    /// 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.
+    /// The handler calls @ref HttpClient::closeIfOutOfBandwidth() to catch
+    /// and close any sockets that have gone ready outside of transactions.
     ///
     /// 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
+    /// the IfaceMgr::select(). If this happens 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.
+    /// This causes IfaceMgr::select() to endlessly interrupt on the 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 e056170efd97a111ba62722bcb996ce89c416f1f..2593359ed7cfeb4107c9e03d3b5e1f667b12443c 100644 (file)
--- a/src/lib/http/client.cc
+++ b/src/lib/http/client.cc
@@ -139,11 +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.
+    /// @brief Checks if a socket descriptor belongs to this connection.
     ///
-    /// @return true if transaction has been initiated, false otherwise.
-    bool isTransactionOngoing(int socket_fd) const;
+    /// @param socket_fd socket descriptor to check
+    ///
+    /// @return True if the socket fd belongs to this connection.
+    bool isMySocket(int socket_fd) const;
 
     /// @brief Checks and logs if premature transaction timeout is suspected.
     ///
@@ -445,21 +446,41 @@ public:
         queue_.clear();
     }
 
-    /// @brief Deteremines if there is an ongoing transaction associated
-    /// with a given socket desecriptor.
+    /// @brief Closes a connection if it has an out-of-bandwidth socket event
     ///
-    /// @param socket_fd socket descriptor to check
+    /// If the pool contains a connection using the given socket and that
+    /// connection is currently in a transaction the method returns as this
+    /// indicates a normal ready event.  If the connection is not in an
+    /// ongoing transaction, then the connection is closed.
     ///
-    /// @return True if the pool contains a connection which is using the
-    /// given socket descriptor for an ongoing transaction.
-    bool isTransactionOngoing(int socket_fd) {
+    /// This is method is intended to be used to detect and clean up then
+    /// sockets that are marked ready outside of transactions. The most comman
+    /// case is the other end of the socket being closed.
+    ///
+    /// @param socket_fd socket descriptor to check
+    void closeIfOutOfBandwidth(int socket_fd) {
+        // First we look for a connection with the socket.
         for (auto conns_it = conns_.begin(); conns_it != conns_.end();
              ++conns_it) {
-            if (conns_it->second->isTransactionOngoing(socket_fd)) {
-                return (true);
+
+            if (!conns_it->second->isMySocket(socket_fd)) {
+                // Not this connection.
+                continue;
             }
+
+            if (conns_it->second->isTransactionOngoing()) {
+                // Matches but is in a transaction, all is well.
+                return;
+            }
+
+            // Socket has no transaction, so any ready event is
+            // out-of-bandwidth (other end probably closed), so
+            // let's close it.  Note we do not remove any queued
+            // requests, as this might somehow be occurring in
+            // between them.
+            conns_it->second->close();
+            conns_.erase(conns_it);
         }
-        return (false);
     }
 
 private:
@@ -631,9 +652,8 @@ Connection::isTransactionOngoing() const {
 }
 
 bool
-Connection::isTransactionOngoing(int socket_fd) const {
-    return ((static_cast<bool>(current_request_)) &&
-            (socket_.getNative() == socket_fd));
+Connection::isMySocket(int socket_fd) const {
+    return (socket_.getNative() == socket_fd);
 }
 
 bool
@@ -952,9 +972,9 @@ 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::closeIfOutOfBandwidth(int socket_fd)  {
+    return (impl_->conn_pool_->closeIfOutOfBandwidth(socket_fd));
 }
 
 void

diff --git a/src/lib/http/client.h b/src/lib/http/client.h
index 95f1af977984515bca9cbf6f76ed50603281455e..9b9aff751f200884edca45c41f4f2ed1122f96b0 100644 (file)
--- a/src/lib/http/client.h
+++ b/src/lib/http/client.h
@@ -191,11 +191,19 @@ public:
     /// @brief Closes all connections.
     void stop();
 
-    /// @brief Deteremines if a socket is part of an ongoing transaction.
+    /// @brief Closes a connection if it has an out-of-bandwidth socket event
     ///
-    /// @return True if the given socket descriptor is being used by an
-    /// ongoing transaction
-    bool isTransactionOngoing(int socket_fd) const;
+    /// If the  client owns a connection using the given socket and that
+    /// connection is currently in a transaction the method returns as this
+    /// indicates a normal ready event.  If the connection is not in an
+    /// ongoing transaction, then the connection is closed.
+    ///
+    /// This is method is intended to be used to detect and clean up then
+    /// sockets that are marked ready outside of transactions. The most comman
+    /// case is the other end of the socket being closed.
+    ///
+    /// @param socket_fd socket descriptor to check
+    void closeIfOutOfBandwidth(int socket_fd);
 
 private: