From 2d7e2fec666b5c4545eff08054f4da2104a68aaf Mon Sep 17 00:00:00 2001 From: Marcin Siodelski Date: Tue, 8 Sep 2015 17:17:52 +0200 Subject: [PATCH] [3970] Addressed review comments again. Implemented new test for the TimerMgr::stopThread(). Removed redundant clear of the error string. Added comment to the TimerMgr about the thread safety. --- src/lib/dhcpsrv/tests/timer_mgr_unittest.cc | 42 +++++++++++++++++++-- src/lib/dhcpsrv/timer_mgr.h | 22 +++++++++++ src/lib/util/watch_socket.cc | 3 -- 3 files changed, 60 insertions(+), 7 deletions(-) diff --git a/src/lib/dhcpsrv/tests/timer_mgr_unittest.cc b/src/lib/dhcpsrv/tests/timer_mgr_unittest.cc index 8f7b00fcbe..02f52075bb 100644 --- a/src/lib/dhcpsrv/tests/timer_mgr_unittest.cc +++ b/src/lib/dhcpsrv/tests/timer_mgr_unittest.cc @@ -62,7 +62,10 @@ public: /// @brief Wait for one or many ready handlers. /// /// @param timeout Wait timeout in milliseconds. - void doWait(const long timeout); + /// @param call_receive Indicates if the @c IfaceMgr::receive6 + /// should be called to run pending callbacks and clear + /// watch sockets. + void doWait(const long timeout, const bool call_receive = true); /// @brief Generic callback for timers under test. /// @@ -129,7 +132,7 @@ TimerMgrTest::registerTimer(const std::string& timer_name, const long timer_inte } void -TimerMgrTest::doWait(const long timeout) { +TimerMgrTest::doWait(const long timeout, const bool call_receive) { IntervalTimer timeout_timer(io_service_); timeout_timer.setup(boost::bind(&TimerMgrTest::timeoutCallback, this), timeout, IntervalTimer::ONE_SHOT); @@ -137,8 +140,10 @@ TimerMgrTest::doWait(const long timeout) { // The timeout flag will be set by the timeoutCallback if the test // lasts for too long. In this case we will return from here. while (!timeout_) { - // Block for one 1 millisecond. - IfaceMgr::instance().receive6(0, 1000); + if (call_receive) { + // Block for one 1 millisecond. + IfaceMgr::instance().receive6(0, 1000); + } // Run ready handlers from the local IO service to execute // the timeout callback if necessary. io_service_.get_io_service().poll_one(); @@ -419,4 +424,33 @@ TEST_F(TimerMgrTest, scheduleTimers) { EXPECT_GT(calls_count_["timer2"], calls_count_timer2); } +// This test verifies that it is possible to force that the pending +// timer callbacks are executed when the worker thread is stopped. +TEST_F(TimerMgrTest, stopThreadWithRunningHandlers) { + TimerMgr& timer_mgr = TimerMgr::instance(); + + // Register 'timer1'. + ASSERT_NO_FATAL_FAILURE(registerTimer("timer1", 1)); + + // We can start the worker thread before we even kick in the timers. + ASSERT_NO_THROW(timer_mgr.startThread()); + + // Kick in the timer. + ASSERT_NO_THROW(timer_mgr.setup("timer1")); + + // Run the thread for 100ms. This should run some timers. The 'false' + // value indicates that the IfaceMgr::receive6 is not called, so the + // watch socket is never cleared. + doWait(100, false); + + // There should be no calls registered for the timer1. + EXPECT_EQ(0, calls_count_["timer1"]); + + // Stop the worker thread with completing pending callbacks. + ASSERT_NO_THROW(timer_mgr.stopThread(true)); + + // There should be one call registered. + EXPECT_EQ(1, calls_count_["timer1"]); +} + } // end of anonymous namespace diff --git a/src/lib/dhcpsrv/timer_mgr.h b/src/lib/dhcpsrv/timer_mgr.h index 5465f892f2..7625204df9 100644 --- a/src/lib/dhcpsrv/timer_mgr.h +++ b/src/lib/dhcpsrv/timer_mgr.h @@ -112,6 +112,17 @@ public: /// @brief Registers new timers in the @c TimerMgr. /// + /// This method must not be called while the worker thread is running, + /// as it modifies the internal data structure holding registered + /// timers, which is also accessed from the worker thread via the + /// callback. Inserting new element to this data structure and + /// reading it at the same time would yield undefined behavior. + /// + /// In order to prevent race conditions between the worker thread and + /// this method a mutex could be introduced. However, locking the mutex + /// would be required for all callback invocations, which could have + /// negative impact on the performance. + /// /// @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 @@ -133,6 +144,17 @@ public: /// socket from the @c IfaceMgr and closes it. It finally removes the /// timer from the internal collection of timers. /// + /// This method must not be called while the worker thread is running, + /// as it modifies the internal data structure holding registered + /// timers, which is also accessed from the worker thread via the + /// callback. Removing element from this data structure and + /// reading it at the same time would yield undefined behavior. + /// + /// In order to prevent race conditions between the worker thread and + /// this method a mutex could be introduced. However, locking the mutex + /// would be required for all callback invocations which could have + /// negative impact on the performance. + /// /// @param timer_name Name of the timer to be unregistered. /// /// @throw BadValue if the specified timer hasn't been registered. diff --git a/src/lib/util/watch_socket.cc b/src/lib/util/watch_socket.cc index 77246732f3..6ac0128e60 100644 --- a/src/lib/util/watch_socket.cc +++ b/src/lib/util/watch_socket.cc @@ -120,9 +120,6 @@ WatchSocket::clearReady() { bool WatchSocket::closeSocket(std::string& error_string) { - // Clear error string. - error_string.clear(); - std::ostringstream s; // Close the pipe fds. Technically a close can fail (hugely unlikely) // but there's no recovery for it either. If one does fail we log it -- 2.47.3