[PATCH] D18811: Fix a race condition in support library ThreadPool

[Mehdi AMINI via llvm-commits]

joker.eph added a comment.

One lock is simpler to understand, however it adds unnecessary contention, which is what I was trying to avoid originally.


================
Comment at: include/llvm/Support/ThreadPool.h:130
@@ -131,3 +129,3 @@
   /// Signal for the destruction of the pool, asking thread to exit.
-  bool EnableFlag;
+  bool InDestructorFlag;
 #endif
----------------
Is this related to the changes here? If not then split it out to a separate NFC patch, reducing the change in this patch to the bug it is actually fixing. Same for renaming `QueueCondition ` to `SubmissionCondition`.

================
Comment at: lib/Support/ThreadPool.cpp:79
@@ -78,3 +70,3 @@
   CompletionCondition.wait(LockGuard,
                            [&] { return Tasks.empty() && !ActiveThreads; });
 }
----------------
Would just reversing the check solve the race?

```
 [&] { return !ActiveThreads && Tasks.empty(); });
```

If not can you elaborate a little?


================
Comment at: lib/Support/ThreadPool.cpp:83-84
@@ -90,3 +82,4 @@
     // Don't allow enqueueing after disabling the pool
-    assert(EnableFlag && "Queuing a thread during ThreadPool destruction");
+    assert(!InDestructorFlag &&
+           "Queuing a thread during ThreadPool destruction");
 
----------------
I agree with jlebar that this scenario is sketchy:

```
{ 
  ThreadPool Pool;
  Pool.async([&] () {
     if (random()) Pool.async(doSomething);
  });
}
```

Technically it means that the client has to insert an explicit wait if a task can enqueue:

```
{ 
  ThreadPool Pool;
  Pool.async([&] () {
     if (random()) Pool.async(doSomething);
  });
  Pool.wait();
}
```

Synchronizing on a future inside a task would have the same kind of issue as well. 


http://reviews.llvm.org/D18811