[3970] Prevent race conditions during timers registration.

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();

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();
    }
 }
 

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