[4047] Updated TimerMgr and tests per review comments.

src/lib/dhcpsrv/tests/timer_mgr_unittest.cc

@@ -100,32 +100,33 @@ public:
     /// of calls to the timer handlers.
     CallsCount calls_count_;
 
+    /// @brief Instance of @c TimerMgr used by the tests.
+    TimerMgrPtr timer_mgr_;
 };
 
 void
 TimerMgrTest::SetUp() {
+    timer_mgr_ = TimerMgr::instance();
     calls_count_.clear();
     // Make sure there are no dangling threads.
-    TimerMgr::instance()->stopThread();
+    timer_mgr_->stopThread();
 }
 
 void
 TimerMgrTest::TearDown() {
     // Make sure there are no dangling threads.
-    TimerMgr::instance()->stopThread();
+    timer_mgr_->stopThread();
     // Remove all timers.
-    TimerMgr::instance()->unregisterTimers();
+    timer_mgr_->unregisterTimers();
 }
 
 void
 TimerMgrTest::registerTimer(const std::string& timer_name, const long timer_interval,
                             const IntervalTimer::Mode& timer_mode) {
-    const TimerMgrPtr& timer_mgr = TimerMgr::instance();
-
     // Register the timer with the generic callback that counts the
     // number of callback invocations.
     ASSERT_NO_THROW(
-        timer_mgr->registerTimer(timer_name, makeCallback(timer_name), timer_interval,
+        timer_mgr_->registerTimer(timer_name, makeCallback(timer_name), timer_interval,
                                  timer_mode)
     );
 
@@ -151,7 +152,7 @@ TimerMgrTest::timerCallback(const std::string& timer_name) {
 
     // The timer installed is the ONE_SHOT timer, so we have
     // to reschedule the timer.
-    TimerMgr::instance()->setup(timer_name);
+    timer_mgr_->setup(timer_name);
 }
 
 void
@@ -173,52 +174,48 @@ TimerMgrTest::makeCallbackWithException() {
 // parameters are specified when registering a timer, or when
 // the registration can't be made.
 TEST_F(TimerMgrTest, registerTimer) {
-    const TimerMgrPtr& timer_mgr = TimerMgr::instance();
-
     // Empty timer name is not allowed.
-    ASSERT_THROW(timer_mgr->registerTimer("", makeCallback("timer1"), 1,
-                                          IntervalTimer::ONE_SHOT),
+    ASSERT_THROW(timer_mgr_->registerTimer("", makeCallback("timer1"), 1,
+                                           IntervalTimer::ONE_SHOT),
                  BadValue);
 
     // Add a timer with a correct name.
-    ASSERT_NO_THROW(timer_mgr->registerTimer("timer2", makeCallback("timer2"), 1,
-                                             IntervalTimer::ONE_SHOT));
+    ASSERT_NO_THROW(timer_mgr_->registerTimer("timer2", makeCallback("timer2"), 1,
+                                              IntervalTimer::ONE_SHOT));
     // Adding the timer with the same name as the existing timer is not
     // allowed.
-    ASSERT_THROW(timer_mgr->registerTimer("timer2", makeCallback("timer2"), 1,
-                                          IntervalTimer::ONE_SHOT),
+    ASSERT_THROW(timer_mgr_->registerTimer("timer2", makeCallback("timer2"), 1,
+                                           IntervalTimer::ONE_SHOT),
                  BadValue);
 
     // Start worker thread.
-    ASSERT_NO_THROW(timer_mgr->startThread());
+    ASSERT_NO_THROW(timer_mgr_->startThread());
 
     // Can't register the timer when the thread is running.
-    ASSERT_THROW(timer_mgr->registerTimer("timer1", makeCallback("timer1"), 1,
-                                          IntervalTimer::ONE_SHOT),
+    ASSERT_THROW(timer_mgr_->registerTimer("timer1", makeCallback("timer1"), 1,
+                                           IntervalTimer::ONE_SHOT),
                  InvalidOperation);
 
     // Stop the thread and retry.
-    ASSERT_NO_THROW(timer_mgr->stopThread());
-    EXPECT_NO_THROW(timer_mgr->registerTimer("timer1", makeCallback("timer1"), 1,
-                                             IntervalTimer::ONE_SHOT));
+    ASSERT_NO_THROW(timer_mgr_->stopThread());
+    EXPECT_NO_THROW(timer_mgr_->registerTimer("timer1", makeCallback("timer1"), 1,
+                                              IntervalTimer::ONE_SHOT));
 
 }
 
 // This test verifies that it is possible to unregister a timer from
 // the TimerMgr.
 TEST_F(TimerMgrTest, unregisterTimer) {
-    const TimerMgrPtr& timer_mgr = TimerMgr::instance();
-
     // Register a timer and start it.
     ASSERT_NO_FATAL_FAILURE(registerTimer("timer1", 1));
-    ASSERT_NO_THROW(timer_mgr->setup("timer1"));
-    ASSERT_NO_THROW(timer_mgr->startThread());
+    ASSERT_NO_THROW(timer_mgr_->setup("timer1"));
+    ASSERT_NO_THROW(timer_mgr_->startThread());
 
     // Wait for the timer to execute several times.
     doWait(100);
 
     // Stop the thread but execute pending callbacks.
-    ASSERT_NO_THROW(timer_mgr->stopThread(true));
+    ASSERT_NO_THROW(timer_mgr_->stopThread(true));
 
     // Remember how many times the timer's callback was executed.
     const unsigned int calls_count = calls_count_["timer1"];
@@ -226,15 +223,15 @@ TEST_F(TimerMgrTest, unregisterTimer) {
 
     // Check that an attempt to unregister a non-existing timer would
     // result in exeception.
-    EXPECT_THROW(timer_mgr->unregisterTimer("timer2"), BadValue);
+    EXPECT_THROW(timer_mgr_->unregisterTimer("timer2"), BadValue);
 
     // Now unregister the correct one.
-    ASSERT_NO_THROW(timer_mgr->unregisterTimer("timer1"));
+    ASSERT_NO_THROW(timer_mgr_->unregisterTimer("timer1"));
 
     // Start the thread again and wait another 100ms.
-    ASSERT_NO_THROW(timer_mgr->startThread());
+    ASSERT_NO_THROW(timer_mgr_->startThread());
     doWait(100);
-    ASSERT_NO_THROW(timer_mgr->stopThread(true));
+    ASSERT_NO_THROW(timer_mgr_->stopThread(true));
 
     // The number of calls for the timer1 shouldn't change as the
     // timer had been unregistered.
@@ -248,8 +245,6 @@ TEST_F(TimerMgrTest, unregisterTimer) {
 /// solve the problem. See ticket #4009. Until this ticket is
 /// implemented, the test should remain disabled.
 TEST_F(TimerMgrTest, unregisterTimers) {
-    const TimerMgrPtr& timer_mgr = TimerMgr::instance();
-
     // Register 10 timers.
     for (int i = 1; i <= 20; ++i) {
         std::ostringstream s;
@@ -257,15 +252,15 @@ TEST_F(TimerMgrTest, unregisterTimers) {
         ASSERT_NO_FATAL_FAILURE(registerTimer(s.str(), 1))
             << "fatal failure occurred while registering "
             << s.str();
-        ASSERT_NO_THROW(timer_mgr->setup(s.str()))
+        ASSERT_NO_THROW(timer_mgr_->setup(s.str()))
             << "exception thrown while calling setup() for the "
             << s.str();
     }
 
     // Start worker thread and wait for 500ms.
-    ASSERT_NO_THROW(timer_mgr->startThread());
+    ASSERT_NO_THROW(timer_mgr_->startThread());
     doWait(500);
-    ASSERT_NO_THROW(timer_mgr->stopThread(true));
+    ASSERT_NO_THROW(timer_mgr_->stopThread(true));
 
     // Make sure that all timers have been executed at least once.
     for (CallsCount::iterator it = calls_count_.begin();
@@ -281,12 +276,12 @@ TEST_F(TimerMgrTest, unregisterTimers) {
     CallsCount calls_count(calls_count_);
 
     // Let's unregister all timers.
-    ASSERT_NO_THROW(timer_mgr->unregisterTimers());
+    ASSERT_NO_THROW(timer_mgr_->unregisterTimers());
 
     // Start worker thread again and wait for 500ms.
-    ASSERT_NO_THROW(timer_mgr->startThread());
+    ASSERT_NO_THROW(timer_mgr_->startThread());
     doWait(500);
-    ASSERT_NO_THROW(timer_mgr->stopThread(true));
+    ASSERT_NO_THROW(timer_mgr_->stopThread(true));
 
     // The calls counter shouldn't change because there are
     // no timers registered.
@@ -303,9 +298,9 @@ TEST_F(TimerMgrTest, unregisterTimerWhileRunning) {
     ASSERT_NO_FATAL_FAILURE(registerTimer("timer2", 1));
 
     // Start the thread and make sure we can't unregister them.
-    ASSERT_NO_THROW(timer_mgr->startThread());
-    EXPECT_THROW(timer_mgr->unregisterTimer("timer1"), InvalidOperation);
-    EXPECT_THROW(timer_mgr->unregisterTimers(), InvalidOperation);
+    ASSERT_NO_THROW(timer_mgr_->startThread());
+    EXPECT_THROW(timer_mgr_->unregisterTimer("timer1"), InvalidOperation);
+    EXPECT_THROW(timer_mgr_->unregisterTimers(), InvalidOperation);
 
     // No need to stop the thread as it will be stopped by the
     // test fixture destructor.
@@ -313,42 +308,40 @@ TEST_F(TimerMgrTest, unregisterTimerWhileRunning) {
 
 // This test verifies that the timer execution can be cancelled.
 TEST_F(TimerMgrTest, cancel) {
-    const TimerMgrPtr& timer_mgr = TimerMgr::instance();
-
     // Register timer.
     ASSERT_NO_FATAL_FAILURE(registerTimer("timer1", 1));
 
     // Kick in the timer and wait for 500ms.
-    ASSERT_NO_THROW(timer_mgr->setup("timer1"));
-    ASSERT_NO_THROW(timer_mgr->startThread());
+    ASSERT_NO_THROW(timer_mgr_->setup("timer1"));
+    ASSERT_NO_THROW(timer_mgr_->startThread());
     doWait(500);
-    ASSERT_NO_THROW(timer_mgr->stopThread());
+    ASSERT_NO_THROW(timer_mgr_->stopThread());
 
     // Cancelling non-existing timer should fail.
-    EXPECT_THROW(timer_mgr->cancel("timer2"), BadValue);
+    EXPECT_THROW(timer_mgr_->cancel("timer2"), BadValue);
 
     // Cancelling the good one should pass, even when the worker
     // thread is running.
-    ASSERT_NO_THROW(timer_mgr->cancel("timer1"));
+    ASSERT_NO_THROW(timer_mgr_->cancel("timer1"));
 
     // Remember how many calls have been invoked and wait for
     // another 500ms.
     unsigned int calls_count = calls_count_["timer1"];
 
-    ASSERT_NO_THROW(timer_mgr->startThread());
+    ASSERT_NO_THROW(timer_mgr_->startThread());
     doWait(500);
     // Stop thread before we setup again.
-    ASSERT_NO_THROW(timer_mgr->stopThread());
+    ASSERT_NO_THROW(timer_mgr_->stopThread());
 
     // The number of calls shouldn't change because the timer had been
     // cancelled.
     ASSERT_EQ(calls_count, calls_count_["timer1"]);
 
     // Setup the timer again.
-    ASSERT_NO_THROW(timer_mgr->setup("timer1"));
+    ASSERT_NO_THROW(timer_mgr_->setup("timer1"));
 
     // Restart the thread.
-    ASSERT_NO_THROW(timer_mgr->startThread());
+    ASSERT_NO_THROW(timer_mgr_->startThread());
     doWait(500);
 
     // New calls should be recorded.
@@ -358,8 +351,6 @@ TEST_F(TimerMgrTest, cancel) {
 // This test verifies that the callbacks for the scheduled timers are
 // actually called.
 TEST_F(TimerMgrTest, scheduleTimers) {
-    const TimerMgrPtr& timer_mgr = TimerMgr::instance();
-
     // Register two timers: 'timer1' and 'timer2'. The first timer will
     // be executed at the 1ms interval. The second one at the 5ms
     // interval.
@@ -367,11 +358,11 @@ TEST_F(TimerMgrTest, scheduleTimers) {
     ASSERT_NO_FATAL_FAILURE(registerTimer("timer2", 5));
 
     // Kick in the timers.
-    ASSERT_NO_THROW(timer_mgr->setup("timer1"));
-    ASSERT_NO_THROW(timer_mgr->setup("timer2"));
+    ASSERT_NO_THROW(timer_mgr_->setup("timer1"));
+    ASSERT_NO_THROW(timer_mgr_->setup("timer2"));
 
     // We can start the worker thread before we even kick in the timers.
-    ASSERT_NO_THROW(timer_mgr->startThread());
+    ASSERT_NO_THROW(timer_mgr_->startThread());
 
     // Run IfaceMgr::receive6() in the loop for 500ms. This function
     // will read data from the watch sockets created when the timers
@@ -383,7 +374,7 @@ TEST_F(TimerMgrTest, scheduleTimers) {
 
     // Stop the worker thread, which would halt the execution of
     // the timers.
-    ASSERT_NO_THROW(timer_mgr->stopThread(true));
+    ASSERT_NO_THROW(timer_mgr_->stopThread(true));
 
     // We have been running the timer for 500ms at the interval of
     // 1 ms. The maximum number of callbacks is 500. However, the
@@ -405,10 +396,10 @@ TEST_F(TimerMgrTest, scheduleTimers) {
     unsigned int calls_count_timer2 = calls_count_["timer2"];
 
     // Unregister the 'timer1'.
-    ASSERT_NO_THROW(timer_mgr->unregisterTimer("timer1"));
+    ASSERT_NO_THROW(timer_mgr_->unregisterTimer("timer1"));
 
     // Restart the thread.
-    ASSERT_NO_THROW(timer_mgr->startThread());
+    ASSERT_NO_THROW(timer_mgr_->startThread());
 
     // Wait another 500ms. The 'timer1' was unregistered so it
     // should not make any more calls. The 'timer2' should still
@@ -424,14 +415,12 @@ TEST_F(TimerMgrTest, scheduleTimers) {
 // 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) {
-    const TimerMgrPtr& timer_mgr = TimerMgr::instance();
-
     // Register 'timer1'.
     ASSERT_NO_FATAL_FAILURE(registerTimer("timer1", 1));
 
     // Kick in the timer.
-    ASSERT_NO_THROW(timer_mgr->setup("timer1"));
-    ASSERT_NO_THROW(timer_mgr->startThread());
+    ASSERT_NO_THROW(timer_mgr_->setup("timer1"));
+    ASSERT_NO_THROW(timer_mgr_->startThread());
 
     // Run the thread for 100ms. This should run some timers. The 'false'
     // value indicates that the IfaceMgr::receive6 is not called, so the
@@ -442,13 +431,13 @@ TEST_F(TimerMgrTest, stopThreadWithRunningHandlers) {
     EXPECT_EQ(0, calls_count_["timer1"]);
 
     // Stop the worker thread without completing pending callbacks.
-    ASSERT_NO_THROW(timer_mgr->stopThread(false));
+    ASSERT_NO_THROW(timer_mgr_->stopThread(false));
 
     // There should be still not be any calls registered.
     EXPECT_EQ(0, calls_count_["timer1"]);
 
     // We can restart the worker thread before we even kick in the timers.
-    ASSERT_NO_THROW(timer_mgr->startThread());
+    ASSERT_NO_THROW(timer_mgr_->startThread());
 
     // Run the thread for 100ms. This should run some timers. The 'false'
     // value indicates that the IfaceMgr::receive6 is not called, so the
@@ -459,7 +448,7 @@ TEST_F(TimerMgrTest, stopThreadWithRunningHandlers) {
     EXPECT_EQ(0, calls_count_["timer1"]);
 
     // Stop the worker thread with completing pending callbacks.
-    ASSERT_NO_THROW(timer_mgr->stopThread(true));
+    ASSERT_NO_THROW(timer_mgr_->stopThread(true));
 
     // There should be one call registered.
     EXPECT_EQ(1, calls_count_["timer1"]);
@@ -468,22 +457,20 @@ TEST_F(TimerMgrTest, stopThreadWithRunningHandlers) {
 // This test verifies that exceptions emitted from the callback would
 // be handled by the TimerMgr.
 TEST_F(TimerMgrTest, callbackWithException) {
-    const TimerMgrPtr& timer_mgr = TimerMgr::instance();
-
     // Create timer which will trigger callback generating exception.
     ASSERT_NO_THROW(
-        timer_mgr->registerTimer("timer1", makeCallbackWithException(), 1,
-                                 IntervalTimer::ONE_SHOT)
+        timer_mgr_->registerTimer("timer1", makeCallbackWithException(), 1,
+                                  IntervalTimer::ONE_SHOT)
     );
 
     // Setup the timer.
-    ASSERT_NO_THROW(timer_mgr->setup("timer1"));
+    ASSERT_NO_THROW(timer_mgr_->setup("timer1"));
 
     // Start thread. We hope that exception will be caught by the @c TimerMgr
     // and will not kill the process.
-    ASSERT_NO_THROW(timer_mgr->startThread());
+    ASSERT_NO_THROW(timer_mgr_->startThread());
     doWait(500);
-    ASSERT_NO_THROW(timer_mgr->stopThread(true));
+    ASSERT_NO_THROW(timer_mgr_->stopThread(true));
 }
 
 } // end of anonymous namespace

src/lib/dhcpsrv/timer_mgr.cc

@@ -205,7 +205,7 @@ public:
     ///
     /// @param run_pending_callbacks Indicates if the pending callbacks
     /// should be executed (if true).
-    void controlledStopThread(const bool run_pending_callbacks);
+    void stopThread(const bool run_pending_callbacks);
 
 private:
 
@@ -216,8 +216,8 @@ private:
     /// its scheduled interval elapses.
     ///
     /// This method marks the @c util::Watch socket associated with the
-    /// timer as ready (writes data to a pipe). This method will block until
-    /// @c TimerMgrImpl::if§aceMgrCallback is executed from the main thread by
+    /// timer as ready (writes data to a pipe). This method will BLOCK until
+    /// @c TimerMgrImpl::ifaceMgrCallback is executed from the main thread by
     /// the @c IfaceMgr.
     ///
     /// @param timer_name Unique timer name to be passed to the callback.
@@ -452,7 +452,7 @@ TimerMgrImpl::createThread() {
 }
 
 void
-TimerMgrImpl::controlledStopThread(const bool run_pending_callbacks) {
+TimerMgrImpl::stopThread(const bool run_pending_callbacks) {
     // Set the stopping flag to true while we're stopping. This will be
     // automatically reset to false when the function exits or exception
     // is thrown.
@@ -692,7 +692,7 @@ TimerMgr::stopThread(const bool run_pending_callbacks) {
         LOG_DEBUG(dhcpsrv_logger, DHCPSRV_DBG_TRACE,
                   DHCPSRV_TIMERMGR_STOP_THREAD);
 
-        impl_->controlledStopThread(run_pending_callbacks);
+        impl_->stopThread(run_pending_callbacks);
     }
 }
 IOService&

src/lib/dhcpsrv/timer_mgr.h

@@ -100,6 +100,24 @@ typedef boost::shared_ptr<TimerMgr> TimerMgrPtr;
 /// because the worker thread is blocked while the callback function
 /// is executed.
 ///
+/// The worker thread is blocked when it executes a generic callback
+/// function in the @c TimerMgr, which marks the watch socket
+/// associated with the elapsed timer as "ready". The thread waits
+/// in the callback function until it is notified by the main thread
+/// (via conditional variable), that one of the watch sockets has
+/// been cleared. It then checks if the main thread cleared the
+/// socket that the worker thread had set. It continues to block
+/// if this was a different socket. It returns (unblocks) otherwise.
+/// The main thread clears the socket when the @c IfaceMgr detects
+/// that this socket has been marked ready by the worker thread.
+/// This is triggered only when the @c IfaceMgr::receive4 or
+/// @c IfaceMgr::receive6 is called. They are called in the main
+/// loops of the DHCP servers, which are also responsible for
+/// processing received packets. Therefore it may take some
+/// time for the main loop to detect that the socket has been
+/// marked ready, call appropriate handler for it and clear it.
+/// In the mean time, the worker thread will remain blocked.
+///
 class TimerMgr : public boost::noncopyable {
 public: