[#4140] ping-check: fix potential deadlock between PingCheckMgr and P…

src/hooks/dhcp/ping_check/ping_channel.cc

@@ -36,11 +36,13 @@ PingChannel::nextEchoInstanceNum() {
 
 PingChannel::PingChannel(IOServicePtr& io_service,
                          NextToSendCallback next_to_send_cb,
+                         UpdateToSendCallback update_to_send_cb,
                          EchoSentCallback echo_sent_cb,
                          ReplyReceivedCallback reply_received_cb,
                          ShutdownCallback shutdown_cb)
     : io_service_(io_service),
       next_to_send_cb_(next_to_send_cb),
+      update_to_send_cb_(update_to_send_cb),
       echo_sent_cb_(echo_sent_cb),
       reply_received_cb_(reply_received_cb),
       shutdown_cb_(shutdown_cb),
@@ -310,19 +312,25 @@ PingChannel::startRead() {
 void
 PingChannel::sendNext() {
     try {
+        // Fetch the next one to send (outside the mutex).
+        IOAddress target("0.0.0.0");
+        PingContextPtr context = ((next_to_send_cb_)(target));
+        if (!context) {
+            // Nothing to send.
+            return;
+        }
+
         MultiThreadingLock lock(*mutex_);
         if (!canSend()) {
             // Can't send right now, get out.
             return;
         }
-
-        // Fetch the next one to send.
-        IOAddress target("0.0.0.0");
-        if (!((next_to_send_cb_)(target))) {
-            // Nothing to send.
-            return;
+
+        // Update context to SENDING inside the mutex.
+        if (update_to_send_cb_) {
+            (update_to_send_cb_)(context);
         }
-
+        
         // Have an target IP, build an ECHO REQUEST for it.
         sending_ = true;
         ICMPMsgPtr next_echo(new ICMPMsg());

src/hooks/dhcp/ping_check/ping_channel.h

@@ -13,6 +13,7 @@
 #include <util/watch_socket.h>
 #include <icmp_msg.h>
 #include <icmp_socket.h>
+#include <ping_context.h>
 
 #include <boost/scoped_ptr.hpp>
 #include <boost/enable_shared_from_this.hpp>
@@ -66,8 +67,12 @@ typedef ICMPSocket<SocketCallback> PingSocket;
 /// @brief Defines a pointer to PingSocket.
 typedef boost::shared_ptr<PingSocket> PingSocketPtr;
 
-/// @brief Function type for callback that fetches next IOAddress to ping.
-typedef std::function<bool(asiolink::IOAddress& target)> NextToSendCallback;
+/// @brief Function type for callback that fetches next target to ping.
+/// Returns the context selected to send, and outputs its target address.
+typedef std::function<PingContextPtr(asiolink::IOAddress& target)> NextToSendCallback;
+
+/// @brief Function type for callback to update a context to SENDING state.
+typedef std::function<void(PingContextPtr context)> UpdateToSendCallback;
 
 /// @brief Function type for callback to invoke upon ECHO send completion.
 typedef std::function<void(ICMPMsgPtr& echo, bool send_failed)> EchoSentCallback;
@@ -108,8 +113,10 @@ class PingChannel : public boost::enable_shared_from_this<PingChannel> {
     ///
     /// @param io_service pointer to the IOService instance that will manage
     /// the channel's IO. Must not be empty
-    /// @param next_to_send_cb callback to invoke to fetch the next IOAddress
-    /// to ping
+    /// @param next_to_send_cb callback to invoke to fetch the next context and
+    /// its target address to ping (called outside the mutex)
+    /// @param update_to_send_cb callback to invoke to update the selected
+    /// context to SENDING and persist it (called inside the mutex)
     /// @param echo_sent_cb callback to invoke when an ECHO send has completed
     /// @param reply_received_cb callback to invoke when an ICMP reply has been
     /// received.  This callback is passed all inbound ICMP messages (e.g. ECHO
@@ -120,6 +127,7 @@ class PingChannel : public boost::enable_shared_from_this<PingChannel> {
     /// @throw BadValue if io_service is empty.
     PingChannel(asiolink::IOServicePtr& io_service,
                 NextToSendCallback next_to_send_cb,
+                UpdateToSendCallback update_to_send_cb,
                 EchoSentCallback echo_sent_cb,
                 ReplyReceivedCallback reply_received_cb,
                 ShutdownCallback shutdown_cb = ShutdownCallback());
@@ -315,9 +323,12 @@ class PingChannel : public boost::enable_shared_from_this<PingChannel> {
     /// @brief IOService instance the drives socket IO
     asiolink::IOServicePtr io_service_;
 
-    /// @brief Callback to invoke to fetch the next address to ping.
+    /// @brief Callback to invoke to fetch the next context/address to ping.
     NextToSendCallback next_to_send_cb_;
 
+    /// @brief Callback to invoke to update selected context to SENDING state.
+    UpdateToSendCallback update_to_send_cb_;
+
     /// @brief Callback to invoke when an ECHO write has completed.
     EchoSentCallback echo_sent_cb_;
 

src/hooks/dhcp/ping_check/ping_check_mgr.cc

@@ -178,23 +178,26 @@ PingCheckMgr::startPing(dhcp::Lease4Ptr& lease, dhcp::Pkt4Ptr& query, hooks::Par
     startPing(lease, query, parking_lot, getGlobalConfig());
 }
 
-bool
+PingContextPtr
 PingCheckMgr::nextToSend(IOAddress& next) {
     if (checkSuspended()) {
-        return (false);
+        return (PingContextPtr());
     }
 
     PingContextPtr context = store_->getNextToSend();
     if (!context) {
-        return (false);
+        return (PingContextPtr());
     }
 
     next = context->getTarget();
+    return (context);
+}
+
+void
+PingCheckMgr::updateContextToSend(PingContextPtr context) {
     // Transition to sending.
     context->setState(PingContext::SENDING);
     store_->updateContext(context);
-
-    return (true);
 }
 
 void
@@ -632,6 +635,8 @@ PingCheckMgr::createChannel(IOServicePtr io_service) {
     return (PingChannelPtr(new PingChannel(io_service,
                                            std::bind(&PingCheckMgr::nextToSend,
                                                      this, ph::_1),
+                                           std::bind(&PingCheckMgr::updateContextToSend,
+                                                     this, ph::_1),
                                            std::bind(&PingCheckMgr::sendCompleted,
                                                      this, ph::_1, ph::_2),
                                            std::bind(&PingCheckMgr::replyReceived,

src/hooks/dhcp/ping_check/ping_check_mgr.h

@@ -118,16 +118,24 @@ class PingCheckMgr : public boost::enable_shared_from_this<PingCheckMgr> {
                    hooks::ParkingLotHandlePtr& parking_lot);
 
     /// @brief Callback passed to PingChannel to use to retrieve the next
-    /// address to check.
+    /// context/address to check.
     ///
     /// Fetches the context which has been in the WAITING_TO_SEND state the
-    /// longest and returns its lease address.
+    /// longest and returns it, also outputs its lease address.
     ///
-    /// @param[out] next upon return it will contain the next target address.
-    /// Contents are only meaningful if the function returns true.
+    /// @param[out] next upon return it will contain the next target address
+    /// if a context is available.
     ///
-    /// @return True another target address exists, false otherwise.
-    virtual bool nextToSend(asiolink::IOAddress& next);
+    /// @return The context selected to send, or empty if none available.
+    virtual PingContextPtr nextToSend(asiolink::IOAddress& next);
+
+    /// @brief Callback passed to PingChannel to update a context to SENDING
+    /// just before sending.
+    ///
+    /// This is invoked inside the channel mutex.
+    ///
+    /// @param context context to update.
+    virtual void updateContextToSend(PingContextPtr context);
 
     /// @brief Callback passed to PingChannel to invoke when an ECHO REQUEST
     /// send has completed.