Browse Source

[4047] Implemented synchronization between the worker thead

Marcin Siodelski 9 years ago
parent
commit
8c4de872d4

+ 23 - 0
src/lib/dhcpsrv/dhcpsrv_messages.mes

@@ -581,6 +581,11 @@ lease from the PostgreSQL database for the specified address.
 A debug message issued when the server is attempting to update IPv6
 lease from the PostgreSQL database for the specified address.
 
+% DHCPSRV_TIMERMGR_CALLBACK_FAILED running handler for timer %1 caused exception: %2
+This error message is emitted when the timer elapsed and the
+operation associated with this timer has thrown an exception.
+The timer name and the reason for exception is logged.
+
 % DHCPSRV_TIMERMGR_REGISTER_TIMER registering timer: %1, using interval: %2 ms
 A debug message issued when the new interval timer is registered in
 the Timer Manager. This timer will have a callback function
@@ -596,6 +601,24 @@ An example of such operation is a periodic cleanup of
 expired leases. The name of the timer is included in the
 message.
 
+% DHCPSRV_TIMERMGR_SOCKET_CLEAR_FAILED clearing watch socket for timer %1 failed: %2
+An error message indicating that the specified timer elapsed,
+the operation associated with the timer was executed but the
+server was unable to signal this to the worker thread responsible
+for dispatching timers. The thread will continue but it will
+not be able to dispatch any operations for this timer. The
+server reconfiguration or restart may solve the problem
+but the situation may repeat.
+
+% DHCPSRV_TIMERMGR_SOCKET_MARK_FAILED marking watch socket for timer %1 failed: %2
+An error message indicating that the specified timer elapsed,
+but the server was unable to flag that the handler function
+should be executed for this timer. The callback will not
+be executed this time and most likely the subsequent attempts
+will not be successful too. This error is highly unlikely.
+The name of the timer and thre reason for failure is included
+in the message.
+
 % DHCPSRV_TIMERMGR_START_THREAD starting thread for timers
 A debug message issued when the Timer Manager is starting a
 worker thread to run started timers. The worker thread is

+ 44 - 1
src/lib/dhcpsrv/tests/timer_mgr_unittest.cc

@@ -75,12 +75,21 @@ public:
     /// be increased.
     void timerCallback(const std::string& timer_name);
 
+    /// @brief Callback which generates exception.
+    ///
+    /// This callback is used to test that the @c TimerMgr can handle
+    /// the case when the callback generates exceptions.
+    void timerCallbackWithException();
+
     /// @brief Create a generic callback function for the timer.
     ///
     /// This is just a wrapped to make it a bit more convenient
     /// in the test.
     boost::function<void ()> makeCallback(const std::string& timer_name);
 
+    /// @brief Create a callback which generates exception.
+    boost::function<void ()> makeCallbackWithException();
+
     /// @brief Callback for timeout.
     ///
     /// This callback indicates the test timeout by setting the
@@ -162,11 +171,21 @@ TimerMgrTest::timerCallback(const std::string& timer_name) {
     TimerMgr::instance().setup(timer_name);
 }
 
+void
+TimerMgrTest::timerCallbackWithException() {
+    isc_throw(Exception, "timerCallbackWithException");
+}
+
 boost::function<void ()>
 TimerMgrTest::makeCallback(const std::string& timer_name) {
     return (boost::bind(&TimerMgrTest::timerCallback, this, timer_name));
 }
 
