From 9e8b86bb9ef84d6214a9ea2110cf6ed83f4ddcd3 Mon Sep 17 00:00:00 2001
From: Boris Kolpackov <boris@codesynthesis.com>
Date: Mon, 1 Jan 2018 11:49:05 +0200
Subject: Fix race in scheduler progress setting logic

---
 build2/operation.cxx |  2 +-
 build2/scheduler.cxx | 40 +++++++++++++++++++++++++++-------------
 build2/scheduler.hxx | 10 +++++++++-
 3 files changed, 37 insertions(+), 15 deletions(-)

diff --git a/build2/operation.cxx b/build2/operation.cxx
index 62f82d3..82b546c 100644
--- a/build2/operation.cxx
+++ b/build2/operation.cxx
@@ -289,7 +289,7 @@ namespace build2
         mg = sched.monitor (
           target_count,
           init - incr,
-          [init, incr, what] (size_t c) -> size_t
+          [init, incr, &what] (size_t c) -> size_t
           {
             size_t p ((init - c) * 100 / init);
             size_t s (skip_count.load (memory_order_relaxed));
diff --git a/build2/scheduler.cxx b/build2/scheduler.cxx
index 51863ab..fa46dbe 100644
--- a/build2/scheduler.cxx
+++ b/build2/scheduler.cxx
@@ -186,6 +186,23 @@ namespace build2
     try { shutdown (); } catch (system_error&) {}
   }
 
+  void scheduler::
+  wait_idle ()
+  {
+    lock l (mutex_);
+
+    while (active_ != init_active_)
+    {
+      l.unlock ();
+      this_thread::yield ();
+      l.lock ();
+    }
+
+    assert (waiting_ == 0);
+    assert (ready_ == 0);
+    assert (starting_ == 0);
+  }
+
   size_t scheduler::
   shard_size (size_t mul, size_t div) const
   {
@@ -294,8 +311,6 @@ namespace build2
   void scheduler::
   tune (size_t max_active)
   {
-    lock l (mutex_);
-
     if (max_active == 0)
       max_active = orig_max_active_;
 
@@ -305,15 +320,7 @@ namespace build2
     // The scheduler must not be active though some threads might still be
     // comming off from finishing a task. So we busy-wait for them.
     //
-    while (active_ != init_active_)
-    {
-      l.unlock ();
-      this_thread::yield ();
-      l.lock ();
-    }
-
-    assert (waiting_ == 0);
-    assert (ready_ == 0);
+    wait_idle ();
 
     max_active_ = max_active;
   }
@@ -400,10 +407,18 @@ namespace build2
   monitor (atomic_count& c, size_t t, function<size_t (size_t)> f)
   {
     assert (monitor_count_ == nullptr && t != 0);
+
+    // While the scheduler must not be active, some threads might still be
+    // comming off from finishing a task and trying to report progress. So we
+    // busy-wait for them (also in ~monitor_guard()).
+    //
+    wait_idle ();
+
     monitor_count_ = &c;
     monitor_tshold_.store (t, memory_order_relaxed);
     monitor_init_ = c.load (memory_order_relaxed);
     monitor_func_ = move (f);
+
     return monitor_guard (this);
   }
 
@@ -627,8 +642,7 @@ namespace build2
           s.ready_condv_.notify_one ();
       }
 
-      // Become idle and wait for a notification (note that task_ is false
-      // here).
+      // Become idle and wait for a notification.
       //
       s.idle_++;
       s.idle_condv_.wait (l);
diff --git a/build2/scheduler.hxx b/build2/scheduler.hxx
index f79ca3e..20e5128 100644
--- a/build2/scheduler.hxx
+++ b/build2/scheduler.hxx
@@ -160,6 +160,7 @@ namespace build2
     // If the maximum threads or task queue depth arguments are unspecified,
     // then appropriate defaults are used.
     //
+    explicit
     scheduler (size_t max_active,
                size_t init_active = 1,
                size_t max_threads = 0,
@@ -197,7 +198,7 @@ namespace build2
     void
     tune (size_t max_active);
 
-    // Return true if the scheduler is running serial.
+    // Return true if the scheduler is configured to run tasks serially.
     //
     // Note: can only be called from threads that have observed startup.
     //
@@ -255,6 +256,7 @@ namespace build2
       {
         if (s_ != nullptr)
         {
+          s_->wait_idle (); // See monitor() for details.
           s_->monitor_count_ = nullptr;
           s_->monitor_func_  = nullptr;
         }
@@ -298,6 +300,12 @@ namespace build2
       task_queue_ = nullptr;
     }
 
+    // Assuming all the task have been executed, busy-wait for all the threads
+    // to become idle. Normally you don't need to call this function directly.
+    //
+    void
+    wait_idle ();
+
     // Return the number of hardware threads or 0 if unable to determine.
     //
     static size_t
-- 
cgit v1.1