[#2043] added critical section with exceptions unittests

diff --git a/src/lib/config/tests/cmd_http_listener_unittests.cc b/src/lib/config/tests/cmd_http_listener_unittests.cc
index 55b2fbf25c77c5cdcd19d702da5e5d8beb034337..4eca580ff6d36a1c2a38d7e0aad5e9e61c9c43b4 100644 (file)
--- a/src/lib/config/tests/cmd_http_listener_unittests.cc
+++ b/src/lib/config/tests/cmd_http_listener_unittests.cc
@@ -241,10 +241,6 @@ public:
                                       const ConstElementPtr& /*command_arguments*/) {
         ElementPtr arguments = Element::createList();
         arguments->add(Element::create("bar"));
-        EXPECT_THROW(listener_->start(), InvalidOperation);
-        EXPECT_THROW(listener_->pause(), MultiThreadingInvalidOperation);
-        EXPECT_THROW(listener_->resume(), MultiThreadingInvalidOperation);
-        EXPECT_THROW(listener_->stop(), MultiThreadingInvalidOperation);
         return (createAnswer(CONTROL_RESULT_SUCCESS, arguments));
     }
 
@@ -311,6 +307,11 @@ public:
             }
         }
 
+        EXPECT_THROW(listener_->start(), InvalidOperation);
+        EXPECT_THROW(listener_->pause(), MultiThreadingInvalidOperation);
+        EXPECT_THROW(listener_->resume(), MultiThreadingInvalidOperation);
+        EXPECT_THROW(listener_->stop(), MultiThreadingInvalidOperation);
+
         // We're done, ship it!
         return (createAnswer(CONTROL_RESULT_SUCCESS, arguments));
     }

diff --git a/src/lib/http/http_thread_pool.cc b/src/lib/http/http_thread_pool.cc
index dfa995d67713eae937f856c3de80b40e6c52e3ec..3f7bb2141693044423f8827c43eff2897a0d2737 100644 (file)
--- a/src/lib/http/http_thread_pool.cc
+++ b/src/lib/http/http_thread_pool.cc
@@ -36,7 +36,7 @@ HttpThreadPool::HttpThreadPool(IOServicePtr io_service, size_t pool_size,
       run_state_(State::STOPPED), mutex_(), thread_cv_(),
       main_cv_(), paused_(0), running_(0), exited_(0)  {
     if (!pool_size) {
-        isc_throw(BadValue, "HttpThreadPool::ctor pool_size must be > 0");
+        isc_throw(BadValue, "pool_size must be non 0");
     }
 
     // If we weren't given an IOService, create our own.

diff --git a/src/lib/http/tests/client_mt_unittests.cc b/src/lib/http/tests/client_mt_unittests.cc
index d5d8cfc99639cee936edc1769679b6fe904283fd..84b3b31a2065625e6ba4e2dc73751c9b5a8180df 100644 (file)
--- a/src/lib/http/tests/client_mt_unittests.cc
+++ b/src/lib/http/tests/client_mt_unittests.cc
@@ -320,6 +320,12 @@ public:
                 }
             }
 
+            if (num_threads_) {
+                EXPECT_THROW(client_->start(), MultiThreadingInvalidOperation);
+                EXPECT_THROW(client_->pause(), MultiThreadingInvalidOperation);
+                EXPECT_THROW(client_->resume(), MultiThreadingInvalidOperation);
+            }
+
             // Get stringified thread-id.
             std::stringstream ss;
             ss << std::this_thread::get_id();

diff --git a/src/lib/http/tests/http_thread_pool_unittests.cc b/src/lib/http/tests/http_thread_pool_unittests.cc
index fcc088bd773997bca66d33f0d4ac536fb03b6461..aa08ada459cd6ea4eb241f1d24c387bd1638d110 100644 (file)
--- a/src/lib/http/tests/http_thread_pool_unittests.cc
+++ b/src/lib/http/tests/http_thread_pool_unittests.cc
@@ -47,7 +47,7 @@ TEST_F(HttpThreadPoolTest, invalidConstruction) {
 
     // Constructing with pool size of 0 should fail.
     ASSERT_THROW_MSG(pool.reset(new HttpThreadPool(io_service_, 0)), BadValue,
-                     "HttpThreadPool::ctor pool_size must be > 0");
+                     "pool_size must be non 0");
 }
 
 // Verifies that a pool can be created without starting it.

diff --git a/src/lib/util/multi_threading_mgr.cc b/src/lib/util/multi_threading_mgr.cc
index 9093873158c1ab6d363419c79e5229b01068559f..3972f5e8423054bda07a183c258741fc1e714e3e 100644 (file)
--- a/src/lib/util/multi_threading_mgr.cc
+++ b/src/lib/util/multi_threading_mgr.cc
@@ -142,6 +142,8 @@ 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.
@@ -173,6 +175,8 @@ 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?
             }
         }
     }

