From: Razvan Becheriu Date: Mon, 7 Dec 2020 13:04:41 +0000 (+0200) Subject: [#1375] implemented thread safe TimerMgr X-Git-Tag: Kea-1.9.3~107 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=e865206822ead8d1c99459907e0dd9eba0cdead2;p=thirdparty%2Fkea.git [#1375] implemented thread safe TimerMgr --- diff --git a/src/lib/dhcpsrv/tests/timer_mgr_unittest.cc b/src/lib/dhcpsrv/tests/timer_mgr_unittest.cc index bdcc27684d..00583740c7 100644 --- a/src/lib/dhcpsrv/tests/timer_mgr_unittest.cc +++ b/src/lib/dhcpsrv/tests/timer_mgr_unittest.cc @@ -10,6 +10,7 @@ #include #include #include +#include #include @@ -20,6 +21,7 @@ using namespace isc; using namespace isc::dhcp; using namespace isc::asiolink; +using namespace isc::test; namespace { @@ -84,6 +86,29 @@ public: /// @c timeout_ member. void timeoutCallback(); + // @brief This test checks that certain errors are returned when invalid + // parameters are specified when registering a timer, or when + // the registration can't be made. + void testRegisterTimer(); + + /// @brief This test verifies that it is possible to unregister a timer from + /// the TimerMgr. + void testUnregisterTimer(); + + /// @brief This test verifies that it is possible to unregister all timers. + void testUnregisterTimers(); + + /// @brief This test verifies that the timer execution can be cancelled. + void testCancel(); + + /// @brief This test verifies that the callbacks for the scheduled timers + /// are actually called. + void testScheduleTimers(); + + /// @brief This test verifies that exceptions emitted from the callback + /// would be handled by the TimerMgr. + void testCallbackWithException(); + /// @brief Type definition for a map holding calls counters for /// timers. typedef std::map CallsCount; @@ -167,7 +192,8 @@ TimerMgrTest::makeCallbackWithException() { // This test checks that certain errors are returned when invalid // parameters are specified when registering a timer, or when // the registration can't be made. -TEST_F(TimerMgrTest, registerTimer) { +void +TimerMgrTest::testRegisterTimer() { // Empty timer name is not allowed. ASSERT_THROW(timer_mgr_->registerTimer("", makeCallback("timer1"), 1, IntervalTimer::ONE_SHOT), @@ -185,9 +211,20 @@ TEST_F(TimerMgrTest, registerTimer) { BadValue); } -// This test verifies that it is possible to unregister a timer from -// the TimerMgr. -TEST_F(TimerMgrTest, unregisterTimer) { +TEST_F(TimerMgrTest, registerTimer) { + // Disable Multi-Threading. + MultiThreadingTest mt(false); + testRegisterTimer(); +} + +TEST_F(TimerMgrTest, registerTimerMultiThreading) { + // Enable Multi-Threading. + MultiThreadingTest mt(true); + testRegisterTimer(); +} + +void +TimerMgrTest::testUnregisterTimer() { // Register a timer and start it. ASSERT_NO_FATAL_FAILURE(registerTimer("timer1", 1)); ASSERT_EQ(1, timer_mgr_->timersCount()); @@ -218,8 +255,20 @@ TEST_F(TimerMgrTest, unregisterTimer) { EXPECT_EQ(calls_count_["timer1"], calls_count); } -// This test verifies taht it is possible to unregister all timers. -TEST_F(TimerMgrTest, unregisterTimers) { +TEST_F(TimerMgrTest, unregisterTimer) { + // Disable Multi-Threading. + MultiThreadingTest mt(false); + testUnregisterTimer(); +} + +TEST_F(TimerMgrTest, unregisterTimerMultiThreading) { + // Enable Multi-Threading. + MultiThreadingTest mt(true); + testUnregisterTimer(); +} + +void +TimerMgrTest::testUnregisterTimers() { // Register 10 timers. for (int i = 1; i <= 20; ++i) { std::ostringstream s; @@ -262,8 +311,20 @@ TEST_F(TimerMgrTest, unregisterTimers) { EXPECT_TRUE(calls_count == calls_count_); } -// This test verifies that the timer execution can be cancelled. -TEST_F(TimerMgrTest, cancel) { +TEST_F(TimerMgrTest, unregisterTimers) { + // Disable Multi-Threading. + MultiThreadingTest mt(false); + testUnregisterTimers(); +} + +TEST_F(TimerMgrTest, unregisterTimersMultiThreading) { + // Enable Multi-Threading. + MultiThreadingTest mt(true); + testUnregisterTimers(); +} + +void +TimerMgrTest::testCancel() { // Register timer. ASSERT_NO_FATAL_FAILURE(registerTimer("timer1", 1)); @@ -299,9 +360,20 @@ TEST_F(TimerMgrTest, cancel) { EXPECT_GT(calls_count_["timer1"], calls_count); } -// This test verifies that the callbacks for the scheduled timers are -// actually called. -TEST_F(TimerMgrTest, scheduleTimers) { +TEST_F(TimerMgrTest, cancel) { + // Disable Multi-Threading. + MultiThreadingTest mt(false); + testCancel(); +} + +TEST_F(TimerMgrTest, cancelMultiThreading) { + // Enable Multi-Threading. + MultiThreadingTest mt(true); + testCancel(); +} + +void +TimerMgrTest::testScheduleTimers() { // Register two timers: 'timer1' and 'timer2'. The first timer will // be executed at the 50ms interval. The second one at the 100ms // interval. @@ -353,9 +425,20 @@ TEST_F(TimerMgrTest, scheduleTimers) { EXPECT_GT(calls_count_["timer2"], calls_count_timer2); } -// This test verifies that exceptions emitted from the callback would -// be handled by the TimerMgr. -TEST_F(TimerMgrTest, callbackWithException) { +TEST_F(TimerMgrTest, scheduleTimers) { + // Disable Multi-Threading. + MultiThreadingTest mt(false); + testScheduleTimers(); +} + +TEST_F(TimerMgrTest, scheduleTimersMultiThreading) { + // Enable Multi-Threading. + MultiThreadingTest mt(true); + testScheduleTimers(); +} + +void +TimerMgrTest::testCallbackWithException() { // Create timer which will trigger callback generating exception. ASSERT_NO_THROW( timer_mgr_->registerTimer("timer1", makeCallbackWithException(), 1, @@ -370,6 +453,18 @@ TEST_F(TimerMgrTest, callbackWithException) { doWait(500); } +TEST_F(TimerMgrTest, callbackWithException) { + // Disable Multi-Threading. + MultiThreadingTest mt(false); + testCallbackWithException(); +} + +TEST_F(TimerMgrTest, callbackWithExceptionMultiThreading) { + // Enable Multi-Threading. + MultiThreadingTest mt(true); + testCallbackWithException(); +} + // This test verifies that IO service specified for the TimerMgr // must not be null. TEST_F(TimerMgrTest, setIOService) { diff --git a/src/lib/dhcpsrv/timer_mgr.cc b/src/lib/dhcpsrv/timer_mgr.cc index 5ec4123654..cc5a879b89 100644 --- a/src/lib/dhcpsrv/timer_mgr.cc +++ b/src/lib/dhcpsrv/timer_mgr.cc @@ -10,12 +10,16 @@ #include #include #include +#include + +#include #include #include using namespace isc; using namespace isc::asiolink; +using namespace isc::util; namespace { @@ -67,7 +71,6 @@ typedef boost::shared_ptr TimerInfoPtr; /// @brief A type definition for the map holding timers configuration. typedef std::map TimerInfoMap; - /// @brief Implementation of the @c TimerMgr class TimerMgrImpl { public: @@ -146,6 +149,63 @@ public: private: + /// @name Internal methods called holding the mutex in multi threading + /// mode. + + /// @brief Registers new timer in the @c TimerMgr. + /// + /// @param timer_name Unique name for the timer. + /// @param callback Pointer to the callback function to be invoked + /// when the timer elapses, e.g. function processing expired leases + /// in the DHCP server. + /// @param interval Timer interval in milliseconds. + /// @param scheduling_mode Scheduling mode of the timer as described in + /// @c asiolink::IntervalTimer::Mode. + /// + /// @throw BadValue if the timer name is invalid or duplicate. + void registerTimerInternal(const std::string& timer_name, + const asiolink::IntervalTimer::Callback& callback, + const long interval, + const asiolink::IntervalTimer::Mode& scheduling_mode); + + + /// @brief Unregisters specified timer. + /// + /// This method cancels the timer if it is setup and removes the timer + /// from the internal collection of timers. + /// + /// @param timer_name Name of the timer to be unregistered. + /// + /// @throw BadValue if the specified timer hasn't been registered. + void unregisterTimerInternal(const std::string& timer_name); + + /// @brief Unregisters all timers. + /// + /// This method must be explicitly called prior to termination of the + /// process. + void unregisterTimersInternal(); + + /// @brief Schedules the execution of the interval timer. + /// + /// This method schedules the timer, i.e. the callback will be executed + /// after specified interval elapses. The interval has been specified + /// during timer registration. Depending on the mode selected during the + /// timer registration, the callback will be executed once after it has + /// been scheduled or until it is cancelled. Though, in the former case + /// the timer can be re-scheduled in the callback function. + /// + /// @param timer_name Unique timer name. + /// + /// @throw BadValue if the timer hasn't been registered. + void setupInternal(const std::string& timer_name); + + /// @brief Cancels the execution of the interval timer. + /// + /// @param timer_name Unique timer name. + /// + /// @throw BadValue if the timer hasn't been registered. + void cancelInternal(const std::string& timer_name); + /// @brief Callback function to be executed for each interval timer when /// its scheduled interval elapses. /// @@ -158,10 +218,13 @@ private: /// @brief Holds mapping of the timer name to timer instance and other /// parameters pertaining to the timer. TimerInfoMap registered_timers_; + + /// @brief The mutex to protect the timer manager. + boost::scoped_ptr mutex_; }; -TimerMgrImpl::TimerMgrImpl() : - io_service_(new IOService()), registered_timers_() { +TimerMgrImpl::TimerMgrImpl() : io_service_(new IOService()), + registered_timers_(), mutex_(new std::mutex) { } void @@ -169,6 +232,7 @@ TimerMgrImpl::setIOService(const IOServicePtr& io_service) { if (!io_service) { isc_throw(BadValue, "IO service object must not be null for TimerMgr"); } + io_service_ = io_service; } @@ -177,7 +241,19 @@ TimerMgrImpl::registerTimer(const std::string& timer_name, const IntervalTimer::Callback& callback, const long interval, const IntervalTimer::Mode& scheduling_mode) { + if (MultiThreadingMgr::instance().getMode()) { + std::lock_guard lock(*mutex_); + registerTimerInternal(timer_name, callback, interval, scheduling_mode); + } else { + registerTimerInternal(timer_name, callback, interval, scheduling_mode); + } +} +void +TimerMgrImpl::registerTimerInternal(const std::string& timer_name, + const IntervalTimer::Callback& callback, + const long interval, + const IntervalTimer::Mode& scheduling_mode) { // Timer name must not be empty. if (timer_name.empty()) { isc_throw(BadValue, "registered timer name must not be empty"); @@ -202,7 +278,16 @@ TimerMgrImpl::registerTimer(const std::string& timer_name, void TimerMgrImpl::unregisterTimer(const std::string& timer_name) { + if (MultiThreadingMgr::instance().getMode()) { + std::lock_guard lock(*mutex_); + unregisterTimerInternal(timer_name); + } else { + unregisterTimerInternal(timer_name); + } +} +void +TimerMgrImpl::unregisterTimerInternal(const std::string& timer_name) { // Find the timer with specified name. TimerInfoMap::iterator timer_info_it = registered_timers_.find(timer_name); @@ -213,7 +298,7 @@ TimerMgrImpl::unregisterTimer(const std::string& timer_name) { } // Cancel any pending asynchronous operation and stop the timer. - cancel(timer_name); + cancelInternal(timer_name); // Remove the timer. registered_timers_.erase(timer_info_it); @@ -221,6 +306,16 @@ TimerMgrImpl::unregisterTimer(const std::string& timer_name) { void TimerMgrImpl::unregisterTimers() { + if (MultiThreadingMgr::instance().getMode()) { + std::lock_guard lock(*mutex_); + unregisterTimersInternal(); + } else { + unregisterTimersInternal(); + } +} + +void +TimerMgrImpl::unregisterTimersInternal() { // Copy the map holding timers configuration. This is required so as // we don't cut the branch which we're sitting on when we will be // erasing the timers. We're going to iterate over the register timers @@ -235,23 +330,42 @@ TimerMgrImpl::unregisterTimers() { // Iterate over the existing timers and unregister them. for (TimerInfoMap::iterator timer_info_it = registered_timers_copy.begin(); timer_info_it != registered_timers_copy.end(); ++timer_info_it) { - unregisterTimer(timer_info_it->first); + unregisterTimerInternal(timer_info_it->first); } } bool TimerMgrImpl::isTimerRegistered(const std::string& timer_name) { - return (registered_timers_.find(timer_name) != registered_timers_.end()); + if (MultiThreadingMgr::instance().getMode()) { + std::lock_guard lock(*mutex_); + return (registered_timers_.find(timer_name) != registered_timers_.end()); + } else { + return (registered_timers_.find(timer_name) != registered_timers_.end()); + } } size_t TimerMgrImpl::timersCount() const { - return (registered_timers_.size()); + if (MultiThreadingMgr::instance().getMode()) { + std::lock_guard lock(*mutex_); + return (registered_timers_.size()); + } else { + return (registered_timers_.size()); + } } void TimerMgrImpl::setup(const std::string& timer_name) { + if (MultiThreadingMgr::instance().getMode()) { + std::lock_guard lock(*mutex_); + setupInternal(timer_name); + } else { + setupInternal(timer_name); + } +} +void +TimerMgrImpl::setupInternal(const std::string& timer_name) { // Check if the specified timer exists. TimerInfoMap::const_iterator timer_info_it = registered_timers_.find(timer_name); if (timer_info_it == registered_timers_.end()) { @@ -270,7 +384,16 @@ TimerMgrImpl::setup(const std::string& timer_name) { void TimerMgrImpl::cancel(const std::string& timer_name) { + if (MultiThreadingMgr::instance().getMode()) { + std::lock_guard lock(*mutex_); + cancelInternal(timer_name); + } else { + cancelInternal(timer_name); + } +} +void +TimerMgrImpl::cancelInternal(const std::string& timer_name) { // Find the timer of our interest. TimerInfoMap::const_iterator timer_info_it = registered_timers_.find(timer_name); if (timer_info_it == registered_timers_.end()) { @@ -326,7 +449,6 @@ TimerMgr::TimerMgr() TimerMgr::~TimerMgr() { impl_->unregisterTimers(); - delete impl_; } void @@ -397,6 +519,5 @@ TimerMgr::setIOService(const IOServicePtr& io_service) { impl_->setIOService(io_service); } - } // end of namespace isc::dhcp } // end of namespace isc diff --git a/src/lib/dhcpsrv/timer_mgr.h b/src/lib/dhcpsrv/timer_mgr.h index a03c8156ed..f0e014b428 100644 --- a/src/lib/dhcpsrv/timer_mgr.h +++ b/src/lib/dhcpsrv/timer_mgr.h @@ -18,6 +18,8 @@ namespace dhcp { /// @brief Forward declaration of the @c TimerMgr implementation. class TimerMgrImpl; +typedef boost::shared_ptr TimerMgrImplPtr; + /// @brief Forward declaration of the @c TimerMgr. class TimerMgr; @@ -151,9 +153,8 @@ private: /// construction via @c TimerMgr::instance. TimerMgr(); - /// @brief Pointer to @c TimerMgr implementation. - TimerMgrImpl* impl_; - + /// @brief The @c TimerMgr implementation. + TimerMgrImplPtr impl_; }; } // end of namespace isc::dhcp