From: Thomas Markwalder Date: Wed, 19 May 2021 17:58:45 +0000 (-0400) Subject: [#1818] Refactored CriticalSection callbacks X-Git-Tag: Kea-1.9.8~52 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=7e53ff36b30b67f64980e5dad5e5bee968dcd269;p=thirdparty%2Fkea.git [#1818] Refactored CriticalSection callbacks util::NamedCallback replaced with CSCallbackPair and absorbed into multi_threaded_mgr.*. This replaces two discrete lists of callbacks with a single list of named, callback pairs. --- diff --git a/src/lib/util/Makefile.am b/src/lib/util/Makefile.am index 85d43d75fd..b5dbe823ed 100644 --- a/src/lib/util/Makefile.am +++ b/src/lib/util/Makefile.am @@ -18,7 +18,6 @@ libkea_util_la_SOURCES += labeled_value.h labeled_value.cc libkea_util_la_SOURCES += memory_segment.h libkea_util_la_SOURCES += memory_segment_local.h memory_segment_local.cc libkea_util_la_SOURCES += multi_threading_mgr.h multi_threading_mgr.cc -libkea_util_la_SOURCES += named_callback.h named_callback.cc libkea_util_la_SOURCES += optional.h libkea_util_la_SOURCES += pid_file.h pid_file.cc libkea_util_la_SOURCES += pointer_util.h @@ -67,7 +66,6 @@ libkea_util_include_HEADERS = \ memory_segment.h \ memory_segment_local.h \ multi_threading_mgr.h \ - named_callback.h \ optional.h \ pid_file.h \ pointer_util.h \ diff --git a/src/lib/util/multi_threading_mgr.cc b/src/lib/util/multi_threading_mgr.cc index 2beb3833e2..4b2c2d3df7 100644 --- a/src/lib/util/multi_threading_mgr.cc +++ b/src/lib/util/multi_threading_mgr.cc @@ -125,9 +125,9 @@ MultiThreadingMgr::stopProcessing() { thread_pool_.stop(); } - for (const auto& cb : critical_entry_cbs_.getCallbacks()) { + for (const auto& cb : cs_callbacks_.getCallbackPairs()) { try { - (cb.callback_)(); + (cb.entry_cb_)(); } catch (...) { // We can't log it and throwing could be chaos. // We'll swallow it and tell people their callbacks @@ -144,9 +144,9 @@ MultiThreadingMgr::startProcessing() { thread_pool_.start(getThreadPoolSize()); } - for (const auto& cb : critical_exit_cbs_.getCallbacks()) { + for (const auto& cb : cs_callbacks_.getCallbackPairs()) { try { - (cb.callback_)(); + (cb.exit_cb_)(); } catch (...) { // We can't log it and throwing could be chaos. // We'll swallow it and tell people their callbacks @@ -158,22 +158,19 @@ MultiThreadingMgr::startProcessing() { void MultiThreadingMgr::addCriticalSectionCallbacks(const std::string& name, - const NamedCallback::Callback& entry_cb, - const NamedCallback::Callback& exit_cb) { - critical_entry_cbs_.addCallback(name, entry_cb); - critical_exit_cbs_.addCallback(name, exit_cb); + const CSCallbackPair::Callback& entry_cb, + const CSCallbackPair::Callback& exit_cb) { + cs_callbacks_.addCallbackPair(name, entry_cb, exit_cb); } void MultiThreadingMgr::removeCriticalSectionCallbacks(const std::string& name) { - critical_entry_cbs_.removeCallback(name); - critical_exit_cbs_.removeCallback(name); + cs_callbacks_.removeCallbackPair(name); } void MultiThreadingMgr::removeAllCriticalSectionCallbacks() { - critical_entry_cbs_.removeAll(); - critical_exit_cbs_.removeAll(); + cs_callbacks_.removeAll(); } MultiThreadingCriticalSection::MultiThreadingCriticalSection() { @@ -184,5 +181,53 @@ MultiThreadingCriticalSection::~MultiThreadingCriticalSection() { MultiThreadingMgr::instance().exitCriticalSection(); } +void +CSCallbackPairList::addCallbackPair(const std::string& name, + const CSCallbackPair::Callback& entry_cb, + const CSCallbackPair::Callback& exit_cb) { + if (name.empty()) { + isc_throw(BadValue, "CSCallbackPairList - name cannot be empty"); + } + + if (!entry_cb) { + isc_throw(BadValue, "CSCallbackPairList - entry callback for " << name + << " cannot be empty"); + } + + if (!exit_cb) { + isc_throw(BadValue, "CSCallbackPairList - exit callback for " << name + << " cannot be empty"); + } + + for (auto const& callback : cb_pairs_) { + if (callback.name_ == name) { + isc_throw(BadValue, "CSCallbackPairList - callbacks for " << name + << " already exist"); + } + } + + cb_pairs_.push_back(CSCallbackPair(name, entry_cb, exit_cb)); +} + +void +CSCallbackPairList::removeCallbackPair(const std::string& name) { + for (auto it = cb_pairs_.begin(); it != cb_pairs_.end(); ++it) { + if ((*it).name_ == name) { + cb_pairs_.erase(it); + break; + } + } +} + +void +CSCallbackPairList::removeAll() { + cb_pairs_.clear(); +} + +const std::list& +CSCallbackPairList::getCallbackPairs() { + return (cb_pairs_); +} + } // namespace util } // namespace isc diff --git a/src/lib/util/multi_threading_mgr.h b/src/lib/util/multi_threading_mgr.h index bb67765382..f6230df70f 100644 --- a/src/lib/util/multi_threading_mgr.h +++ b/src/lib/util/multi_threading_mgr.h @@ -8,7 +8,8 @@ #define MULTI_THREADING_MGR_H #include -#include +#include +#include #include @@ -17,6 +18,77 @@ namespace isc { 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. +struct CSCallbackPair { + /// @brief Defines a callback as a simple void() functor. + typedef std::function Callback; + + /// @brief Constructor + /// + /// @param name Name by which the callbacks can be found. + /// @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) {} + + /// @brief Name by which the callback can be found. + std::string name_; + + /// @brief Entry point callback associated with name. + Callback entry_cb_; + + /// @brief Exit point callback associated with name. + Callback exit_cb_; +}; + +/// @brief Maintains list of unique CSCallbackPairs. +/// +/// The list emphasizes iteration order and speed over +/// retrieval by name. When iterating over the list of +/// callback pairs, they are returned in the order they were +/// added, not by name. +class CSCallbackPairList { +public: + /// @brief Constructor. + CSCallbackPairList() {} + + /// @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. + /// + /// @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& entry_cb, + const CSCallbackPair::Callback& exit_cb); + + /// @brief Removes a callback pair from the list. + /// + /// @param name Name of the callback to remove. + /// If no such callback exists, it simply returns. + void removeCallbackPair(const std::string& name); + + /// @brief Removes all callbacks from the list. + void removeAll(); + + /// @brief Fetches the list of callbacks pairs. + const std::list& getCallbackPairs(); + +private: + /// @brief The list of callback pairs. + std::list cb_pairs_; +}; + /// @brief Multi Threading Manager. /// /// This singleton class holds the multi-threading mode. @@ -141,8 +213,8 @@ public: /// @param exit_cb Callback to invoke upon CriticalSection exit. Cannot be /// empty. void addCriticalSectionCallbacks(const std::string& name, - const NamedCallback::Callback& entry_cb, - const NamedCallback::Callback& exit_cb); + const CSCallbackPair::Callback& entry_cb, + const CSCallbackPair::Callback& exit_cb); /// @brief Removes the set of callbacks associated with a given name /// from the list of CriticalSection callbacks. @@ -208,11 +280,8 @@ private: /// @brief Packet processing thread pool. ThreadPool> thread_pool_; - /// @brief List of callbacks to invoke upon CriticalSection entry. - NamedCallbackList critical_entry_cbs_; - - /// @brief List of callbacks to invoke upon CriticalSection exit. - NamedCallbackList critical_exit_cbs_; + /// @brief List of CriticalSection entry and exit callback pairs. + CSCallbackPairList cs_callbacks_; }; /// @note: everything here MUST be used ONLY from the main thread. diff --git a/src/lib/util/named_callback.cc b/src/lib/util/named_callback.cc deleted file mode 100644 index c982caf8fe..0000000000 --- a/src/lib/util/named_callback.cc +++ /dev/null @@ -1,56 +0,0 @@ -// Copyright (C) 2021 Internet Systems Consortium, Inc. ("ISC") -// -// This Source Code Form is subject to the terms of the Mozilla Public -// License, v. 2.0. If a copy of the MPL was not distributed with this -// file, You can obtain one at http://mozilla.org/MPL/2.0/. - -#include - -#include -#include - -#include -#include - -namespace isc { -namespace util { - -void -NamedCallbackList::addCallback(const std::string& name, const NamedCallback::Callback& cb) { - if (!cb) { - isc_throw(BadValue, "NamedCallbackList - callback: " << name - << ", cannot be empty"); - } - - for (auto const& callback : callbacks_) { - if (callback.name_ == name) { - isc_throw(BadValue, "NamedCallbackList - callback: " << name - << ", already exists"); - } - } - - callbacks_.push_back(NamedCallback(name, cb)); -} - -void -NamedCallbackList::removeCallback(const std::string& name) { - for (auto it = callbacks_.begin(); it != callbacks_.end(); ++it) { - if ((*it).name_ == name) { - callbacks_.erase(it); - break; - } - } -} - -void -NamedCallbackList::removeAll() { - callbacks_.clear(); -} - -const std::list& -NamedCallbackList::getCallbacks() { - return (callbacks_); -} - -} // namespace util -} // namespace isc diff --git a/src/lib/util/named_callback.h b/src/lib/util/named_callback.h deleted file mode 100644 index dcedde1e5c..0000000000 --- a/src/lib/util/named_callback.h +++ /dev/null @@ -1,78 +0,0 @@ -// Copyright (C) 2021 Internet Systems Consortium, Inc. ("ISC") -// -// This Source Code Form is subject to the terms of the Mozilla Public -// License, v. 2.0. If a copy of the MPL was not distributed with this -// file, You can obtain one at http://mozilla.org/MPL/2.0/. - -#ifndef NAMED_CALLBACK_H -#define NAMED_CALLBACK_H - -#include -#include -#include - -#include -#include - -namespace isc { -namespace util { - -/// @brief Associates a Callback with a name. -struct NamedCallback { - /// @brief Defines a callback as a simple void() functor. - typedef std::function Callback; - - /// @brief Constructor - /// - /// @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) {} - - /// @brief Name by which the callback can be found. - std::string name_; - - /// @brief Callback associated with name. - Callback callback_; -}; - -/// @brief Maintains list of unique NamedCallbacks. -/// -/// The list emphasizes iteration order and speed over -/// retrieval by name. When iterating over the list of -/// callbacks, they are returned in the order they were -/// added, not by name. -class NamedCallbackList { -public: - /// @brief Constructor. - NamedCallbackList() {} - - /// @brief Adds a callback to the list. - /// - /// @param name Name of the callback to add. - /// @param cb Callback to add. - /// - /// @throw BadValue if the name is already in the list. - void addCallback(const std::string& name, const NamedCallback::Callback& cb); - - /// @brief Removes a callback from the list. - /// - /// @param name Name of the callback to remove. - /// If no such callback exists, it simply returns. - void removeCallback(const std::string& name); - - /// @brief Removes all callbacks from the list. - void removeAll(); - - /// @brief Fetches the list of callbacks. - const std::list& getCallbacks(); - -private: - /// @brief The list of callbacks. - std::list callbacks_; -}; - -} // namespace util -} // namespace isc - -#endif // NAMED_CALLBACK_H diff --git a/src/lib/util/tests/Makefile.am b/src/lib/util/tests/Makefile.am index dfde69ccfe..d8354e967b 100644 --- a/src/lib/util/tests/Makefile.am +++ b/src/lib/util/tests/Makefile.am @@ -42,7 +42,6 @@ run_unittests_SOURCES += memory_segment_local_unittest.cc run_unittests_SOURCES += memory_segment_common_unittest.h run_unittests_SOURCES += memory_segment_common_unittest.cc run_unittests_SOURCES += multi_threading_mgr_unittest.cc -run_unittests_SOURCES += named_callback_unittest.cc run_unittests_SOURCES += optional_unittest.cc run_unittests_SOURCES += pid_file_unittest.cc run_unittests_SOURCES += qid_gen_unittest.cc diff --git a/src/lib/util/tests/multi_threading_mgr_unittest.cc b/src/lib/util/tests/multi_threading_mgr_unittest.cc index 3328445fb5..a829d288e9 100644 --- a/src/lib/util/tests/multi_threading_mgr_unittest.cc +++ b/src/lib/util/tests/multi_threading_mgr_unittest.cc @@ -8,6 +8,7 @@ #include #include +#include #include @@ -384,6 +385,12 @@ public: if (entries.size()) { // We expect entry invocations. + ASSERT_EQ(invocations_.size(), entries.size()); + for (auto val :invocations_) { + std::cout << val << " "; + } + std::cout << std::endl; + ASSERT_TRUE(invocations_ == entries); } else { // We do not expect entry invocations. @@ -397,7 +404,7 @@ 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. @@ -405,7 +412,7 @@ public: } // After exiting inner section, the thread pool should - // still be stopped + // still be stopped. ASSERT_FALSE(isThreadPoolRunning()); // We should not have had more callback invocations. @@ -429,8 +436,45 @@ public: std::vector invocations_; }; +/// @brief Verifies that critical section callback maintenance. +/// Catch invalid pairs, add pairs, remover pairs. +TEST_F(CriticalSectionCallbackTest, addAndRemove) { + auto& mgr = MultiThreadingMgr::instance(); + + // Cannot add with a blank name. + 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, [](){}), + BadValue, "CSCallbackPairList - entry callback for bad cannot be empty"); + + // Cannot add with an empty exit callback. + 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", [](){}, [](){})); + + // Should not be able to add foo twice. + 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", [](){}, [](){})); + + // Should be able to remove foo. + ASSERT_NO_THROW_LOG(mgr.removeCriticalSectionCallbacks("foo")); + + // Should be able to remove foo twice without issue. + ASSERT_NO_THROW_LOG(mgr.removeCriticalSectionCallbacks("foo")); + + // Should be able to remove all without issue. + ASSERT_NO_THROW_LOG(mgr.removeAllCriticalSectionCallbacks()); +} + /// @brief Verifies that the critical section callbacks work. -TEST_F(CriticalSectionCallbackTest, basics) { +TEST_F(CriticalSectionCallbackTest, invocations) { // get the thread pool instance auto& thread_pool = MultiThreadingMgr::instance().getThreadPool(); // thread pool should be stopped diff --git a/src/lib/util/tests/named_callback_unittest.cc b/src/lib/util/tests/named_callback_unittest.cc deleted file mode 100644 index f79707cc80..0000000000 --- a/src/lib/util/tests/named_callback_unittest.cc +++ /dev/null @@ -1,94 +0,0 @@ -// Copyright (C) 2021 Internet Systems Consortium, Inc. ("ISC") -// -// This Source Code Form is subject to the terms of the Mozilla Public -// License, v. 2.0. If a copy of the MPL was not distributed with this -// file, You can obtain one at http://mozilla.org/MPL/2.0/. - -#include - -#include - -#include -#include -#include - -using namespace isc; -using namespace isc::util; -using namespace std; - -namespace { - -class NamedCallbackListTest : public ::testing::Test { -public: - /// @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 (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_; -}; - -TEST_F(NamedCallbackListTest, basics) { - - ASSERT_NO_THROW(cbs_.addCallback("one", - std::bind(&NamedCallbackListTest::one, this))); - ASSERT_NO_THROW(cbs_.addCallback("two", - std::bind(&NamedCallbackListTest::two, this))); - ASSERT_NO_THROW(cbs_.addCallback("three", - std::bind(&NamedCallbackListTest::three, this))); - - // Can't add an empty callback. - ASSERT_THROW_MSG(cbs_.addCallback("nadda", 0), - BadValue, "NamedCallbackList - callback: nadda, cannot be empty"); - - // Can't add one twice. - ASSERT_THROW_MSG(cbs_.addCallback("one", - std::bind(&NamedCallbackListTest::one, this)), - BadValue, "NamedCallbackList - callback: one, already exists"); - - runCallbacks(); - - EXPECT_EQ(3, invocations_.size()); - EXPECT_EQ(1, invocations_[0]); - EXPECT_EQ(2, invocations_[1]); - EXPECT_EQ(3, invocations_[2]); - - // Removing two shouldn't throw. - ASSERT_NO_THROW(cbs_.removeCallback("two")); - - // Removing it again shouldn't throw. - ASSERT_NO_THROW(cbs_.removeCallback("two")); - - runCallbacks(); - - EXPECT_EQ(2, invocations_.size()); - EXPECT_EQ(1, invocations_[0]); - EXPECT_EQ(3, invocations_[1]); -} - -} // namespace