[#2043] add check permissions for current thread action on thread pools

11 files changed, 241 insertions, 79 deletions

diff --git a/src/hooks/dhcp/high_availability/ha_service.cc b/src/hooks/dhcp/high_availability/ha_service.cc
index 9e3042360c..033165d9a5 100644
--- a/src/hooks/dhcp/high_availability/ha_service.cc
+++ b/src/hooks/dhcp/high_availability/ha_service.cc
@@ -2814,9 +2814,34 @@ HAService::getPendingRequestInternal(const QueryPtrType& query) {
 }
 
 void
+HAService::checkPermissionsClientAndListener() {
+    // Since we're used as CS callback we need to suppress
+    // any exceptions, unlikely though they may be.
+    try {
+        if (client_) {
+            client_->checkPermissions();
+        }
+
+        if (listener_) {
+            listener_->checkPermissions();
+        }
+    } catch (const isc::MultiThreadingInvalidOperation& ex) {
+        LOG_ERROR(ha_logger, HA_PAUSE_CLIENT_LISTENER_ILLEGAL)
+                  .arg(ex.what());
+        // The exception needs to be propagated to the caller of the
+        // @ref MultiThreadingCriticalSection constructor.
+        throw;
+    } catch (const std::exception& ex) {
+        LOG_ERROR(ha_logger, HA_PAUSE_CLIENT_LISTENER_FAILED)
+                  .arg(ex.what());
+    }
+}
+
+void
 HAService::startClientAndListener() {
     // Add critical section callbacks.
     MultiThreadingMgr::instance().addCriticalSectionCallbacks("HA_MT",
+        std::bind(&HAService::checkPermissionsClientAndListener, this),
         std::bind(&HAService::pauseClientAndListener, this),
         std::bind(&HAService::resumeClientAndListener, this));
 
@@ -2834,24 +2859,13 @@ HAService::pauseClientAndListener() {
     // Since we're used as CS callback we need to suppress
     // any exceptions, unlikely though they may be.
     try {
-        // The listener is the only one handling commands, so if any command
-        // uses @ref MultiThreadingCriticalSection, it should throw first.
-        // In this situation there is no need to resume the client's
-        // @ref HttpThreadPool because it does not get paused in the first place.
-        if (listener_) {
-            listener_->pause();
-        }
-
         if (client_) {
             client_->pause();
         }
 
-    } catch (const isc::MultiThreadingInvalidOperation& ex) {
-        LOG_ERROR(ha_logger, HA_PAUSE_CLIENT_LISTENER_ILLEGAL)
-                  .arg(ex.what());
-        // The exception needs to be propagated to the caller of the
-        // @ref MultiThreadingCriticalSection constructor.
-        throw;
+        if (listener_) {
+            listener_->pause();
+        }
     } catch (const std::exception& ex) {
         LOG_ERROR(ha_logger, HA_PAUSE_CLIENT_LISTENER_FAILED)
                   .arg(ex.what());
@@ -2881,13 +2895,13 @@ HAService::stopClientAndListener() {
     // Remove critical section callbacks.
     MultiThreadingMgr::instance().removeCriticalSectionCallbacks("HA_MT");
 
-    if (listener_) {
-        listener_->stop();
-    }
-
     if (client_) {
         client_->stop();
     }
+
+    if (listener_) {
+        listener_->stop();
+    }
 }
 
 // Explicit instantiations.
diff --git a/src/hooks/dhcp/high_availability/ha_service.h b/src/hooks/dhcp/high_availability/ha_service.h
index 1ff786349c..159b0f0e47 100644
--- a/src/hooks/dhcp/high_availability/ha_service.h
+++ b/src/hooks/dhcp/high_availability/ha_service.h
@@ -1002,6 +1002,13 @@ public:
     /// @return Pointer to the response to the ha-maintenance-cancel.
     data::ConstElementPtr processMaintenanceCancel();
 
+    /// @brief Check client and(or) listener current thread permissions to
+    /// perform thread pool state transition.
+    ///
+    /// @throw MultiThreadingInvalidOperation if the state transition is done on
+    /// any of the owned threads
+    void checkPermissionsClientAndListener();
+
     /// @brief Start the client and(or) listener instances.
     ///
     /// When HA+MT is enabled it starts the client's thread pool
diff --git a/src/lib/config/cmd_http_listener.cc b/src/lib/config/cmd_http_listener.cc
index 7063e17b4e..36c3678219 100644
--- a/src/lib/config/cmd_http_listener.cc
+++ b/src/lib/config/cmd_http_listener.cc
@@ -83,6 +83,13 @@ CmdHttpListener::start() {
 }
 
 void
+CmdHttpListener::checkPermissions() {
+    if (thread_pool_) {
+        thread_pool_->checkPausePermissions();
+    }
+}
+
+void
 CmdHttpListener::pause() {
     if (thread_pool_) {
         thread_pool_->pause();
diff --git a/src/lib/config/cmd_http_listener.h b/src/lib/config/cmd_http_listener.h
index 4b1f414218..bc59d214d5 100644
--- a/src/lib/config/cmd_http_listener.h
+++ b/src/lib/config/cmd_http_listener.h
@@ -38,6 +38,13 @@ public:
     /// @brief Destructor
     virtual ~CmdHttpListener();
 
+    /// @brief Check if the current thread can perform thread pool state
+    /// transition.
+    ///
+    /// @throw MultiThreadingInvalidOperation if the state transition is done on
+    /// any of the owned threads
+    void checkPermissions();
+
     /// @brief Starts running the listener's thread pool.
     void start();
 
diff --git a/src/lib/http/client.cc b/src/lib/http/client.cc
index 7b7a9d4687..574049b150 100644
--- a/src/lib/http/client.cc
+++ b/src/lib/http/client.cc
@@ -1778,6 +1778,17 @@ public:
         stop();
     }
 
+    /// @brief Check if the current thread can perform thread pool state
+    /// transition.
+    ///
+    /// @throw MultiThreadingInvalidOperation if the state transition is done on
+    /// any of the owned threads
+    void checkPermissions() {
+        if (thread_pool_) {
+            thread_pool_->checkPausePermissions();
+        }
+    }
+
     /// @brief Starts running the client's thread pool, if multi-threaded.
     void start() {
         if (thread_pool_) {
@@ -1964,6 +1975,11 @@ HttpClient::start() {
 }
 
 void
+HttpClient::checkPermissions() {
+    impl_->checkPermissions();
+}
+
+void
 HttpClient::pause() {
     impl_->pause();
 }
diff --git a/src/lib/http/client.h b/src/lib/http/client.h
index 34a0a23acc..6670b7f73d 100644
--- a/src/lib/http/client.h
+++ b/src/lib/http/client.h
@@ -249,6 +249,13 @@ public:
                           const CloseHandler& close_callback =
                           CloseHandler());
 
+    /// @brief Check if the current thread can perform thread pool state
+    /// transition.
+    ///
+    /// @throw MultiThreadingInvalidOperation if the state transition is done on
+    /// any of the owned threads
+    void checkPermissions();
+
     /// @brief Starts running the client's thread pool, if multi-threaded.
     void start();
 
diff --git a/src/lib/http/http_thread_pool.cc b/src/lib/http/http_thread_pool.cc
index 3f7bb21416..638c7be999 100644
--- a/src/lib/http/http_thread_pool.cc
+++ b/src/lib/http/http_thread_pool.cc
@@ -101,6 +101,20 @@ HttpThreadPool::stateToText(State state) {
     return (std::string("unknown-state"));
 }
 
+void
+HttpThreadPool::checkPausePermissions() {
+    checkPermissions(State::PAUSED);
+}
+
+void
+HttpThreadPool::checkPermissions(State new_state) {
+    auto id = std::this_thread::get_id();
+    if (checkThreadId(id)) {
+        isc_throw(MultiThreadingInvalidOperation, "invalid thread pool state change to "
+                  << HttpThreadPool::stateToText(new_state) << " performed by owned thread");
+    }
+}
+
 bool
 HttpThreadPool::checkThreadId(std::thread::id id) {
     for (auto thread : threads_) {
@@ -113,11 +127,7 @@ HttpThreadPool::checkThreadId(std::thread::id id) {
 
 void
 HttpThreadPool::setState(State new_state) {
-    auto id = std::this_thread::get_id();
-    if (checkThreadId(id)) {
-        isc_throw(MultiThreadingInvalidOperation, "invalid thread pool state change to "
-                  << HttpThreadPool::stateToText(new_state) << " performed by owned thread");
-    }
+    checkPermissions(new_state);
 
     std::unique_lock<std::mutex> main_lck(mutex_);
 
diff --git a/src/lib/http/http_thread_pool.h b/src/lib/http/http_thread_pool.h
index 3027d3fac4..d28512200e 100644
--- a/src/lib/http/http_thread_pool.h
+++ b/src/lib/http/http_thread_pool.h
@@ -92,7 +92,20 @@ public:
         return (getState() == State::STOPPED);
     }
 
+    /// @brief Check current thread permissions to transition to the new PAUSED
+    /// state.
+    /// @throw MultiThreadingInvalidOperation if the state transition is done on
+    /// any of the owned threads
+    void checkPausePermissions();
+
 private:
+    /// @brief Check current thread permissions to transition to the new state.
+    ///
+    /// @param state The new transition state for the pool.
+    /// @throw MultiThreadingInvalidOperation if the state transition is done on
+    /// any of the owned threads
+    void checkPermissions(State state);
+
     /// @brief Check specified thread id against own threads.
     ///
     /// @return true if thread is owned, false otherwise.
@@ -129,7 +142,7 @@ private:
     /// -# Waits until all threads have paused.
     /// -# Returns to caller.
     ///
-    /// @param state new state for the pool.
+    /// @param state The new transition state for the pool.
     /// @throw MultiThreadingInvalidOperation if the state transition is done on
     /// any of the owned threads.
     void setState(State state);
diff --git a/src/lib/util/multi_threading_mgr.cc b/src/lib/util/multi_threading_mgr.cc
index 3972f5e842..b23617b62b 100644
--- a/src/lib/util/multi_threading_mgr.cc
+++ b/src/lib/util/multi_threading_mgr.cc
@@ -36,6 +36,7 @@ MultiThreadingMgr::setMode(bool enabled) {
 
 void
 MultiThreadingMgr::enterCriticalSection() {
+    checkCallbacksPermissions();
     std::lock_guard<std::mutex> lk(mutex_);
     stopProcessing();
     ++critical_section_count_;
@@ -43,6 +44,7 @@ MultiThreadingMgr::enterCriticalSection() {
 
 void
 MultiThreadingMgr::exitCriticalSection() {
+    checkCallbacksPermissions();
     std::lock_guard<std::mutex> lk(mutex_);
     if (critical_section_count_ == 0) {
         isc_throw(InvalidOperation, "invalid negative value for override");
@@ -53,6 +55,7 @@ MultiThreadingMgr::exitCriticalSection() {
 
 bool
 MultiThreadingMgr::isInCriticalSection() {
+    checkCallbacksPermissions();
     std::lock_guard<std::mutex> lk(mutex_);
     return (isInCriticalSectionInternal());
 }
@@ -127,14 +130,11 @@ MultiThreadingMgr::apply(bool enabled, uint32_t thread_count, uint32_t queue_siz
 }
 
 void
-MultiThreadingMgr::stopProcessing() {
-    if (getMode() && !isInCriticalSectionInternal()) {
-        // First call the registered callback for entering the critical section
-        // so that if any exception is thrown, there is no need to stop and then
-        // start the service threads.
+MultiThreadingMgr::checkCallbacksPermissions() {
+    if (getMode()) {
         for (const auto& cb : cs_callbacks_.getCallbackPairs()) {
             try {
-                (cb.entry_cb_)();
+                (cb.check_cb_)();
             } catch (const isc::MultiThreadingInvalidOperation& ex) {
                 // If any registered callback throws, the exception needs to be
                 // propagated to the caller of the
@@ -142,8 +142,6 @@ MultiThreadingMgr::stopProcessing() {
                 // Because this function is called by the
                 // @ref MultiThreadingCriticalSection constructor, throwing here
                 // is safe.
-                // @note should we call the exit_cb_ here for all successful
-                // entry_cb_ already run?
                 throw;
             } catch (...) {
                 // We can't log it and throwing could be chaos.
@@ -151,23 +149,32 @@ MultiThreadingMgr::stopProcessing() {
                 // must be exception-proof
             }
         }
-
-        if (getThreadPoolSize()) {
-            thread_pool_.stop();
-        }
     }
 }
 
 void
-MultiThreadingMgr::startProcessing() {
-    if (getMode() && !isInCriticalSectionInternal()) {
-        if (getThreadPoolSize()) {
-            thread_pool_.start(getThreadPoolSize());
+MultiThreadingMgr::callEntryCallbacks() {
+    if (getMode()) {
+        const auto& callbacks = cs_callbacks_.getCallbackPairs();
+        for (auto cb_it = callbacks.begin(); cb_it != callbacks.end(); cb_it++) {
+            try {
+                (cb_it->entry_cb_)();
+            } catch (...) {
+                // We can't log it and throwing could be chaos.
+                // We'll swallow it and tell people their callbacks
+                // must be exception-proof
+            }
         }
+    }
+}
 
-        for (const auto& cb : cs_callbacks_.getCallbackPairs()) {
+void
+MultiThreadingMgr::callExitCallbacks() {
+    if (getMode()) {
+        const auto& callbacks = cs_callbacks_.getCallbackPairs();
+        for (auto cb_it = callbacks.rbegin(); cb_it != callbacks.rend(); cb_it++) {
             try {
-                (cb.exit_cb_)();
+                (cb_it->exit_cb_)();
             } catch (...) {
                 // We can't log it and throwing could be chaos.
                 // We'll swallow it and tell people their callbacks
@@ -175,18 +182,37 @@ MultiThreadingMgr::startProcessing() {
                 // Because this function is called by the
                 // @ref MultiThreadingCriticalSection destructor, throwing here
                 // is not safe and will cause the process to crash.
-                // @note should we call the exit_cb_ here in reverse order of
-                // the entry_cb_ calls?
             }
         }
     }
 }
 
 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 CSCallbackPair::Callback& check_cb,
                                                const CSCallbackPair::Callback& entry_cb,
                                                const CSCallbackPair::Callback& exit_cb) {
-    cs_callbacks_.addCallbackPair(name, entry_cb, exit_cb);
+    cs_callbacks_.addCallbackPair(name, check_cb, entry_cb, exit_cb);
 }
 
 void
@@ -209,12 +235,18 @@ MultiThreadingCriticalSection::~MultiThreadingCriticalSection() {
 
 void
 CSCallbackPairList::addCallbackPair(const std::string& name,
+                                    const CSCallbackPair::Callback& check_cb,
                                     const CSCallbackPair::Callback& entry_cb,
                                     const CSCallbackPair::Callback& exit_cb) {
     if (name.empty()) {
         isc_throw(BadValue, "CSCallbackPairList - name cannot be empty");
     }
 
+    if (!check_cb) {
+        isc_throw(BadValue, "CSCallbackPairList - check callback for " << name
+                  << " cannot be empty");
+    }
+
     if (!entry_cb) {
         isc_throw(BadValue, "CSCallbackPairList - entry callback for " << name
                   << " cannot be empty");
@@ -232,7 +264,7 @@ CSCallbackPairList::addCallbackPair(const std::string& name,
         }
     }
 
-    cb_pairs_.push_back(CSCallbackPair(name, entry_cb, exit_cb));
+    cb_pairs_.push_back(CSCallbackPair(name, check_cb, entry_cb, exit_cb));
 }
 
 void
diff --git a/src/lib/util/multi_threading_mgr.h b/src/lib/util/multi_threading_mgr.h
index 6c9613fa69..abc7cb93ef 100644
--- a/src/lib/util/multi_threading_mgr.h
+++ b/src/lib/util/multi_threading_mgr.h
@@ -21,11 +21,12 @@ namespace util {
 
 /// @brief Embodies a named pair of CriticalSection callbacks.
 ///
-/// This class associates a pair of callbacks, one to be invoked
-/// upon CriticalSection entry and the other invoked upon
-/// CriticalSection exit, with name.  The name allows the pair
-/// to be uniquely identified such that they can be added and
-/// removed as needed.
+/// This class associates with a name, a pair of callbacks, one to be invoked
+/// before CriticalSection entry and exit callbacks to validate current thread
+/// permissions to perform such actions, one to be invoked upon CriticalSection
+/// entry and one to be invoked upon CriticalSection exit,
+/// The name allows the pair to be uniquely identified such that they can be
+/// added and removed as needed.
 struct CSCallbackPair {
     /// @brief Defines a callback as a simple void() functor.
     typedef std::function<void()> Callback;
@@ -33,15 +34,21 @@ struct CSCallbackPair {
     /// @brief Constructor
     ///
     /// @param name Name by which the callbacks can be found.
+    /// @param entry_cb Callback to check current thread permissions to call
+    /// the CriticalSection entry and exit callbacks.
     /// @param entry_cb Callback to invoke upon CriticalSection entry.
     /// @param entry_cb Callback to invoke upon CriticalSection exit.
-    CSCallbackPair(const std::string& name, const Callback& entry_cb,
-                   const Callback& exit_cb)
-        : name_(name), entry_cb_(entry_cb), exit_cb_(exit_cb) {}
+    CSCallbackPair(const std::string& name, const Callback& check_cb,
+                   const Callback& entry_cb, const Callback& exit_cb)
+        : name_(name), check_cb_(check_cb), entry_cb_(entry_cb),
+          exit_cb_(exit_cb) {}
 
     /// @brief Name by which the callback can be found.
     std::string name_;
 
+    /// @brief Check permissions callback associated with name.
+    Callback check_cb_;
+
     /// @brief Entry point callback associated with name.
     Callback entry_cb_;
 
@@ -63,12 +70,14 @@ public:
     /// @brief Adds a callback pair to the list.
     ///
     /// @param name Name of the callback to add.
-    /// @param entry_cb Callback to add.
-    /// @param exit_cb Callback to add.
+    /// @param check_cb The check permissions callback to add.
+    /// @param entry_cb The CriticalSection entry callback to add.
+    /// @param exit_cb The CriticalSection exit callback to add.
     ///
     /// @throw BadValue if the name is already in the list,
     /// the name is blank, or either callback is empty.
     void addCallbackPair(const std::string& name,
+                         const CSCallbackPair::Callback& check_cb,
                          const CSCallbackPair::Callback& entry_cb,
                          const CSCallbackPair::Callback& exit_cb);
 
@@ -89,6 +98,8 @@ private:
     std::list<CSCallbackPair> cb_pairs_;
 };
 
+class MultiThreadingCriticalSection;
+
 /// @brief Multi Threading Manager.
 ///
 /// This singleton class holds the multi-threading mode.
@@ -119,7 +130,6 @@ private:
 /// @endcode
 class MultiThreadingMgr : public boost::noncopyable {
 public:
-
     /// @brief Returns a single instance of Multi Threading Manager.
     ///
     /// MultiThreadingMgr is a singleton and this method is the only way
@@ -212,11 +222,14 @@ public:
     ///
     /// @param name Name of the set of callbacks. This value is used by the
     /// callback owner to add and remove them. Duplicates are not allowed.
+    /// @param entry_cb Callback to check current thread permissions to call
+    /// the CriticalSection entry and exit callbacks.
     /// @param entry_cb Callback to invoke upon CriticalSection entry. Cannot be
     /// empty.
     /// @param exit_cb Callback to invoke upon CriticalSection exit. Cannot be
     /// empty.
     void addCriticalSectionCallbacks(const std::string& name,
+                                     const CSCallbackPair::Callback& check_cb,
                                      const CSCallbackPair::Callback& entry_cb,
                                      const CSCallbackPair::Callback& exit_cb);
 
@@ -249,6 +262,37 @@ private:
     /// @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 stop and start
+    /// callbacks.
+    ///
+    /// Has no effect in single-threaded mode.
+    ///
+    /// @note This function swallows exceptions thrown by validate
+    /// callbacks without logging to avoid breaking the CS
+    /// chain.
+    /// @throw MultiThreadingInvalidOperation if current thread has no
+    /// permission to enter CriticalSection.
+    void checkCallbacksPermissions();
+
+    /// @brief Class method which invokes CriticalSection entry callbacks.
+    ///
+    /// Has no effect in single-threaded mode.
+    ///
+    /// @note This function swallows exceptions thrown by validate
+    /// callbacks without logging to avoid breaking the CS
+    /// chain.
+    void callEntryCallbacks();
+
+    /// @brief Class method which invokes CriticalSection entry callbacks.
+    ///
+    /// Has no effect in single-threaded mode.
+    ///
+    /// @note This function swallows exceptions thrown by validate
+    /// callbacks 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
diff --git a/src/lib/util/tests/multi_threading_mgr_unittest.cc b/src/lib/util/tests/multi_threading_mgr_unittest.cc
index 088073f0d1..4d9dd7d547 100644
--- a/src/lib/util/tests/multi_threading_mgr_unittest.cc
+++ b/src/lib/util/tests/multi_threading_mgr_unittest.cc
@@ -357,18 +357,15 @@ public:
         invocations_.push_back(4);
     }
 
-    /// @brief A callback that adds the value 5 to invocations lists and throws
-    /// isc::Exception which is ignored.
+    /// @brief A callback that throws @ref isc::Exception which is ignored.
     void ignoredException() {
-        invocations_.push_back(5);
         isc_throw(isc::Exception, "ignored");
     }
 
-    /// @brief A callback that adds the value 6 to invocations lists and throws
-    /// isc::MultiThreadingInvalidOperation which is propagated to the scope of
-    /// the @ref MultiThreadingCriticalSection constructor.
+    /// @brief A callback that throws @ref isc::MultiThreadingInvalidOperation
+    /// which is propagated to the scope of the
+    /// @ref MultiThreadingCriticalSection constructor.
     void observedException() {
-        invocations_.push_back(6);
         isc_throw(isc::MultiThreadingInvalidOperation, "observed");
     }
 
@@ -412,7 +409,7 @@ public:
             if (entries.size()) {
                 // We expect entry invocations.
                 ASSERT_EQ(invocations_.size(), entries.size());
-                ASSERT_TRUE(invocations_ == entries);
+                ASSERT_EQ(invocations_, entries);
             } else {
                 // We do not expect entry invocations.
                 ASSERT_FALSE(invocations_.size());
@@ -444,7 +441,7 @@ public:
             if (entries.size()) {
                 // We expect entry invocations.
                 ASSERT_EQ(invocations_.size(), entries.size());
-                ASSERT_TRUE(invocations_ == entries);
+                ASSERT_EQ(invocations_, entries);
             } else {
                 // We do not expect entry invocations.
                 ASSERT_FALSE(invocations_.size());
@@ -460,7 +457,7 @@ public:
 
         if (exits.size()) {
             // We expect exit invocations.
-            ASSERT_TRUE(invocations_ == exits);
+            ASSERT_EQ(invocations_, exits);
         } else {
             // We do not expect exit invocations.
             ASSERT_FALSE(invocations_.size());
@@ -477,26 +474,30 @@ TEST_F(CriticalSectionCallbackTest, addAndRemove) {
     auto& mgr = MultiThreadingMgr::instance();
 
     // Cannot add with a blank name.
-    ASSERT_THROW_MSG(mgr.addCriticalSectionCallbacks("", [](){}, [](){}),
+    ASSERT_THROW_MSG(mgr.addCriticalSectionCallbacks("", [](){}, [](){}, [](){}),
                      BadValue, "CSCallbackPairList - name cannot be empty");
 
-    // Cannot add with an empty entry callback.
-    ASSERT_THROW_MSG(mgr.addCriticalSectionCallbacks("bad", nullptr, [](){}),
+    // Cannot add with an empty check callback.
+    ASSERT_THROW_MSG(mgr.addCriticalSectionCallbacks("bad", nullptr, [](){}, [](){}),
+                     BadValue, "CSCallbackPairList - check callback for bad cannot be empty");
+
+    // Cannot add with an empty exit callback.
+    ASSERT_THROW_MSG(mgr.addCriticalSectionCallbacks("bad", [](){}, nullptr, [](){}),
                      BadValue, "CSCallbackPairList - entry callback for bad cannot be empty");
 
     // Cannot add with an empty exit callback.
-    ASSERT_THROW_MSG(mgr.addCriticalSectionCallbacks("bad", [](){}, nullptr),
+    ASSERT_THROW_MSG(mgr.addCriticalSectionCallbacks("bad", [](){}, [](){}, nullptr),
                      BadValue, "CSCallbackPairList - exit callback for bad cannot be empty");
 
     // Should be able to add foo.
-    ASSERT_NO_THROW_LOG(mgr.addCriticalSectionCallbacks("foo", [](){}, [](){}));
+    ASSERT_NO_THROW_LOG(mgr.addCriticalSectionCallbacks("foo", [](){}, [](){}, [](){}));
 
     // Should not be able to add foo twice.
-    ASSERT_THROW_MSG(mgr.addCriticalSectionCallbacks("foo", [](){}, [](){}),
+    ASSERT_THROW_MSG(mgr.addCriticalSectionCallbacks("foo", [](){}, [](){}, [](){}),
                      BadValue, "CSCallbackPairList - callbacks for foo already exist");
 
     // Should be able to add bar.
-    ASSERT_NO_THROW_LOG(mgr.addCriticalSectionCallbacks("bar", [](){}, [](){}));
+    ASSERT_NO_THROW_LOG(mgr.addCriticalSectionCallbacks("bar", [](){}, [](){}, [](){}));
 
     // Should be able to remove foo.
     ASSERT_NO_THROW_LOG(mgr.removeCriticalSectionCallbacks("foo"));
@@ -517,10 +518,12 @@ TEST_F(CriticalSectionCallbackTest, invocations) {
 
     // Add two sets of CriticalSection call backs.
     MultiThreadingMgr::instance().addCriticalSectionCallbacks("oneAndTwo",
+         std::bind(&CriticalSectionCallbackTest::ignoredException, this),
          std::bind(&CriticalSectionCallbackTest::one, this),
          std::bind(&CriticalSectionCallbackTest::two, this));
 
     MultiThreadingMgr::instance().addCriticalSectionCallbacks("threeAndFour",
+         std::bind(&CriticalSectionCallbackTest::ignoredException, this),
          std::bind(&CriticalSectionCallbackTest::three, this),
          std::bind(&CriticalSectionCallbackTest::four, this));
 
@@ -531,7 +534,7 @@ TEST_F(CriticalSectionCallbackTest, invocations) {
     // callbacks execute at the appropriate times and we can do
     // so repeatedly.
     for (int i = 0; i < 3; ++i) {
-        runCriticalSections({1,3}, {2,4});
+        runCriticalSections({1 ,3}, {2, 4});
     }
 
     // Now remove the first set of callbacks.
@@ -557,11 +560,13 @@ TEST_F(CriticalSectionCallbackTest, invocationsWithExceptions) {
     // Add two sets of CriticalSection call backs.
     MultiThreadingMgr::instance().addCriticalSectionCallbacks("observed",
          std::bind(&CriticalSectionCallbackTest::observedException, this),
-         std::bind(&CriticalSectionCallbackTest::observedException, this));
+         std::bind(&CriticalSectionCallbackTest::one, this),
+         std::bind(&CriticalSectionCallbackTest::two, this));
 
     MultiThreadingMgr::instance().addCriticalSectionCallbacks("ignored",
          std::bind(&CriticalSectionCallbackTest::ignoredException, this),
-         std::bind(&CriticalSectionCallbackTest::ignoredException, this));
+         std::bind(&CriticalSectionCallbackTest::three, this),
+         std::bind(&CriticalSectionCallbackTest::four, this));
 
     // Apply multi-threading configuration with 16 threads and queue size 256.
     MultiThreadingMgr::instance().apply(true, 16, 256);
@@ -570,14 +575,14 @@ TEST_F(CriticalSectionCallbackTest, invocationsWithExceptions) {
     // callbacks execute at the appropriate times and we can do
     // so repeatedly.
     for (int i = 0; i < 3; ++i) {
-        runCriticalSections({6}, {}, true);
+        runCriticalSections({}, {}, true);
     }
 
     // Now remove the first set of callbacks.
     MultiThreadingMgr::instance().removeCriticalSectionCallbacks("observed");
 
     // Retest CriticalSections.
-    runCriticalSections({5}, {5});
+    runCriticalSections({3}, {4});
 
     // Now remove the remaining callbacks.
     MultiThreadingMgr::instance().removeAllCriticalSectionCallbacks();