Fixing service_executor

The destructor and thread_wrapper of the service_executor might race for completion. This patch ensures that the thread_wrapper always completes before the destructor
STEllAR-GROUP · Sep 12, 2017 · 2c6e61f · 2c6e61f
1 parent 61eefd2
commit 2c6e61f
Show file tree

Hide file tree

Showing 2 changed files with 18 additions and 7 deletions.
diff --git a/hpx/runtime/threads/executors/service_executors.hpp b/hpx/runtime/threads/executors/service_executors.hpp
@@ -8,7 +8,7 @@
 
 #include <hpx/config.hpp>
 #include <hpx/exception_fwd.hpp>
-#include <hpx/lcos/local/counting_semaphore.hpp>
+#include <hpx/lcos/local/condition_variable.hpp>
 #include <hpx/runtime/threads/thread_enums.hpp>
 #include <hpx/runtime/threads/thread_executor.hpp>
 #include <hpx/throw_exception.hpp>
@@ -73,8 +73,10 @@ namespace hpx { namespace threads { namespace executors
 
         private:
             util::io_service_pool* pool_;
+            typedef hpx::lcos::local::spinlock mutex_type;
+            mutex_type mtx_;
             std::atomic<std::uint64_t> task_count_;
-            lcos::local::counting_semaphore shutdown_sem_;
+            lcos::local::condition_variable_any shutdown_cv_;
         };
     }
 

diff --git a/src/runtime/threads/executors/service_executor.cpp b/src/runtime/threads/executors/service_executor.cpp
@@ -31,7 +31,7 @@ namespace hpx { namespace threads { namespace executors { namespace detail
     service_executor::service_executor(
             char const* pool_name, char const* pool_name_suffix)
       : pool_(get_thread_pool(pool_name, pool_name_suffix)),
-        task_count_(0), shutdown_sem_(0)
+        task_count_(0)
     {
         if (!pool_) {
             HPX_THROW_EXCEPTION(bad_parameter,
@@ -42,16 +42,25 @@ namespace hpx { namespace threads { namespace executors { namespace detail
 
     service_executor::~service_executor()
     {
-        if (task_count_ != 0)
-            shutdown_sem_.wait();
+        std::unique_lock<mutex_type> l(mtx_);
+        while (task_count_ != 0)
+        {
+            shutdown_cv_.wait(l);
+        }
     }
 
     void service_executor::thread_wrapper(closure_type&& f) //-V669
     {
         f();                          // execute the actual thread function
 
-        if (--task_count_ == 0)
-            shutdown_sem_.signal();
+        // By hanging on to the lock during notify_all, we ensure that the
+        // destructor is only completed after this function returned
+        std::unique_lock<mutex_type> l(mtx_);
+        {
+            HPX_ASSERT(task_count_ > 0);
+            if (--task_count_ == 0)
+                shutdown_cv_.notify_all();
+        }
     }
 
     struct thread_wrapper_helper