Merge pull request #473 from anosach-luxoft/fix/Add-lock-to-timer

Protect Start/Stop methods in timer

2 files changed, 27 insertions, 17 deletions

diff --git a/src/components/utils/include/utils/timer.h b/src/components/utils/include/utils/timer.h
index 1def5e6b7a..69b2d08a4d 100644
--- a/src/components/utils/include/utils/timer.h
+++ b/src/components/utils/include/utils/timer.h
@@ -47,13 +47,14 @@ typedef uint32_t Milliseconds;
 
 /**
  * @brief Timer calls custom callback function after
- * specified timeout has been elapsed
+ * specified timeout has been elapsed.
+ * Thread-safe class
  */
 class Timer {
  public:
   /**
    * @brief Constructor
-   * Does not starts timer
+   * Does not start timer
    * @param name Timer name for identity
    * @param task Task for tracking
    */
@@ -104,28 +105,24 @@ class Timer {
 
     /**
      * @brief Sets timer timeout
-     * Thread-safe method
      * @param timeout Timeout in milliseconds to be set
      */
     void set_timeout(const Milliseconds timeout);
 
     /**
      * @brief Gets timer timeout
-     * Thread-safe method
      * @return Timer timeout
      */
     Milliseconds timeout() const;
 
     /**
      * @brief Sets timer delegate stop flag
-     * Thread-safe method
      * @param stop_flag Bool flag to be set
      */
     void set_stop_flag(const bool stop_flag);
 
     /**
      * @brief Gets timer delegate stop flag
-     * Thread-safe method
      * @return Delegate stop flag
      */
     bool stop_flag() const;
@@ -152,6 +149,8 @@ class Timer {
     DISALLOW_COPY_AND_ASSIGN(TimerDelegate);
   };
 
+  void StopUnsafe();
+
   /**
    * @brief Callback called on timeout
    */
@@ -165,6 +164,10 @@ class Timer {
   mutable sync_primitives::Lock task_lock_;
   TimerTask* task_;
 
+  /*
+   * State lock used to protect thread and delegate
+   */
+  sync_primitives::Lock state_lock_;
   mutable TimerDelegate delegate_;
   threads::Thread* thread_;
 
diff --git a/src/components/utils/src/timer.cc b/src/components/utils/src/timer.cc
index 543c3c5b06..99141d61b7 100644
--- a/src/components/utils/src/timer.cc
+++ b/src/components/utils/src/timer.cc
@@ -59,10 +59,11 @@ timer::Timer::Timer(const std::string& name, TimerTask* task)
 
 timer::Timer::~Timer() {
   LOG4CXX_AUTO_TRACE(logger_);
-  Stop();
+  sync_primitives::AutoLock state_auto_lock(state_lock_);
+  StopUnsafe();
   DCHECK(thread_);
   DeleteThread(thread_);
-  sync_primitives::AutoLock auto_lock(task_lock_);
+  sync_primitives::AutoLock task_auto_lock(task_lock_);
   DCHECK(task_);
   delete task_;
   LOG4CXX_DEBUG(logger_, "Timer " << name_ << " has been destroyed");
@@ -70,8 +71,9 @@ timer::Timer::~Timer() {
 
 void timer::Timer::Start(const Milliseconds timeout, const bool single_shot) {
   LOG4CXX_AUTO_TRACE(logger_);
+  sync_primitives::AutoLock auto_lock(state_lock_);
+  StopUnsafe();
   DCHECK_OR_RETURN_VOID(thread_);
-  Stop();
   delegate_.set_timeout(timeout);
   single_shot_ = single_shot;
   thread_->start();
@@ -80,14 +82,8 @@ void timer::Timer::Start(const Milliseconds timeout, const bool single_shot) {
 }
 
 void timer::Timer::Stop() {
-  LOG4CXX_AUTO_TRACE(logger_);
-  DCHECK_OR_RETURN_VOID(thread_);
-  delegate_.set_stop_flag(true);
-  if (!thread_->IsCurrentThread()) {
-    thread_->join();
-  }
-  delegate_.set_timeout(0);
-  LOG4CXX_DEBUG(logger_, "Timer " << name_ << " has been stopped");
+  sync_primitives::AutoLock auto_lock(state_lock_);
+  StopUnsafe();
 }
 
 bool timer::Timer::is_running() const {
@@ -98,6 +94,17 @@ timer::Milliseconds timer::Timer::timeout() const {
   return delegate_.timeout();
 }
 
+void timer::Timer::StopUnsafe() {
+  LOG4CXX_AUTO_TRACE(logger_);
+  DCHECK_OR_RETURN_VOID(thread_);
+  delegate_.set_stop_flag(true);
+  if (!thread_->IsCurrentThread()) {
+    thread_->join();
+  }
+  delegate_.set_timeout(0);
+  LOG4CXX_DEBUG(logger_, "Timer " << name_ << " has been stopped");
+}
+
 void timer::Timer::OnTimeout() const {
   LOG4CXX_AUTO_TRACE(logger_);
   delegate_.set_stop_flag(single_shot_);