[#2477] backport the fix for HA+MT CalloutHandle reset crash (#2743) to 2.0.3

diff --git a/src/bin/dhcp4/dhcp4_srv.cc b/src/bin/dhcp4/dhcp4_srv.cc
index 4d732a2c077062aa587de32f00a65733c542ba44..3d6f56edd8beeb7a13a0fd50363ddeb6f13bc788 100644 (file)
--- a/src/bin/dhcp4/dhcp4_srv.cc
+++ b/src/bin/dhcp4/dhcp4_srv.cc
@@ -1329,11 +1329,31 @@ Dhcpv4Srv::processDhcp4Query(Pkt4Ptr& query, Pkt4Ptr& rsp,
 
     CalloutHandlePtr callout_handle = getCalloutHandle(query);
     if (ctx && HooksManager::calloutsPresent(Hooks.hook_index_leases4_committed_)) {
+        // The ScopedCalloutHandleState class which guarantees that the task
+        // is added to the thread pool after the response is reset (if needed)
+        // and CalloutHandle state is reset. In ST it does nothing.
+        // A smart pointer is used to store the ScopedCalloutHandleState so that
+        // a copy of the pointer is created by the lambda and only on the
+        // destruction of the last reference the task is added.
+        // In MT there are 2 cases:
+        // 1. packet is unparked before current thread smart pointer to
+        //    ScopedCalloutHandleState is destroyed:
+        //  - the lamba uses the smart pointer to set the callout which adds the
+        //    task, but the task is added after ScopedCalloutHandleState is
+        //    destroyed, on the destruction of the last reference which is held
+        //    by the current thread.
+        // 2. packet is unparked after the current thread smart pointer to
+        //    ScopedCalloutHandleState is destroyed:
+        //  - the current thread reference to ScopedCalloutHandleState is
+        //    destroyed, but the reference in the lambda keeps it alive until
+        //    the lamba is called and the last reference is released, at which
+        //    time the task is actually added.
         // Use the RAII wrapper to make sure that the callout handle state is
         // reset when this object goes out of scope. All hook points must do
         // it to prevent possible circular dependency between the callout
         // handle and its arguments.
-        ScopedCalloutHandleState callout_handle_state(callout_handle);
+        std::shared_ptr<ScopedCalloutHandleState> callout_handle_state =
+                std::make_shared<ScopedCalloutHandleState>(callout_handle);
 
         ScopedEnableOptionsCopy<Pkt4> query4_options_copy(query);
 
@@ -1386,13 +1406,15 @@ Dhcpv4Srv::processDhcp4Query(Pkt4Ptr& query, Pkt4Ptr& rsp,
             // NEXT_STEP_PARK.  Otherwise the callback we bind here will be
             // executed when the hook library unparks the packet.
             HooksManager::park("leases4_committed", query,
-            [this, callout_handle, query, rsp]() mutable {
+            [this, callout_handle, query, rsp, callout_handle_state]() mutable {
                 if (MultiThreadingMgr::instance().getMode()) {
                     typedef function<void()> CallBack;
                     boost::shared_ptr<CallBack> call_back =
                         boost::make_shared<CallBack>(std::bind(&Dhcpv4Srv::sendResponseNoThrow,
                                                                this, callout_handle, query, rsp));
-                    MultiThreadingMgr::instance().getThreadPool().add(call_back);
+                    callout_handle_state->on_completion_ = [call_back]() {
+                        MultiThreadingMgr::instance().getThreadPool().add(call_back);
+                    };
                 } else {
                     processPacketPktSend(callout_handle, query, rsp);
                     processPacketBufferSend(callout_handle, rsp);

diff --git a/src/bin/dhcp6/dhcp6_srv.cc b/src/bin/dhcp6/dhcp6_srv.cc
index b0c16d346cd29d2608c3f865fedcc64eac4549f8..584691d13b30558fed73934c2b6e59ea566f406a 100644 (file)
--- a/src/bin/dhcp6/dhcp6_srv.cc
+++ b/src/bin/dhcp6/dhcp6_srv.cc
@@ -978,12 +978,31 @@ Dhcpv6Srv::processDhcp6Query(Pkt6Ptr& query, Pkt6Ptr& rsp) {
     if (!ctx.fake_allocation_ && (ctx.query_->getType() != DHCPV6_CONFIRM) &&
         (ctx.query_->getType() != DHCPV6_INFORMATION_REQUEST) &&
         HooksManager::calloutsPresent(Hooks.hook_index_leases6_committed_)) {
-
+        // The ScopedCalloutHandleState class which guarantees that the task
+        // is added to the thread pool after the response is reset (if needed)
+        // and CalloutHandle state is reset. In ST it does nothing.
+        // A smart pointer is used to store the ScopedCalloutHandleState so that
+        // a copy of the pointer is created by the lambda and only on the
+        // destruction of the last reference the task is added.
+        // In MT there are 2 cases:
+        // 1. packet is unparked before current thread smart pointer to
+        //    ScopedCalloutHandleState is destroyed:
+        //  - the lamba uses the smart pointer to set the callout which adds the
+        //    task, but the task is added after ScopedCalloutHandleState is
+        //    destroyed, on the destruction of the last reference which is held
+        //    by the current thread.
+        // 2. packet is unparked after the current thread smart pointer to
+        //    ScopedCalloutHandleState is destroyed:
+        //  - the current thread reference to ScopedCalloutHandleState is
+        //    destroyed, but the reference in the lambda keeps it alive until
+        //    the lamba is called and the last reference is released, at which
+        //    time the task is actually added.
         // Use the RAII wrapper to make sure that the callout handle state is
         // reset when this object goes out of scope. All hook points must do
         // it to prevent possible circular dependency between the callout
         // handle and its arguments.
-        ScopedCalloutHandleState callout_handle_state(callout_handle);
+        std::shared_ptr<ScopedCalloutHandleState> callout_handle_state =
+                std::make_shared<ScopedCalloutHandleState>(callout_handle);
 
         ScopedEnableOptionsCopy<Pkt6> query6_options_copy(query);
 
@@ -1056,13 +1075,15 @@ Dhcpv6Srv::processDhcp6Query(Pkt6Ptr& query, Pkt6Ptr& rsp) {
         // NEXT_STEP_PARK.  Otherwise the callback we bind here will be
         // executed when the hook library unparks the packet.
         HooksManager::park("leases6_committed", query,
-        [this, callout_handle, query, rsp]() mutable {
+        [this, callout_handle, query, rsp, callout_handle_state]() mutable {
             if (MultiThreadingMgr::instance().getMode()) {
                 typedef function<void()> CallBack;
                 boost::shared_ptr<CallBack> call_back =
                     boost::make_shared<CallBack>(std::bind(&Dhcpv6Srv::sendResponseNoThrow,
                                                            this, callout_handle, query, rsp));
-                MultiThreadingMgr::instance().getThreadPool().add(call_back);
+                callout_handle_state->on_completion_ = [call_back]() {
+                    MultiThreadingMgr::instance().getThreadPool().add(call_back);
+                };
             } else {
                 processPacketPktSend(callout_handle, query, rsp);
                 processPacketBufferSend(callout_handle, rsp);

diff --git a/src/lib/hooks/callout_handle.cc b/src/lib/hooks/callout_handle.cc
index e80760ac922aa9420aac4cc520897a7528e5601c..89a235a8fca83a4fd79ba3a70bc2ae627e3e3cd7 100644 (file)
--- a/src/lib/hooks/callout_handle.cc
+++ b/src/lib/hooks/callout_handle.cc
@@ -149,6 +149,10 @@ ScopedCalloutHandleState(const CalloutHandlePtr& callout_handle)
 
 ScopedCalloutHandleState::~ScopedCalloutHandleState() {
     resetState();
+
+    if (on_completion_) {
+        on_completion_();
+    }
 }
 
 void

diff --git a/src/lib/hooks/callout_handle.h b/src/lib/hooks/callout_handle.h
index b9e018b7c026ed34c364d1d3b0789ac533bd93a5..a8be978cfe78e7f58db6a81d02d8cfb930e0b137 100644 (file)
--- a/src/lib/hooks/callout_handle.h
+++ b/src/lib/hooks/callout_handle.h
@@ -490,6 +490,9 @@ public:
     /// Resets state of the callout handle.
     ~ScopedCalloutHandleState();
 
+    /// @brief Continuation callback.
+    std::function<void()> on_completion_;
+
 private:
 
     /// @brief Resets the callout handle state.