diff --git a/src/hooks/dhcp/high_availability/ha_messages.cc b/src/hooks/dhcp/high_availability/ha_messages.cc index 93cca6f8eb..c360fd781d 100644 --- a/src/hooks/dhcp/high_availability/ha_messages.cc +++ b/src/hooks/dhcp/high_availability/ha_messages.cc @@ -185,11 +185,11 @@ const char* values[] = { "HA_MAINTENANCE_STARTED_IN_PARTNER_DOWN", "the server is now in the partner-down mode as a result of requested maintenance", "HA_MAINTENANCE_START_HANDLER_FAILED", "ha-maintenance-start command failed: %1", "HA_MISSING_CONFIGURATION", "high-availability parameter not specified for High Availability hooks library", - "HA_PAUSE_CLIENT_LISTENER_FAILED", "Pausing mutli-threaded HTTP processing failed: %1", + "HA_PAUSE_CLIENT_LISTENER_FAILED", "Pausing multi-threaded HTTP processing failed: %1", "HA_RESET_COMMUNICATIONS_FAILED", "failed to send ha-reset command to %1: %2", "HA_RESET_FAILED", "failed to reset HA state machine of %1: %2", "HA_RESET_HANDLER_FAILED", "ha-reset command failed: %1", - "HA_RESUME_CLIENT_LISTENER_FAILED", "Resuming mutli-threaded HTTP processing failed: %1", + "HA_RESUME_CLIENT_LISTENER_FAILED", "Resuming multi-threaded HTTP processing failed: %1", "HA_SCOPES_HANDLER_FAILED", "ha-scopes command failed: %1", "HA_SERVICE_STARTED", "started high availability service in %1 mode as %2 server", "HA_STATE_MACHINE_CONTINUED", "state machine is un-paused", diff --git a/src/hooks/dhcp/high_availability/ha_messages.mes b/src/hooks/dhcp/high_availability/ha_messages.mes index 5799a601ea..513c410dd3 100644 --- a/src/hooks/dhcp/high_availability/ha_messages.mes +++ b/src/hooks/dhcp/high_availability/ha_messages.mes @@ -481,9 +481,9 @@ This error message is issued to indicate that the configuration for the High Availability hooks library hasn't been specified. The 'high-availability' parameter must be specified for the hooks library to load properly. -% HA_PAUSE_CLIENT_LISTENER_FAILED Pausing mutli-threaded HTTP processing failed: %1 -This error message is emitted when an attempting to pause HA's HTTP client and -the listener threads. This error is highly unlikely and indicates a programmatic +% HA_PAUSE_CLIENT_LISTENER_FAILED Pausing multi-threaded HTTP processing failed: %1 +This error message is emitted when attempting to pause HA's HTTP client and +listener threads. This error is highly unlikely and indicates a programmatic issue that should be reported as a defect. % HA_RESET_COMMUNICATIONS_FAILED failed to send ha-reset command to %1: %2 @@ -501,9 +501,9 @@ This error message is issued to indicate that the ha-reset command handler failed while processing the command. The argument provides the reason for failure. -% HA_RESUME_CLIENT_LISTENER_FAILED Resuming mutli-threaded HTTP processing failed: %1 -This error message is emitted when an attempting to resume HA's HTTP client and -the listener threads. This is unlikely to occur and indicates a programmatic +% HA_RESUME_CLIENT_LISTENER_FAILED Resuming multi-threaded HTTP processing failed: %1 +This error message is emitted when attempting to resume HA's HTTP client and +listener threads. This error is highly unlikely and indicates a programmatic issue that should be reported as a defect. % HA_SCOPES_HANDLER_FAILED ha-scopes command failed: %1 diff --git a/src/hooks/dhcp/high_availability/ha_service.h b/src/hooks/dhcp/high_availability/ha_service.h index 617f60dfed..1ff786349c 100644 --- a/src/hooks/dhcp/high_availability/ha_service.h +++ b/src/hooks/dhcp/high_availability/ha_service.h @@ -1004,7 +1004,7 @@ public: /// @brief Start the client and(or) listener instances. /// - /// When HA+Mt is enabled it starts the client's thread pool + /// When HA+MT is enabled it starts the client's thread pool /// and the dedicated listener thread pool, if the listener exists. /// It registers pauseClientAndListener() and resumeClientAndListener() /// as the MultiThreading critical section entry and exit callbacks, @@ -1030,7 +1030,7 @@ public: /// @brief Stop the client and(or) listener instances. /// /// It unregisters the MultiThreading critical section callbacks, - /// closes all connections, and the stops the thread pools for the client + /// closes all connections and stops the thread pools for the client /// and listener, if they exist. void stopClientAndListener(); diff --git a/src/lib/config/tests/cmd_http_listener_unittests.cc b/src/lib/config/tests/cmd_http_listener_unittests.cc index be700b3b71..32b4b5ecaf 100644 --- a/src/lib/config/tests/cmd_http_listener_unittests.cc +++ b/src/lib/config/tests/cmd_http_listener_unittests.cc @@ -738,7 +738,7 @@ TEST_F(CmdHttpListenerTest, basics) { EXPECT_EQ(listener_->getPort(), port); EXPECT_EQ(listener_->getThreadPoolSize(), 1); - // It should not have an IOService, should not be listening + // It should not have an IOService, should not be listening and // should have no threads. ASSERT_FALSE(listener_->getThreadIOService()); EXPECT_TRUE(listener_->isStopped()); @@ -763,7 +763,6 @@ TEST_F(CmdHttpListenerTest, basics) { EXPECT_EQ(listener_->getThreadCount(), 1); ASSERT_TRUE(listener_->getThreadIOService()); EXPECT_FALSE(listener_->getThreadIOService()->stopped()); - EXPECT_TRUE(listener_->isRunning()); // Trying to start it again should fail. ASSERT_THROW_MSG(listener_->start(), InvalidOperation, @@ -773,7 +772,6 @@ TEST_F(CmdHttpListenerTest, basics) { ASSERT_NO_THROW_LOG(listener_->stop()); ASSERT_TRUE(listener_->isStopped()); EXPECT_EQ(listener_->getThreadCount(), 0); - EXPECT_TRUE(listener_->isStopped()); ASSERT_FALSE(listener_->getThreadIOService()); // Make sure we can call stop again without problems. @@ -795,6 +793,7 @@ TEST_F(CmdHttpListenerTest, basics) { ASSERT_NO_THROW_LOG(listener_->start()); EXPECT_EQ(listener_->getAddress(), address); EXPECT_EQ(listener_->getPort(), port); + EXPECT_EQ(listener_->getThreadCount(), 4); EXPECT_EQ(listener_->getThreadPoolSize(), 4); ASSERT_TRUE(listener_->isRunning()); ASSERT_TRUE(listener_->getThreadIOService()); @@ -805,6 +804,7 @@ TEST_F(CmdHttpListenerTest, basics) { ASSERT_NO_THROW_LOG(listener_->pause()); ASSERT_TRUE(listener_->isPaused()); EXPECT_EQ(listener_->getThreadCount(), 4); + EXPECT_EQ(listener_->getThreadPoolSize(), 4); ASSERT_TRUE(listener_->getThreadIOService()); EXPECT_TRUE(listener_->getThreadIOService()->stopped()); @@ -812,6 +812,7 @@ TEST_F(CmdHttpListenerTest, basics) { ASSERT_NO_THROW_LOG(listener_->resume()); ASSERT_TRUE(listener_->isRunning()); EXPECT_EQ(listener_->getThreadCount(), 4); + EXPECT_EQ(listener_->getThreadPoolSize(), 4); ASSERT_TRUE(listener_->getThreadIOService()); EXPECT_FALSE(listener_->getThreadIOService()->stopped()); @@ -819,6 +820,7 @@ TEST_F(CmdHttpListenerTest, basics) { ASSERT_NO_THROW_LOG(listener_->stop()); ASSERT_TRUE(listener_->isStopped()); EXPECT_EQ(listener_->getThreadCount(), 0); + EXPECT_EQ(listener_->getThreadPoolSize(), 4); ASSERT_FALSE(listener_->getThreadIOService()); EXPECT_TRUE(listener_->isStopped()); } diff --git a/src/lib/http/client.cc b/src/lib/http/client.cc index dcebd6012c..6067898ad3 100644 --- a/src/lib/http/client.cc +++ b/src/lib/http/client.cc @@ -1739,13 +1739,13 @@ public: /// @param io_service IOService that will drive connection IO in single /// threaded mode. (Currently ignored in multi-threaded mode) /// @param thread_pool_size maximum number of concurrent threads - /// Internally this also sets the maximum number concurrent connections + /// Internally this also sets the maximum number of concurrent connections /// per URL. - /// @param defer_thread_start When true, creation of the pool threads is + /// @param defer_thread_start When true, starting of the pool threads is /// deferred until a subsequent call to @ref start(). In this case the - /// pool's operational state post-construction is STOPPED. Otherwise, - /// the thread pool threads will be created and started, with the post- - /// construction state being RUNNING. Applicable only when thread-pool size + /// pool's operational state after construction is STOPPED. Otherwise, + /// the thread pool threads will be created and started, with the + /// operational state being RUNNING. Applicable only when thread-pool size /// is greater than zero. HttpClientImpl(IOService& io_service, size_t thread_pool_size = 0, bool defer_thread_start = false) diff --git a/src/lib/http/client.h b/src/lib/http/client.h index aeb5fc340c..34a0a23acc 100644 --- a/src/lib/http/client.h +++ b/src/lib/http/client.h @@ -136,8 +136,6 @@ public: /// /// @param io_service IO service to be used by the HTTP client. /// @param thread_pool_size maximum number of threads in the thread pool. - /// @param defer_thread_start if true, the thread pool will be created but - /// not started. Applicable only when thread-pool-size is greater than zero. /// A value greater than zero enables multi-threaded mode and sets the /// maximum number of concurrent connections per URL. A value of zero /// (default) enables single-threaded mode with one connection per URL. diff --git a/src/lib/http/http_thread_pool.h b/src/lib/http/http_thread_pool.h index e1d101728d..5a04a3d8d7 100644 --- a/src/lib/http/http_thread_pool.h +++ b/src/lib/http/http_thread_pool.h @@ -187,7 +187,7 @@ public: uint16_t getThreadCount() const; private: - /// @brief Maxim number of threads in the thread pool. + /// @brief Maximum number of threads in the thread pool. size_t pool_size_; /// @brief Pointer to private IOService used in multi-threaded mode. diff --git a/src/lib/http/tests/Makefile.am b/src/lib/http/tests/Makefile.am index 8fbc3397e7..663fee6f1c 100644 --- a/src/lib/http/tests/Makefile.am +++ b/src/lib/http/tests/Makefile.am @@ -48,7 +48,7 @@ libhttp_unittests_SOURCES += tls_client_unittests.cc endif libhttp_unittests_SOURCES += url_unittests.cc libhttp_unittests_SOURCES += test_http_client.h -libhttp_unittests_SOURCES += mt_client_unittests.cc +libhttp_unittests_SOURCES += client_mt_unittests.cc libhttp_unittests_SOURCES += http_thread_pool_unittests.cc libhttp_unittests_CPPFLAGS = $(AM_CPPFLAGS) $(GTEST_INCLUDES) diff --git a/src/lib/http/tests/mt_client_unittests.cc b/src/lib/http/tests/client_mt_unittests.cc similarity index 99% rename from src/lib/http/tests/mt_client_unittests.cc rename to src/lib/http/tests/client_mt_unittests.cc index 278d69408a..08850688ec 100644 --- a/src/lib/http/tests/mt_client_unittests.cc +++ b/src/lib/http/tests/client_mt_unittests.cc @@ -937,6 +937,8 @@ TEST_F(MtHttpClientTest, restartAfterStop) { // Starting again should succeed. ASSERT_NO_THROW_LOG(client->start()); + + // Verify we didn't break it. ASSERT_EQ(client->getThreadCount(), 3); ASSERT_TRUE(client->getThreadIOService()); ASSERT_FALSE(client->getThreadIOService()->stopped()); diff --git a/src/lib/http/tests/http_thread_pool_unittests.cc b/src/lib/http/tests/http_thread_pool_unittests.cc index c3958c9734..fcc088bd77 100644 --- a/src/lib/http/tests/http_thread_pool_unittests.cc +++ b/src/lib/http/tests/http_thread_pool_unittests.cc @@ -57,9 +57,9 @@ TEST_F(HttpThreadPoolTest, deferredStartConstruction) { ASSERT_NO_THROW_LOG(pool.reset(new HttpThreadPool(io_service_, 3, true))); // State should be stopped. - // Pool size should be 3 + // Pool size should be 3. // IOService should be there. - // IOService is new, so it should not be stopped, + // IOService is new, so it should not be stopped. // No threads in the pool. ASSERT_TRUE(pool->isStopped()); EXPECT_EQ(pool->getPoolSize(), 3); @@ -77,8 +77,11 @@ TEST_F(HttpThreadPoolTest, startDuringConstruction) { ASSERT_NO_THROW_LOG(pool.reset(new HttpThreadPool(io_service_, 3))); - // Pool size should be 3, state should be RUNNING, IOService should - // set but not stopped, and we should have 3 threads in the pool. + // State should be running. + // Pool size should be 3. + // IOService should be there. + // IOService is new, so it should not be stopped. + // Should have 3 threads in the pool. ASSERT_TRUE(pool->isRunning()); EXPECT_EQ(pool->getPoolSize(), 3); ASSERT_TRUE(pool->getIOService()); diff --git a/src/lib/util/multi_threading_mgr.cc b/src/lib/util/multi_threading_mgr.cc index 608e01ed85..2beb3833e2 100644 --- a/src/lib/util/multi_threading_mgr.cc +++ b/src/lib/util/multi_threading_mgr.cc @@ -125,7 +125,7 @@ MultiThreadingMgr::stopProcessing() { thread_pool_.stop(); } - for (auto cb : critical_entry_cbs_.getCallbacks() ) { + for (const auto& cb : critical_entry_cbs_.getCallbacks()) { try { (cb.callback_)(); } catch (...) { @@ -144,8 +144,8 @@ MultiThreadingMgr::startProcessing() { thread_pool_.start(getThreadPoolSize()); } - for (auto cb : critical_exit_cbs_.getCallbacks() ) { - try { + for (const auto& cb : critical_exit_cbs_.getCallbacks()) { + try { (cb.callback_)(); } catch (...) { // We can't log it and throwing could be chaos. diff --git a/src/lib/util/multi_threading_mgr.h b/src/lib/util/multi_threading_mgr.h index 0a0c2d4ae1..bb67765382 100644 --- a/src/lib/util/multi_threading_mgr.h +++ b/src/lib/util/multi_threading_mgr.h @@ -139,7 +139,7 @@ public: /// @param entry_cb Callback to invoke upon CriticalSection entry. Cannot be /// empty. /// @param exit_cb Callback to invoke upon CriticalSection exit. Cannot be - /// be empty. + /// empty. void addCriticalSectionCallbacks(const std::string& name, const NamedCallback::Callback& entry_cb, const NamedCallback::Callback& exit_cb); diff --git a/src/lib/util/named_callback.cc b/src/lib/util/named_callback.cc index 9ad5d7f059..c982caf8fe 100644 --- a/src/lib/util/named_callback.cc +++ b/src/lib/util/named_callback.cc @@ -19,7 +19,7 @@ void NamedCallbackList::addCallback(const std::string& name, const NamedCallback::Callback& cb) { if (!cb) { isc_throw(BadValue, "NamedCallbackList - callback: " << name - << ", cannot be empty"); + << ", cannot be empty"); } for (auto const& callback : callbacks_) { @@ -34,13 +34,11 @@ NamedCallbackList::addCallback(const std::string& name, const NamedCallback::Cal void NamedCallbackList::removeCallback(const std::string& name) { - for (auto it = callbacks_.begin(); it != callbacks_.end(); ) { + for (auto it = callbacks_.begin(); it != callbacks_.end(); ++it) { if ((*it).name_ == name) { - it = callbacks_.erase(it); + callbacks_.erase(it); break; } - - ++it; } } diff --git a/src/lib/util/named_callback.h b/src/lib/util/named_callback.h index 13a1ba6336..dcedde1e5c 100644 --- a/src/lib/util/named_callback.h +++ b/src/lib/util/named_callback.h @@ -27,13 +27,12 @@ struct NamedCallback { /// @param name Name by which the callback can be found. /// @param cb Callback associated with name. NamedCallback(const std::string& name, const Callback& cb) - : name_(name), callback_(cb) { - }; + : name_(name), callback_(cb) {} - /// @Brief Name by which the callback can be found. + /// @brief Name by which the callback can be found. std::string name_; - /// @Brief Callback associated with name. + /// @brief Callback associated with name. Callback callback_; }; @@ -46,7 +45,7 @@ struct NamedCallback { class NamedCallbackList { public: /// @brief Constructor. - NamedCallbackList(){}; + NamedCallbackList() {} /// @brief Adds a callback to the list. /// @@ -70,7 +69,7 @@ public: private: /// @brief The list of callbacks. - std::list callbacks_; + std::list callbacks_; }; } // namespace util diff --git a/src/lib/util/tests/multi_threading_mgr_unittest.cc b/src/lib/util/tests/multi_threading_mgr_unittest.cc index 3ff29b3719..3328445fb5 100644 --- a/src/lib/util/tests/multi_threading_mgr_unittest.cc +++ b/src/lib/util/tests/multi_threading_mgr_unittest.cc @@ -326,25 +326,25 @@ TEST(MultiThreadingMgrTest, criticalSection) { /// @brief Test fixture for exercised CriticalSection callbacks. class CriticalSectionCallbackTest : public ::testing::Test { public: - /// @Brief Constructor. - CriticalSectionCallbackTest() {}; + /// @brief Constructor. + CriticalSectionCallbackTest() {} - /// @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); } @@ -357,16 +357,16 @@ public: } /// @brief Checks callback invocations over a series of nested - /// CriticalSecitons. + /// CriticalSections. /// /// @param entries A vector of the invocation values that should /// be present after entry into the outermost CriticalSection. The - /// expected values should be in the order the callbacks were - /// added to the MultiThreadingMgr's list of callbacks. + /// expected values should be in the order the callbacks were added + /// to the MultiThreadingMgr's list of callbacks. /// @param exits A vector of the invocation values that should - /// be present after exiting the CriticalSection. The expected - /// values should be in the order the callbacks were added to the - /// MultiThreadingMgr's list of callbacks. + /// 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 entries, std::vectorexits) { // Pool must be running. ASSERT_TRUE(isThreadPoolRunning()); @@ -384,7 +384,7 @@ public: if (entries.size()) { // We expect entry invocations. - ASSERT_TRUE(invocations_ == entries); + ASSERT_TRUE(invocations_ == entries); } else { // We do not expect entry invocations. ASSERT_FALSE(invocations_.size()); @@ -397,14 +397,14 @@ public: // Enter another CriticalSection. MultiThreadingCriticalSection inner_cs; - // thread pool should still be stopped + // Thread pool should still be stopped ASSERT_FALSE(isThreadPoolRunning()); // We should not have had any callback invocations. ASSERT_FALSE(invocations_.size()); } - // After exiting inner setion, the thread pool should + // After exiting inner section, the thread pool should // still be stopped ASSERT_FALSE(isThreadPoolRunning()); @@ -412,20 +412,20 @@ public: ASSERT_FALSE(invocations_.size()); } - // After exiting the outer setion, the thread pool should + // After exiting the outer section, the thread pool should // match the thread count. ASSERT_TRUE(isThreadPoolRunning()); if (exits.size()) { // We expect exit invocations. - ASSERT_TRUE(invocations_ == exits); + ASSERT_TRUE(invocations_ == exits); } else { // We do not expect exit invocations. ASSERT_FALSE(invocations_.size()); } } - /// @Brief A list of values set by callback invocations. + /// @brief A list of values set by callback invocations. std::vector invocations_; }; diff --git a/src/lib/util/tests/named_callback_unittest.cc b/src/lib/util/tests/named_callback_unittest.cc index 7de913510c..f79707cc80 100644 --- a/src/lib/util/tests/named_callback_unittest.cc +++ b/src/lib/util/tests/named_callback_unittest.cc @@ -20,28 +20,36 @@ namespace { class NamedCallbackListTest : public ::testing::Test { public: - NamedCallbackListTest() {}; + /// @brief Constructor. + NamedCallbackListTest() {} + /// @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. void two() { invocations_.push_back(2); } + /// @brief A callback that adds the value, 3, to invocations lists. void three() { invocations_.push_back(3); } + /// @brief Run all callbacks. void runCallbacks() { invocations_.clear(); - for (auto cb : cbs_.getCallbacks() ) { + for (const auto& cb : cbs_.getCallbacks()) { ASSERT_NO_THROW((cb.callback_)()); } } + /// @brief A list of callbacks. NamedCallbackList cbs_; + + /// @brief A list of values set by callback invocations. std::vector invocations_; }; @@ -63,15 +71,6 @@ TEST_F(NamedCallbackListTest, basics) { std::bind(&NamedCallbackListTest::one, this)), BadValue, "NamedCallbackList - callback: one, already exists"); - for (auto cb : cbs_.getCallbacks() ) { - ASSERT_NO_THROW((cb.callback_)()); - } - - int i = 0; - for (auto invocation : invocations_) { - EXPECT_EQ(invocation, ++i); - } - runCallbacks(); EXPECT_EQ(3, invocations_.size());