From 65b5204084e6e39c8f507bc898b777a4b6313816 Mon Sep 17 00:00:00 2001 From: Marcin Siodelski Date: Wed, 2 Sep 2015 12:52:32 +0200 Subject: [PATCH] [3970] Prevent race conditions during timers registration. --- src/lib/dhcpsrv/tests/timer_mgr_unittest.cc | 23 ++++++++++++++++-- src/lib/dhcpsrv/timer_mgr.cc | 13 +++++----- src/lib/dhcpsrv/timer_mgr.h | 27 ++++++++++++++------- 3 files changed, 45 insertions(+), 18 deletions(-) diff --git a/src/lib/dhcpsrv/tests/timer_mgr_unittest.cc b/src/lib/dhcpsrv/tests/timer_mgr_unittest.cc index 473fd71b17..fadd2baffb 100644 --- a/src/lib/dhcpsrv/tests/timer_mgr_unittest.cc +++ b/src/lib/dhcpsrv/tests/timer_mgr_unittest.cc @@ -227,8 +227,8 @@ TEST_F(TimerMgrTest, deregisterTimer) { const unsigned int calls_count = calls_count_["timer1"]; ASSERT_GT(calls_count, 0); - // Check that an attempt to deregister a non-existing timerm would - // result in execption. + // Check that an attempt to deregister a non-existing timer would + // result in exeception. EXPECT_THROW(timer_mgr.deregisterTimer("timer2"), BadValue); // Now deregister the correct one. @@ -237,6 +237,7 @@ TEST_F(TimerMgrTest, deregisterTimer) { // Start the thread again and wait another 100ms. ASSERT_NO_THROW(timer_mgr.startThread()); doWait(100); + ASSERT_NO_THROW(timer_mgr.stopThread()); // The number of calls for the timer1 shouldn't change as the // timer had been deregistered. @@ -290,6 +291,24 @@ TEST_F(TimerMgrTest, deregisterTimers) { EXPECT_TRUE(calls_count == calls_count_); } +// This test checks that it is not possible to deregister timers +// while the thread is running. +TEST_F(TimerMgrTest, deregisterTimerWhileRunning) { + TimerMgr& timer_mgr = TimerMgr::instance(); + + // Register two timers. + ASSERT_NO_FATAL_FAILURE(registerTimer("timer1", 1)); + ASSERT_NO_FATAL_FAILURE(registerTimer("timer2", 1)); + + // Start the thread and make sure we can't deregister them. + ASSERT_NO_THROW(timer_mgr.startThread()); + EXPECT_THROW(timer_mgr.deregisterTimer("timer1"), InvalidOperation); + EXPECT_THROW(timer_mgr.deregisterTimers(), InvalidOperation); + + // No need to stop the thread as it will be stopped by the + // test fixture destructor. +} + // This test verifies that the timer execution can be cancelled. TEST_F(TimerMgrTest, cancel) { TimerMgr& timer_mgr = TimerMgr::instance(); diff --git a/src/lib/dhcpsrv/timer_mgr.cc b/src/lib/dhcpsrv/timer_mgr.cc index c54f614b8b..1d81e1ccf0 100644 --- a/src/lib/dhcpsrv/timer_mgr.cc +++ b/src/lib/dhcpsrv/timer_mgr.cc @@ -91,6 +91,11 @@ TimerMgr::registerTimer(const std::string& timer_name, void TimerMgr::deregisterTimer(const std::string& timer_name) { + if (thread_) { + isc_throw(InvalidOperation, "unable to deregister timer " + << timer_name << " while worker thread is running"); + } + // Find the timer with specified name. TimerInfoMap::iterator timer_info_it = registered_timers_.find(timer_name); @@ -178,7 +183,6 @@ void TimerMgr::stopThread() { // If thread is not running, this is no-op. if (thread_) { - std::cout << "stopThread" << std::endl; // Stop the IO Service. This will break the IOService::run executed in the // worker thread. The thread will now terminate. getIOService().post(boost::bind(&IOService::stop, &getIOService())); @@ -234,12 +238,9 @@ TimerMgr::watchSocketCallback(const std::string& timer_name, const bool mark_rea // from the worker thrad and will likely block the thread until the socket // is cleared. if (mark_ready) { - std::cout << "markReady" << std::endl; timer_info.watch_socket_->markReady(); - std::cout << "markReady ended" << std::endl; } else { - std::cout << "clearReady" << std::endl; // We're executing a callback function from the Interface Manager. // This callback function is executed when the call to select() is // interrupted as a result of receiving some data over the watch @@ -257,9 +258,7 @@ void TimerMgr::clearReadySockets() { for (TimerInfoMap::iterator timer_info_it = registered_timers_.begin(); timer_info_it != registered_timers_.end(); ++timer_info_it) { - { - timer_info_it->second.watch_socket_->clearReady(); - } + timer_info_it->second.watch_socket_->clearReady(); } } diff --git a/src/lib/dhcpsrv/timer_mgr.h b/src/lib/dhcpsrv/timer_mgr.h index c2c7289b0d..4aa14ea1c0 100644 --- a/src/lib/dhcpsrv/timer_mgr.h +++ b/src/lib/dhcpsrv/timer_mgr.h @@ -71,16 +71,24 @@ namespace dhcp { /// @c util::WatchSocket and marks it "ready". This call effectively /// writes the data to a socket (pipe) which interrupts the call /// to the @c select() function in the main thread. Note that this -/// operation will likely block the worker thread until the -/// socket is cleared. When the @c IfaceMgr (in the main thread) -/// detects data transmitted over the external socket it will -/// invoke a callback function associated with this socket. This -/// is the @c TimerMgr::ifaceMgrCallback associated with the socket -/// when the timer is registered. This callback function is executed -/// in the main thread. It clears the socket, which unblocks the -/// worker thread. It also invokes the user callback function specified +/// operation may block the worker thread until the socket is cleared. +/// When the @c IfaceMgr (in the main thread) detects data transmitted +/// over the external socket it will invoke a callback function +/// associated with this socket. This is the +/// @c TimerMgr::ifaceMgrCallback associated with the socket when the +/// timer is registered. This callback function is executed/ in the +/// main thread. It clears the socket, which unblocks the worker +/// thread. It also invokes the user callback function specified /// for a given timer. /// +/// The @c TimerMgr::timerCallback function searches for the +/// registered timer for which it has been called. This may cause +/// race conditions between the worker thread and the main thread +/// if the latter is modifying the collection of the registered +/// timers. Therefore, the @c TimerMgr does not allow for +/// registering or deregistering the timers when the worker thread +/// is running. The worker thread must be stopped first. +/// /// @warning The application (DHCP server) is responsible for /// deregistering the timers before it terminates: /// @code @@ -93,6 +101,8 @@ namespace dhcp { /// instance which may not exist at this point. If the timers are /// not deregistered before the application terminates this will /// likely result in segmentation fault on some systems. +/// +/// All timers are associated with the callback function class TimerMgr : public boost::noncopyable { public: @@ -297,7 +307,6 @@ private: /// the map. The timer is associated with an instance of the @c WatchSocket /// which is marked ready when the interval for the particular elapses. TimerInfoMap registered_timers_; - }; } // end of namespace isc::dhcp -- 2.47.2