diff --git a/src/lib/util/tests/multi_threading_mgr_unittest.cc b/src/lib/util/tests/multi_threading_mgr_unittest.cc
index 64182a871eef09b62d4d7ec7e05995c8218ed610..088073f0d1dd1fd8857070265969aafcc549942d 100644 (file)
--- a/src/lib/util/tests/multi_threading_mgr_unittest.cc
+++ b/src/lib/util/tests/multi_threading_mgr_unittest.cc
@@ -328,28 +328,50 @@ TEST(MultiThreadingMgrTest, criticalSection) {
 class CriticalSectionCallbackTest : public ::testing::Test {
 public:
     /// @brief Constructor.
-    CriticalSectionCallbackTest() {}
+    CriticalSectionCallbackTest() {
+        MultiThreadingMgr::instance().apply(false, 0, 0);
+    }
+
+    /// @brief Destructor.
+    ~CriticalSectionCallbackTest() {
+        MultiThreadingMgr::instance().apply(false, 0, 0);
+    }
 
-    /// @brief A callback that adds the value, 1, to invocations lists.
+    /// @brief A callback that adds the value 1 to invocations lists.
     void one() {
         invocations_.push_back(1);
     }
 
-    /// @brief A callback that adds the value, 2, to invocations lists.
+    /// @brief A callback that adds the value 2 to invocations lists.
     void two() {
         invocations_.push_back(2);
     }
 
-    /// @brief A callback that adds the value, 3, to invocations lists.
+    /// @brief A callback that adds the value 3 to invocations lists.
     void three() {
         invocations_.push_back(3);
     }
 
-    /// @brief A callback that adds the value, 4, to invocations lists.
+    /// @brief A callback that adds the value 4 to invocations lists.
     void four() {
         invocations_.push_back(4);
     }
 
+    /// @brief A callback that adds the value 5 to invocations lists and throws
+    /// 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.
+    void observedException() {
+        invocations_.push_back(6);
+        isc_throw(isc::MultiThreadingInvalidOperation, "observed");
+    }
+
     /// @brief Indicates whether or not the DHCP thread pool is running.
     ///
     /// @return True if the pool is running, false otherwise.
@@ -368,7 +390,11 @@ public:
     /// be present after exiting the outermost CriticalSection.  The
     /// expected values should be in the order the callbacks were added
     /// to the MultiThreadingMgr's list of callbacks.
-    void runCriticalSections(std::vector<int> entries, std::vector<int>exits) {
+    /// @param should_throw The flag indicating if the CriticalSection should
+    /// throw, simulating a dead-lock scenario when a processing thread tries
+    /// to stop the thread pool.
+    void runCriticalSections(std::vector<int> entries, std::vector<int>exits,
+                             bool should_throw = false) {
         // Pool must be running.
         ASSERT_TRUE(isThreadPoolRunning());
 
@@ -376,7 +402,7 @@ public:
         invocations_.clear();
 
         // Use scope to create nested CriticalSections.
-        {
+        if (!should_throw) {
             // Enter a critical section.
             MultiThreadingCriticalSection cs;
 
@@ -412,6 +438,20 @@ public:
 
             // We should not have had more callback invocations.
             ASSERT_FALSE(invocations_.size());
+        } else {
+            ASSERT_THROW(MultiThreadingCriticalSection cs, MultiThreadingInvalidOperation);
+
+            if (entries.size()) {
+                // We expect entry invocations.
+                ASSERT_EQ(invocations_.size(), entries.size());
+                ASSERT_TRUE(invocations_ == entries);
+            } else {
+                // We do not expect entry invocations.
+                ASSERT_FALSE(invocations_.size());
+            }
+
+            // Clear the invocations list.
+            invocations_.clear();
         }
 
         // After exiting the outer section, the thread pool should
@@ -506,3 +546,42 @@ TEST_F(CriticalSectionCallbackTest, invocations) {
     // Retest CriticalSections.
     runCriticalSections({}, {});
 }
+
+/// @brief Verifies that the critical section callbacks work.
+TEST_F(CriticalSectionCallbackTest, invocationsWithExceptions) {
+    // get the thread pool instance
+    auto& thread_pool = MultiThreadingMgr::instance().getThreadPool();
+    // thread pool should be stopped
+    EXPECT_EQ(thread_pool.size(), 0);
+
+    // Add two sets of CriticalSection call backs.
+    MultiThreadingMgr::instance().addCriticalSectionCallbacks("observed",
+         std::bind(&CriticalSectionCallbackTest::observedException, this),
+         std::bind(&CriticalSectionCallbackTest::observedException, this));
+
+    MultiThreadingMgr::instance().addCriticalSectionCallbacks("ignored",
+         std::bind(&CriticalSectionCallbackTest::ignoredException, this),
+         std::bind(&CriticalSectionCallbackTest::ignoredException, this));
+
+    // Apply multi-threading configuration with 16 threads and queue size 256.
+    MultiThreadingMgr::instance().apply(true, 16, 256);
+
+    // Make three passes over nested CriticalSections to ensure
+    // callbacks execute at the appropriate times and we can do
+    // so repeatedly.
+    for (int i = 0; i < 3; ++i) {
+        runCriticalSections({6}, {}, true);
+    }
+
+    // Now remove the first set of callbacks.
+    MultiThreadingMgr::instance().removeCriticalSectionCallbacks("observed");
+
+    // Retest CriticalSections.
+    runCriticalSections({5}, {5});
+
+    // Now remove the remaining callbacks.
+    MultiThreadingMgr::instance().removeAllCriticalSectionCallbacks();
+
+    // Retest CriticalSections.
+    runCriticalSections({}, {});
+}