+boost::function<void ()>
+TimerMgrTest::makeCallbackWithException() {
+    return (boost::bind(&TimerMgrTest::timerCallbackWithException, this));
+}
+
 
 void
 TimerMgrTest::timeoutCallback() {
@@ -255,7 +274,7 @@ TEST_F(TimerMgrTest, unregisterTimer) {
 /// Replacing it with the ASIO implementation from BOOST does
 /// solve the problem. See ticket #4009. Until this ticket is
 /// implemented, the test should remain disabled.
-TEST_F(TimerMgrTest, DISABLED_unregisterTimers) {
+TEST_F(TimerMgrTest, unregisterTimers) {
     TimerMgr& timer_mgr = TimerMgr::instance();
 
     // Register 10 timers.
@@ -349,8 +368,11 @@ TEST_F(TimerMgrTest, cancel) {
     // cancelled.
     ASSERT_EQ(calls_count, calls_count_["timer1"]);
 
+    TimerMgr::instance().stopThread();
+
     // Setup the timer again.
     ASSERT_NO_THROW(timer_mgr.setup("timer1"));
+    TimerMgr::instance().startThread();
     doWait(500);
 
     // New calls should be recorded.
@@ -470,4 +492,25 @@ TEST_F(TimerMgrTest, stopThreadWithRunningHandlers) {
     EXPECT_EQ(1, calls_count_["timer1"]);
 }
 
+// This test verifies that exceptions emitted from the callback would
+// be handled by the TimerMgr.
+TEST_F(TimerMgrTest, callbackWithException) {
+    TimerMgr& timer_mgr = TimerMgr::instance();
+
+    // Create timer which will trigger callback generating exception.
+    ASSERT_NO_THROW(
+        timer_mgr.registerTimer("timer1", makeCallbackWithException(), 1,
+                                IntervalTimer::ONE_SHOT)
+    );
+
+    // Setup the timer.
+    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());
+    doWait(500);
+    ASSERT_NO_THROW(timer_mgr.stopThread(true));
+}
+
 } // end of anonymous namespace

+ 503 - 94
src/lib/dhcpsrv/timer_mgr.cc

@@ -18,6 +18,9 @@
 #include <dhcpsrv/dhcpsrv_log.h>
 #include <dhcpsrv/timer_mgr.h>
 #include <exceptions/exceptions.h>
+#include <util/threads/sync.h>
+#include <util/threads/thread.h>
+#include <util/watch_socket.h>
 #include <boost/bind.hpp>
 #include <utility>
 
@@ -26,38 +29,294 @@ using namespace isc::asiolink;
 using namespace isc::util;
 using namespace isc::util::thread;
 
+namespace {
+
+/// @brief Simple RAII object setting value to true while in scope.
+///
+/// This class is useful to temporarly set the value to true and
+/// automatically reset it to false when the object is destroyed
+/// as a result of return or exception.
+class ScopedTrue {
+public:
+
+    /// @brief Constructor.
+    ///
+    /// Sets the boolean value to true.
+    ///
+    /// @param value reference to the value to be set to true.
+    ScopedTrue(bool& value, Mutex& mutex)
+        : value_(value), mutex_(mutex) {
+        Mutex::Locker lock(mutex_);
+        value_ = true;
+    }
+
+    /// @brief Destructor.
+    ///
+    /// Sets the value to false.
+    ~ScopedTrue() {
+        Mutex::Locker lock(mutex_);
+        value_ = false;
+    }
+
+private:
+
+    /// @brief Reference to the controlled value.
+    bool& value_;
+
+    /// @brief Mutex to be used to lock while performing write
+    /// operations.
+    Mutex& mutex_;
+};
+
+/// @brief Structure holding information for a single timer.
+///
+/// This structure holds the instance of the watch socket being used to
+/// signal that the timer is "ready". It also holds the instance of the
+/// interval timer and other parameters pertaining to it.
+struct TimerInfo {
+    /// @brief Instance of the watch socket.
+    util::WatchSocket watch_socket_;
+
+    /// @brief Instance of the interval timer.
+    asiolink::IntervalTimer interval_timer_;
+
+    /// @brief Holds the pointer to the callback supplied when registering
+    /// the timer.
+    asiolink::IntervalTimer::Callback user_callback_;
+
+    /// @brief Interval timer interval supplied during registration.
+    long interval_;
+
+    /// @brief Interval timer scheduling mode supplied during registration.
+    asiolink::IntervalTimer::Mode scheduling_mode_;
+
+    /// @brief Constructor.
+    ///
+    /// @param io_service Reference to the IO service to be used by the
+    /// interval timer created.
+    /// @param user_callback Pointer to the callback function supplied
+    /// during the timer registration.
+    /// @param interval Timer interval in milliseconds.
+    /// @param mode Interval timer scheduling mode.
+    TimerInfo(asiolink::IOService& io_service,
+              const asiolink::IntervalTimer::Callback& user_callback,
+              const long interval,
+              const asiolink::IntervalTimer::Mode& mode)
+        : watch_socket_(),
+          interval_timer_(io_service),
+          user_callback_(user_callback),
+          interval_(interval),
+          scheduling_mode_(mode) { };
+};
+
+}
+
 namespace isc {
 namespace dhcp {
 
-TimerMgr&
-TimerMgr::instance() {
-    static TimerMgr timer_mgr;
-    return (timer_mgr);
-}
+/// @brief A type definition for the pointer to @c TimerInfo structure.
+typedef boost::shared_ptr<TimerInfo> TimerInfoPtr;
 
-TimerMgr::TimerMgr()
-    : io_service_(new IOService()), thread_(),
-      registered_timers_() {
-}
+/// @brief A type definition for the map holding timers configuration.
+typedef std::map<std::string, TimerInfoPtr> TimerInfoMap;
 
-TimerMgr::~TimerMgr() {
-    // Stop the thread, but do not unregister any timers. Unregistering
-    // the timers could cause static deinitialization fiasco between the
-    // TimerMgr and IfaceMgr. By now, the caller should have unregistered
-    // the timers.
-    stopThread();
+
+/// @brief Implementation of the @c TimerMgr
+class TimerMgrImpl {
+public:
+
+    /// @brief Constructor.
+    TimerMgrImpl();
+
+    /// @brief Returns a reference to IO service used by the @c TimerMgr.
+    asiolink::IOService& getIOService() const {
+        return (*io_service_);
+    }
+
+    /// @brief Registers new timers in the @c TimerMgr.
+    ///
+    /// @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
+    /// in the DHCP server.
+    /// @param interval Timer interval in milliseconds.
+    /// @param scheduling_mode Scheduling mode of the timer as described in
+    /// @c asiolink::IntervalTimer::Mode.
+    ///
+    /// @throw BadValue if the timer name is invalid or duplicate.
+    /// @throw InvalidOperation if the worker thread is running.
+    void registerTimer(const std::string& timer_name,
+                       const asiolink::IntervalTimer::Callback& callback,
+                       const long interval,
+                       const asiolink::IntervalTimer::Mode& scheduling_mode);
+
+
+    /// @brief Unregisters specified timer.
+    ///
+    /// This method cancels the timer if it is setup. It removes the external
+    /// socket from the @c IfaceMgr and closes it. It finally removes the
+    /// timer from the internal collection of timers.
+    ///
+    /// @param timer_name Name of the timer to be unregistered.
+    ///
+    /// @throw BadValue if the specified timer hasn't been registered.
+    void unregisterTimer(const std::string& timer_name);
+
+    /// @brief Unregisters all timers.
+    ///
+    /// This method must be explicitly called prior to termination of the
+    /// process.
+    void unregisterTimers();
+
+    /// @brief Schedules the execution of the interval timer.
+    ///
+    /// This method schedules the timer, i.e. the callback will be executed
+    /// after specified interval elapses. The interval has been specified
+    /// during timer registration. Depending on the mode selected during the
+    /// timer registration, the callback will be executed once after it has
+    /// been scheduled or until it is cancelled. Though, in the former case
+    /// the timer can be re-scheduled in the callback function.
+    ///
+    /// @param timer_name Unique timer name.
+    ///
+    /// @throw BadValue if the timer hasn't been registered.
+    void setup(const std::string& timer_name);
+
+    /// @brief Cancels the execution of the interval timer.
+    ///
+    /// This method has no effect if the timer hasn't been scheduled with
+    /// the @c TimerMgr::setup method.
+    ///
+    /// @param timer_name Unique timer name.
+    ///
+    /// @throw BadValue if the timer hasn't been registered.
+    void cancel(const std::string& timer_name);
+
+    /// @brief Checks if the thread is running.
+    ///
+    /// @return true if the thread is running.
+    bool threadRunning() const;
+
+    /// @brief Starts thread.
+    void createThread();
+
+    /// @brief Stops thread gracefully.
+    ///
+    /// This methods unblocks worker thread if it is blocked waiting for
+    /// any handlers and stops it. Outstanding handlers are later executed
+    /// in the main thread and all watch sockets are cleared.
+    ///
+    /// @param run_pending_callbacks Indicates if the pending callbacks
+    /// should be executed (if true).
+    void controlledStopThread(const bool run_pending_callbacks);
+
+private:
+
+    /// @name Internal callbacks.
+    //@{
+    ///
+    /// @brief Callback function to be executed for each interval timer when
+    /// 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 TimerMgr::ifaceMgrCallback is executed from the main thread by the
+    /// @c IfaceMgr.
+    ///
+    /// @param timer_name Unique timer name to be passed to the callback.
+    void timerCallback(const std::string& timer_name);
+
+    /// @brief Callback function installed on the @c IfaceMgr and associated
+    /// with the particular timer.
+    ///
+    /// This callback function is executed by the @c IfaceMgr when the data
+    /// over the specific @c util::WatchSocket is received. This method clears
+    /// the socket (reads the data from the pipe) and executes the callback
+    /// supplied when the timer was registered.
+    ///
+    /// @param timer_name Unique timer name.
+    void ifaceMgrCallback(const std::string& timer_name);
+
+    //@}
+
+    /// @name Methods to handle ready sockets.
+    //@{
+    ///
+    /// @brief Clear ready sockets and optionally run callbacks.
+    ///
+    /// This method is called by the @c TimerMgr::stopThread method
+    /// to clear watch sockets which may be marked as ready. It will
+    /// also optionally run callbacks installed for the timers which
+    /// marked sockets as ready.
+    ///
+    /// @param run_pending_callbacks Indicates if the callbacks should
+    /// be executed for the sockets being cleared (if true).
+    void clearReadySockets(const bool run_pending_callbacks);
+
+    /// @brief Clears a socket and optionally runs a callback.
+    ///
+    /// This method clears the ready socket pointed to by the specified
+    /// iterator. If the @c run_callback is set, the callback will
+    /// also be executed.
+    ///
+    /// @param timer_info_iterator Iterator pointing to the timer
+    /// configuration structure.
+    /// @param run_callback Boolean value indicating if the callback
+    /// should be executed for the socket being cleared (if true).
+    ///
+    /// @tparam Iterator Iterator pointing to the timer configuration
+    /// structure.
+    template<typename Iterator>
+    void handleReadySocket(Iterator timer_info_iterator, const bool run_callback);
+
+    //@}
+
+    /// @brief Blocking wait for the socket to be cleared.
+    void waitForSocketClearing(WatchSocket& watch_socket);
+
+    /// @brief Signals that a watch socket has been cleared.
+    void signalSocketClearing();
+
+    /// @brief Pointer to the io service.
+    asiolink::IOServicePtr io_service_;
+
+    /// @brief Pointer to the worker thread.
+    ///
+    /// This is initially set to NULL until the thread is started using the
+    /// @c TimerMgr::startThread. The @c TimerMgr::stopThread sets it back
+    /// to NULL.
+    boost::shared_ptr<util::thread::Thread> thread_;
+
+    /// @brief Mutex used to synchronize main thread and the worker thread.
+    util::thread::Mutex mutex_;
+
+    /// @brief Conditional variable used to synchronize main thread and
+    /// worker thread.
+    util::thread::CondVar cond_var_;
+
+    /// @brief Boolean value indicating if the thread is being stopped.
+    bool stopping_;
+
+    /// @brief Holds mapping of the timer name to the watch socket, timer
+    /// instance and other parameters pertaining to the timer.
+    ///
+    /// Each registered timer has a unique name which is used as a key to
+    /// 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_;
+
+};
+
+TimerMgrImpl::TimerMgrImpl() :
+    io_service_(new IOService()), thread_(), mutex_(), cond_var_(),
+    stopping_(false), registered_timers_() {
 }
 
 void
-TimerMgr::registerTimer(const std::string& timer_name,
-                        const IntervalTimer::Callback& callback,
-                        const long interval,
-                        const IntervalTimer::Mode& scheduling_mode) {
-
-    LOG_DEBUG(dhcpsrv_logger, DHCPSRV_DBG_TRACE,
-              DHCPSRV_TIMERMGR_REGISTER_TIMER)
-        .arg(timer_name)
-        .arg(interval);
+TimerMgrImpl::registerTimer(const std::string& timer_name,
+                            const IntervalTimer::Callback& callback,
+                            const long interval,
+                            const IntervalTimer::Mode& scheduling_mode) {
 
     // Timer name must not be empty.
     if (timer_name.empty()) {
@@ -90,7 +349,7 @@ TimerMgr::registerTimer(const std::string& timer_name,
     // this may fail is when the socket failed to open which would have caused
     // an exception in the previous call. So we should be safe here.
     IfaceMgr::instance().addExternalSocket(timer_info->watch_socket_.getSelectFd(),
-                                           boost::bind(&TimerMgr::ifaceMgrCallback,
+                                           boost::bind(&TimerMgrImpl::ifaceMgrCallback,
                                                        this, timer_name));
 
     // Actually register the timer.
@@ -99,11 +358,7 @@ TimerMgr::registerTimer(const std::string& timer_name,
 }
 
 void
-TimerMgr::unregisterTimer(const std::string& timer_name) {
-
-    LOG_DEBUG(dhcpsrv_logger, DHCPSRV_DBG_TRACE,
-              DHCPSRV_TIMERMGR_UNREGISTER_TIMER)
-        .arg(timer_name);
+TimerMgrImpl::unregisterTimer(const std::string& timer_name) {
 
     if (thread_) {
         isc_throw(InvalidOperation, "unable to unregister timer "
@@ -132,11 +387,7 @@ TimerMgr::unregisterTimer(const std::string& timer_name) {
 }
 
 void
-TimerMgr::unregisterTimers() {
-
-    LOG_DEBUG(dhcpsrv_logger, DHCPSRV_DBG_TRACE,
-              DHCPSRV_TIMERMGR_UNREGISTER_ALL_TIMERS);
-
+TimerMgrImpl::unregisterTimers() {
     // Copy the map holding timers configuration. This is required so as
     // we don't cut the branch which we're sitting on when we will be
     // erasing the timers. We're going to iterate over the register timers
@@ -156,11 +407,7 @@ TimerMgr::unregisterTimers() {
 }
 
 void
-TimerMgr::setup(const std::string& timer_name) {
-
-    LOG_DEBUG(dhcpsrv_logger, DHCPSRV_DBG_TRACE,
-              DHCPSRV_TIMERMGR_START_TIMER)
-        .arg(timer_name);
+TimerMgrImpl::setup(const std::string& timer_name) {
 
    // Check if the specified timer exists.
    TimerInfoMap::const_iterator timer_info_it = registered_timers_.find(timer_name);
@@ -172,17 +419,14 @@ TimerMgr::setup(const std::string& timer_name) {
    // Schedule the execution of the timer using the parameters supplied
    // during the registration.
    const TimerInfoPtr& timer_info = timer_info_it->second;
-   timer_info->interval_timer_.setup(boost::bind(&TimerMgr::timerCallback, this, timer_name),
-                                     timer_info->interval_,
+   IntervalTimer::Callback cb = boost::bind(&TimerMgrImpl::timerCallback, this,
+                                            timer_name);
+   timer_info->interval_timer_.setup(cb, timer_info->interval_,
                                      timer_info->scheduling_mode_);
 }
 
 void
-TimerMgr::cancel(const std::string& timer_name) {
-
-    LOG_DEBUG(dhcpsrv_logger, DHCPSRV_DBG_TRACE,
-              DHCPSRV_TIMERMGR_STOP_TIMER)
-        .arg(timer_name);
+TimerMgrImpl::cancel(const std::string& timer_name) {
 
     // Find the timer of our interest.
     TimerInfoMap::const_iterator timer_info_it = registered_timers_.find(timer_name);
@@ -196,54 +440,45 @@ TimerMgr::cancel(const std::string& timer_name) {
     timer_info_it->second->watch_socket_.clearReady();
 }
 
-void
-TimerMgr::startThread() {
-    // Do not start the thread if the thread is already there.
-    if (!thread_) {
-        // Only log it if we really start the thread.
-        LOG_DEBUG(dhcpsrv_logger, DHCPSRV_DBG_TRACE,
-                  DHCPSRV_TIMERMGR_START_THREAD);
 
-        // The thread will simply run IOService::run(), which is a blocking call
-        // to keep running handlers for all timers according to how they have
-        // been scheduled.
-        thread_.reset(new Thread(boost::bind(&IOService::run, &getIOService())));
-    }
+bool
+TimerMgrImpl::threadRunning() const {
+    return (static_cast<bool>(thread_));
 }
 
 void
-TimerMgr::stopThread(const bool run_pending_callbacks) {
-    // If thread is not running, this is no-op.
-    if (thread_) {
-        // Only log it if we really have something to stop.
-        LOG_DEBUG(dhcpsrv_logger, DHCPSRV_DBG_TRACE,
-                  DHCPSRV_TIMERMGR_STOP_THREAD);
-
-        // Stop the IO Service. This will break the IOService::run executed in the
-        // worker thread. The thread will now terminate.
-        getIOService().stop();
-        // Some of the watch sockets may be already marked as ready and
-        // have some pending callbacks to be executed. If the caller
-        // wants us to run the callbacks we clear the sockets and run
-        // them. If pending callbacks shouldn't be executed, this will
-        // only clear the sockets (which should be substantially faster).
-        clearReadySockets(run_pending_callbacks);
-        // Wait for the thread to terminate.
-        thread_->wait();
-        // Set the thread pointer to NULL to indicate that the thread is not running.
-        thread_.reset();
-        // IO Service has to be reset so as we can call run() on it again if we
-        // desire (using the startThread()).
-        getIOService().get_io_service().reset();
-    }
+TimerMgrImpl::createThread() {
+    thread_.reset(new Thread(boost::bind(&IOService::run, &getIOService())));
 }
-IOService&
-TimerMgr::getIOService() const {
-    return (*io_service_);
+
+void
+TimerMgrImpl::controlledStopThread(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.
+    ScopedTrue scoped_true(stopping_, mutex_);
+
+    // Stop the IO Service. This will break the IOService::run executed in the
+    // worker thread. The thread will now terminate.
+    getIOService().stop();
+
+    // Some of the watch sockets may be already marked as ready and
+    // have some pending callbacks to be executed. If the caller
+    // wants us to run the callbacks we clear the sockets and run
+    // them. If pending callbacks shouldn't be executed, this will
+    // only clear the sockets (which should be substantially faster).
+    clearReadySockets(run_pending_callbacks);
+    // Wait for the thread to terminate.
+    thread_->wait();
+    // Set the thread pointer to NULL to indicate that the thread is not running.
+    thread_.reset();
+    // IO Service has to be reset so as we can call run() on it again if we
+    // desire (using the startThread()).
+    getIOService().get_io_service().reset();
 }
 
 void
-TimerMgr::timerCallback(const std::string& timer_name) {
+TimerMgrImpl::timerCallback(const std::string& timer_name) {
     // Find the specified timer setup.
     TimerInfoMap::iterator timer_info_it = registered_timers_.find(timer_name);
     if (timer_info_it != registered_timers_.end()) {
@@ -251,12 +486,33 @@ TimerMgr::timerCallback(const std::string& timer_name) {
         // interrupt the execution of the select() function. This is
         // executed from the worker thread.
         const TimerInfoPtr& timer_info = timer_info_it->second;
-        timer_info->watch_socket_.markReady();
+
+        // This function is called from the worker thread and we don't want
+        // the worker thread to get exceptions out of here. It is unlikely
+        // that markReady() would cause any problems but theoretically
+        // possible. Hence, we rather log an error and leave.
+        try {
+            timer_info->watch_socket_.markReady();
+
+        } catch (const std::exception& ex) {
+            LOG_ERROR(dhcpsrv_logger, DHCPSRV_TIMERMGR_SOCKET_MARK_FAILED)
+                .arg(timer_name)
+                .arg(ex.what());
+
+            // Do not wait for clearing the socket because we were unable
+            // to mark it ready anyway.
+            return;
+        }
+
+        // Blocking wait for the socket to be cleared on the other
+        // end. This may be interrupted both if the socket is cleared
+        // or if the stopThread() has been called on the TimerMgr.
+        waitForSocketClearing(timer_info->watch_socket_);
     }
 }
 
 void
-TimerMgr::ifaceMgrCallback(const std::string& timer_name) {
+TimerMgrImpl::ifaceMgrCallback(const std::string& timer_name) {
     // Find the specified timer setup.
     TimerInfoMap::iterator timer_info_it = registered_timers_.find(timer_name);
     if (timer_info_it != registered_timers_.end()) {
@@ -272,7 +528,7 @@ TimerMgr::ifaceMgrCallback(const std::string& timer_name) {
 }
 
 void
-TimerMgr::clearReadySockets(const bool run_pending_callbacks) {
+TimerMgrImpl::clearReadySockets(const bool run_pending_callbacks) {
     for (TimerInfoMap::iterator timer_info_it = registered_timers_.begin();
          timer_info_it != registered_timers_.end(); ++timer_info_it) {
         handleReadySocket(timer_info_it, run_pending_callbacks);
@@ -281,10 +537,8 @@ TimerMgr::clearReadySockets(const bool run_pending_callbacks) {
 
 template<typename Iterator>
 void
-TimerMgr::handleReadySocket(Iterator timer_info_iterator,
+TimerMgrImpl::handleReadySocket(Iterator timer_info_iterator,
                             const bool run_callback) {
-    timer_info_iterator->second->watch_socket_.clearReady();
-
     if (run_callback) {
         // Running user-defined operation for the timer. Logging it
         // on the slightly lower debug level as there may be many
@@ -292,9 +546,164 @@ TimerMgr::handleReadySocket(Iterator timer_info_iterator,
         LOG_DEBUG(dhcpsrv_logger, DHCPSRV_DBG_TRACE_DETAIL,
                   DHCPSRV_TIMERMGR_RUN_TIMER_OPERATION)
             .arg(timer_info_iterator->first);
-        timer_info_iterator->second->user_callback_();
+
+        std::string error_string;
+        try {
+            timer_info_iterator->second->user_callback_();
+
+        } catch (const std::exception& ex){
+            error_string = ex.what();
+
+        } catch (...) {
+            error_string = "unknown reason";
+        }
+
+        // Exception was thrown. Log an error.
+        if (!error_string.empty()) {
+            LOG_ERROR(dhcpsrv_logger, DHCPSRV_TIMERMGR_CALLBACK_FAILED)
+                .arg(timer_info_iterator->first)
+                .arg(error_string);
+        }
+    }
+
+    try {
+        // This shouldn't really fail, but if it does we want to log an
+        // error and make sure that the thread is not stuck waiting for
+        // the socket to clear.
+        timer_info_iterator->second->watch_socket_.clearReady();
+
+    } catch (const std::exception& ex) {
+        LOG_ERROR(dhcpsrv_logger, DHCPSRV_TIMERMGR_SOCKET_CLEAR_FAILED)
+            .arg(timer_info_iterator->first)
+            .arg(ex.what());
+    }
+
+    // Whether it succeeded or not, clear the socket to make sure that
+    // the worker thread is not stuck waiting for the main thread.
+    signalSocketClearing();
+}
+
+void
+TimerMgrImpl::waitForSocketClearing(WatchSocket& watch_socket) {
+    // Waiting for the specific socket to be cleared.
+    while (watch_socket.isReady()) {
+        Mutex::Locker lock(mutex_);
+        // If stopThread has been called there is no sense to further
+        // wait for the socket clearing. Leave from here to unblock the
+        // worker thread.
+        if (stopping_) {
+            break;
+        }
+        cond_var_.wait(mutex_);
+    }
+}
+
+void
+TimerMgrImpl::signalSocketClearing() {
+    cond_var_.signal();
+}
+
+TimerMgr&
+TimerMgr::instance() {
+    static TimerMgr timer_mgr;
+    return (timer_mgr);
+}
+
+TimerMgr::TimerMgr()
+    : impl_(new TimerMgrImpl()) {
+}
+
+TimerMgr::~TimerMgr() {
+    // Stop the thread, but do not unregister any timers. Unregistering
+    // the timers could cause static deinitialization fiasco between the
+    // TimerMgr and IfaceMgr. By now, the caller should have unregistered
+    // the timers.
+    stopThread();
+
+    delete impl_;
+}
+
+void
+TimerMgr::registerTimer(const std::string& timer_name,
+                        const IntervalTimer::Callback& callback,
+                        const long interval,
+                        const IntervalTimer::Mode& scheduling_mode) {
+
+    LOG_DEBUG(dhcpsrv_logger, DHCPSRV_DBG_TRACE,
+              DHCPSRV_TIMERMGR_REGISTER_TIMER)
+        .arg(timer_name)
+        .arg(interval);
+
+    impl_->registerTimer(timer_name, callback, interval, scheduling_mode);
+}
+
+void
+TimerMgr::unregisterTimer(const std::string& timer_name) {
+
+    LOG_DEBUG(dhcpsrv_logger, DHCPSRV_DBG_TRACE,
+              DHCPSRV_TIMERMGR_UNREGISTER_TIMER)
+        .arg(timer_name);
+
+    impl_->unregisterTimer(timer_name);
+}
+
+void
+TimerMgr::unregisterTimers() {
+
+    LOG_DEBUG(dhcpsrv_logger, DHCPSRV_DBG_TRACE,
+              DHCPSRV_TIMERMGR_UNREGISTER_ALL_TIMERS);
+
+    impl_->unregisterTimers();
+}
+
+void
+TimerMgr::setup(const std::string& timer_name) {
+
+    LOG_DEBUG(dhcpsrv_logger, DHCPSRV_DBG_TRACE,
+              DHCPSRV_TIMERMGR_START_TIMER)
+        .arg(timer_name);
+
+    impl_->setup(timer_name);
+}
+
+void
+TimerMgr::cancel(const std::string& timer_name) {
+
+    LOG_DEBUG(dhcpsrv_logger, DHCPSRV_DBG_TRACE,
+              DHCPSRV_TIMERMGR_STOP_TIMER)
+        .arg(timer_name);
+
+    impl_->cancel(timer_name);
+}
+
+void
+TimerMgr::startThread() {
+    // Do not start the thread if the thread is already there.
+    if (!impl_->threadRunning()) {
+        // Only log it if we really start the thread.
+        LOG_DEBUG(dhcpsrv_logger, DHCPSRV_DBG_TRACE,
+                  DHCPSRV_TIMERMGR_START_THREAD);
+
+        impl_->createThread();
+    }
+}
+
+void
+TimerMgr::stopThread(const bool run_pending_callbacks) {
+    // If thread is not running, this is no-op.
+    if (impl_->threadRunning()) {
+        // Only log it if we really have something to stop.
+        LOG_DEBUG(dhcpsrv_logger, DHCPSRV_DBG_TRACE,
+                  DHCPSRV_TIMERMGR_STOP_THREAD);
+
+        impl_->controlledStopThread(run_pending_callbacks);
     }
 }
+IOService&
+TimerMgr::getIOService() const {
+    return (impl_->getIOService());
+}
+
 
 } // end of namespace isc::dhcp
 } // end of namespace isc

+ 5 - 126
src/lib/dhcpsrv/timer_mgr.h

@@ -16,17 +16,16 @@
 #define TIMER_MGR_H
 
 #include <asiolink/interval_timer.h>
-#include <asiolink/io_service.h>
-#include <util/threads/thread.h>
-#include <util/watch_socket.h>
 #include <boost/noncopyable.hpp>
 #include <boost/scoped_ptr.hpp>
-#include <map>
 #include <string>
 
 namespace isc {
 namespace dhcp {
 
+/// @brief Forward declaration of the @c TimerMgr implementation.
+class TimerMgrImpl;
+
 /// @brief Manages a pool of asynchronous interval timers for DHCP server.
 ///
 /// This class holds a pool of asynchronous interval timers which are
@@ -256,129 +255,9 @@ private:
 
     //@}
 
-    /// @name Internal callbacks.
-    //@{
-    ///
-    /// @brief Callback function to be executed for each interval timer when
-    /// 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 TimerMgr::ifaceMgrCallback is executed from the main thread by the
-    /// @c IfaceMgr.
-    ///
-    /// @param timer_name Unique timer name to be passed to the callback.
-    void timerCallback(const std::string& timer_name);
-
-    /// @brief Callback function installed on the @c IfaceMgr and associated
-    /// with the particular timer.
-    ///
-    /// This callback function is executed by the @c IfaceMgr when the data
-    /// over the specific @c util::WatchSocket is received. This method clears
-    /// the socket (reads the data from the pipe) and executes the callback
-    /// supplied when the timer was registered.
-    ///
-    /// @param timer_name Unique timer name.
-    void ifaceMgrCallback(const std::string& timer_name);
-
-    //@}
-
-    /// @name Methods to handle ready sockets.
-    //@{
-    ///
-    /// @brief Clear ready sockets and optionally run callbacks.
-    ///
-    /// This method is called by the @c TimerMgr::stopThread method
-    /// to clear watch sockets which may be marked as ready. It will
-    /// also optionally run callbacks installed for the timers which
-    /// marked sockets as ready.
-    ///
-    /// @param run_pending_callbacks Indicates if the callbacks should
-    /// be executed for the sockets being cleared (if true).
-    void clearReadySockets(const bool run_pending_callbacks);
+    /// @brief Pointer to @c TimerMgr implementation.
+    TimerMgrImpl* impl_;
 
-    /// @brief Clears a socket and optionally runs a callback.
-    ///
-    /// This method clears the ready socket pointed to by the specified
-    /// iterator. If the @c run_callback is set, the callback will
-    /// also be executed.
-    ///
-    /// @param timer_info_iterator Iterator pointing to the timer
-    /// configuration structure.
-    /// @param run_callback Boolean value indicating if the callback
-    /// should be executed for the socket being cleared (if true).
-    ///
-    /// @tparam Iterator Iterator pointing to the timer configuration
-    /// structure.
-    template<typename Iterator>
-    void handleReadySocket(Iterator timer_info_iterator, const bool run_callback);
-
-    //@}
-
-    /// @brief Pointer to the io service.
-    asiolink::IOServicePtr io_service_;
-
-    /// @brief Pointer to the worker thread.
-    ///
-    /// This is initially set to NULL until the thread is started using the
-    /// @c TimerMgr::startThread. The @c TimerMgr::stopThread sets it back
-    /// to NULL.
-    boost::scoped_ptr<util::thread::Thread> thread_;
-
-    /// @brief Structure holding information for a single timer.
-    ///
-    /// This structure holds the instance of the watch socket being used to
-    /// signal that the timer is "ready". It also holds the instance of the
-    /// interval timer and other parameters pertaining to it.
-    struct TimerInfo {
-        /// @brief Instance of the watch socket.
-        util::WatchSocket watch_socket_;
-
-        /// @brief Instance of the interval timer.
-        asiolink::IntervalTimer interval_timer_;
-
-        /// @brief Holds the pointer to the callback supplied when registering
-        /// the timer.
-        asiolink::IntervalTimer::Callback user_callback_;
-
-        /// @brief Interval timer interval supplied during registration.
-        long interval_;
-
-        /// @brief Interval timer scheduling mode supplied during registration.
-        asiolink::IntervalTimer::Mode scheduling_mode_;
-
-        /// @brief Constructor.
-        ///
-        /// @param io_service Reference to the IO service to be used by the
-        /// interval timer created.
-        /// @param user_callback Pointer to the callback function supplied
-        /// during the timer registration.
-        /// @param interval Timer interval in milliseconds.
-        /// @param mode Interval timer scheduling mode.
-        TimerInfo(asiolink::IOService& io_service,
-                  const asiolink::IntervalTimer::Callback& user_callback,
-                  const long interval,
-                  const asiolink::IntervalTimer::Mode& mode)
-            : watch_socket_(),
-              interval_timer_(io_service),
-              user_callback_(user_callback),
-              interval_(interval),
-              scheduling_mode_(mode) { };
-    };
-
-    /// @brief A type definition for the pointer to @c TimerInfo structure.
-    typedef boost::shared_ptr<TimerInfo> TimerInfoPtr;
-
-    /// @brief A type definition for the map holding timers configuration.
-    typedef std::map<std::string, TimerInfoPtr> TimerInfoMap;
-
-    /// @brief Holds mapping of the timer name to the watch socket, timer
-    /// instance and other parameters pertaining to the timer.
-    ///
-    /// Each registered timer has a unique name which is used as a key to
-    /// 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