[#2043] allow callbacks to create CS inside the CS constructor and destructor

diff --git a/src/lib/util/multi_threading_mgr.cc b/src/lib/util/multi_threading_mgr.cc
index c6689667ccb78d9514ce4382236e28e526814664..7efd048bca96ecac1515f314e24ad1774a05daae 100644 (file)
--- a/src/lib/util/multi_threading_mgr.cc
+++ b/src/lib/util/multi_threading_mgr.cc
@@ -37,30 +37,45 @@ MultiThreadingMgr::setMode(bool enabled) {
 void
 MultiThreadingMgr::enterCriticalSection() {
     checkCallbacksPermissions();
-    std::lock_guard<std::mutex> lk(mutex_);
-    stopProcessing();
+    bool inside = isInCriticalSection();
+    // Increment the counter to allow CS to be created in the registered
+    // callbacks (in which case the new CS would not call callbacks again).
+    // The counter must be updated regardless of the MT mode because the MT mode
+    // can change between the constructor call and the destructor call.
     ++critical_section_count_;
+    if (getMode() && !inside) {
+        if (getThreadPoolSize()) {
+            thread_pool_.stop();
+        }
+        // Now it is safe to call callbacks which can also create other CSs.
+        callEntryCallbacks();
+    }
 }
 
 void
 MultiThreadingMgr::exitCriticalSection() {
-    std::lock_guard<std::mutex> lk(mutex_);
-    if (critical_section_count_ == 0) {
-        isc_throw(InvalidOperation, "invalid negative value for override");
+    // The number of CS destructors should match the number of CS constructors.
+    // The case when counter is 0 is only possible if calling this function
+    // explicitly, which is a programming error.
+    if (!isInCriticalSection()) {
+        isc_throw(InvalidOperation, "invalid value for critical section count");
     }
+    // Decrement the counter to allow the check for last CS destructor which
+    // would result in restarting the thread pool.
+    // The counter must be updated regardless of the MT mode because the MT mode
+    // can change between the constructor call and the destructor call.
     --critical_section_count_;
-    startProcessing();
+    if (getMode() && !isInCriticalSection()) {
+        if (getThreadPoolSize()) {
+            thread_pool_.start(getThreadPoolSize());
+        }
+        // Now it is safe to call callbacks which can also create other CSs.
+        callExitCallbacks();
+    }
 }
 
 bool
 MultiThreadingMgr::isInCriticalSection() {
-    checkCallbacksPermissions();
-    std::lock_guard<std::mutex> lk(mutex_);
-    return (isInCriticalSectionInternal());
-}
-
-bool
-MultiThreadingMgr::isInCriticalSectionInternal() {
     return (critical_section_count_ != 0);
 }
 
@@ -186,26 +201,6 @@ MultiThreadingMgr::callExitCallbacks() {
     }
 }
 
-void
-MultiThreadingMgr::stopProcessing() {
-    if (getMode() && !isInCriticalSectionInternal()) {
-        if (getThreadPoolSize()) {
-            thread_pool_.stop();
-        }
-        callEntryCallbacks();
-    }
-}
-
-void
-MultiThreadingMgr::startProcessing() {
-    if (getMode() && !isInCriticalSectionInternal()) {
-        if (getThreadPoolSize()) {
-            thread_pool_.start(getThreadPoolSize());
-        }
-        callExitCallbacks();
-    }
-}
-
 void
 MultiThreadingMgr::addCriticalSectionCallbacks(const std::string& name,
                                                const CSCallbackSet::Callback& check_cb,

diff --git a/src/lib/util/multi_threading_mgr.h b/src/lib/util/multi_threading_mgr.h
index ec2d99196057b3b9870e5d66d2ccbae4b6261d3b..fa3f3a353ead5b7239b0a048b9fa15e19fd52cb1 100644 (file)
--- a/src/lib/util/multi_threading_mgr.h
+++ b/src/lib/util/multi_threading_mgr.h
@@ -159,6 +159,12 @@ public:
     /// counter so that any configuration change that might start the packet
     /// thread pool is delayed until exiting the respective section.
     /// If the internal counter is 0, then stop the thread pool.
+    ///
+    /// Invokes all CriticalSection entry callbacks. Has no effect in
+    /// single-threaded mode.
+    ///
+    /// @note This function swallows exceptions thrown by all entry callbacks
+    /// without logging to avoid breaking the CS chain.
     void enterCriticalSection();
 
     /// @brief Exit critical section.
@@ -167,6 +173,12 @@ public:
     /// counter so that the dhcp thread pool can be started according to the
     /// new configuration.
     /// If the internal counter is 0, then start the thread pool.
+    ///
+    /// Invokes all CriticalSection exit callbacks. Has no effect in
+    /// single-threaded mode.
+    ///
+    /// @note This function swallows exceptions thrown by all exit callbacks
+    /// without logging to avoid breaking the CS chain.
     void exitCriticalSection();
 
     /// @brief Is in critical section flag.
@@ -259,13 +271,6 @@ protected:
 
 private:
 
-    /// @brief Is in critical section flag.
-    ///
-    /// Should be called in a thread safe context.
-    ///
-    /// @return The critical section flag.
-    bool isInCriticalSectionInternal();
-
     /// @brief Class method tests if current thread is allowed to enter the
     /// @ref MultiThreadingCriticalSection and to invoke the entry and exit
     /// callbacks.
@@ -296,25 +301,6 @@ private:
     /// without logging to avoid breaking the CS chain.
     void callExitCallbacks();
 
-    /// @brief Class method stops non-critical processing.
-    ///
-    /// Stops the DHCP thread pool if it's running and invokes all
-    /// CriticalSection entry callbacks. Has no effect in single-threaded mode.
-    ///
-    /// @note This function swallows exceptions thrown by all exit callbacks
-    /// without logging to avoid breaking the CS chain.
-    void stopProcessing();
-
-    /// @brief Class method (re)starts non-critical processing.
-    ///
-    /// Starts the DHCP thread pool according to current configuration, and
-    /// invokes all CriticalSection exit callbacks. Has no effect in
-    /// single-threaded mode.
-    ///
-    /// @note This function swallows exceptions thrown by all entry callbacks
-    /// without logging to avoid breaking the CS chain.
-    void startProcessing();
-
     /// @brief The current multi-threading mode.
     ///
     /// The multi-threading flag: true if multi-threading is enabled, false
@@ -337,9 +323,6 @@ private:
 
     /// @brief List of CriticalSection entry and exit callback sets.
     CSCallbackSetList cs_callbacks_;
-
-    /// @brief Mutex to protect the internal state.
-    std::mutex mutex_;
 };
 
 /// @note: everything here MUST be used ONLY from the main thread.