[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.

diff --git a/src/lib/dhcpsrv/tests/timer_mgr_unittest.cc b/src/lib/dhcpsrv/tests/timer_mgr_unittest.cc
index 8f7b00fcbeddd0af0ef242807d0fde419b8d664a..02f52075bb3d4753f8425faa3a9d5a5271a7b723 100644 (file)
--- 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 5465f892f296e596cf797b9ab65340c41f98cfb0..7625204df9a597e21c7f4115cdb3572f6d40e380 100644 (file)
--- 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 77246732f369707b9fed903665036c20f02e374c..6ac0128e60a2300f76b6778a25c6b906b64b8ca6 100644 (file)
--- 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