From 2a04dd5cc10cfd28e52d852220f336121cb276ca Mon Sep 17 00:00:00 2001
From: Tom <git@thomastrapp.com>
Date: Mon, 23 Sep 2019 12:04:28 +0200
Subject: [PATCH] AtomicCondition: fix race condition, remove notify_{one,all}

* Fix race condition in wait_for between calling atomic load
  and condition_variable::wait_for, by protecting atomic store
  with same mutex as AtomicCondition::wait_for.
* Subsequently, simplify the interface by removing notify_{one,all}.
  condition_varaiable::notify_all is now automatically called in
  `AtomicCondition::set(val)`.
---
 include/sgnl/AtomicCondition.h | 29 ++++++++++++++---------------
 include/sgnl/SignalHandler.h   |  1 -
 test/test.cpp                  |  2 --
 3 files changed, 14 insertions(+), 18 deletions(-)

diff --git a/include/sgnl/AtomicCondition.h b/include/sgnl/AtomicCondition.h
index 7e30b28..80ed484 100644
--- a/include/sgnl/AtomicCondition.h
+++ b/include/sgnl/AtomicCondition.h
@@ -18,7 +18,7 @@ class AtomicCondition
 public:
   explicit AtomicCondition(ValueType val)
   : value_(val)
-  , condvar_mutex_()
+  , mutex_()
   , condvar_()
   {
   }
@@ -30,14 +30,23 @@ public:
 
   void set(ValueType val) noexcept
   {
-    this->value_.store(val);
+    {
+      // This ensures that wait_for is either
+      // 1. not running, or
+      // 2. in a waiting state
+      // to avoid a data race between value_.load and cond_var.wait_for.
+      std::unique_lock<std::mutex> lock(this->mutex_);
+      this->value_.store(val);
+    }
+
+    this->condvar_.notify_all();
   }
 
   template<class Rep, class Period>
   void wait_for(const std::chrono::duration<Rep, Period>& time,
                 ValueType val) const
   {
-    std::unique_lock<std::mutex> lock(this->condvar_mutex_);
+    std::unique_lock<std::mutex> lock(this->mutex_);
 
     while( this->value_.load() != val )
       if( this->condvar_.wait_for(lock, time) == std::cv_status::timeout )
@@ -48,26 +57,16 @@ public:
   void wait_for(const std::chrono::duration<Rep, Period>& time,
                 Predicate pred) const
   {
-    std::unique_lock<std::mutex> lock(this->condvar_mutex_);
+    std::unique_lock<std::mutex> lock(this->mutex_);
 
     while( !pred() )
       if( this->condvar_.wait_for(lock, time) == std::cv_status::timeout )
         return;
   }
 
-  void notify_one() noexcept
-  {
-    this->condvar_.notify_one();
-  }
-
-  void notify_all() noexcept
-  {
-    this->condvar_.notify_all();
-  }
-
 private:
   std::atomic<ValueType> value_;
-  mutable std::mutex condvar_mutex_;
+  mutable std::mutex mutex_;
   mutable std::condition_variable condvar_;
 };
 
diff --git a/include/sgnl/SignalHandler.h b/include/sgnl/SignalHandler.h
index 04410d1..ee7ca92 100644
--- a/include/sgnl/SignalHandler.h
+++ b/include/sgnl/SignalHandler.h
@@ -58,7 +58,6 @@ public:
           it != this->signal_map_.end() )
       {
         this->condition_.set(it->second);
-        this->condition_.notify_all();
         return it->first;
       }
     }
diff --git a/test/test.cpp b/test/test.cpp
index 751993c..9ccd3f8 100644
--- a/test/test.cpp
+++ b/test/test.cpp
@@ -58,7 +58,6 @@ TEST_CASE("wait-for-value")
   std::this_thread::yield();
 
   condition.set(42);
-  condition.notify_one();
   future.get();
 
   REQUIRE( condition.get() == 42 );
@@ -79,7 +78,6 @@ TEST_CASE("wait-for-predicate")
   std::this_thread::yield();
 
   condition.set(42);
-  condition.notify_one();
   future.get();
 
   REQUIRE( condition.get() == 42 );