Add TaskScheduler::JoinForTesting().

base/task_scheduler/scheduler_worker_pool_impl.h

Index: base/task_scheduler/scheduler_worker_pool_impl.h
diff --git a/base/task_scheduler/scheduler_worker_pool_impl.h b/base/task_scheduler/scheduler_worker_pool_impl.h
index 4693d635045a45c82c116008986095d9700c8680..66a27668d7d4595337b5a160e4e9168e78fa781a 100644
--- a/base/task_scheduler/scheduler_worker_pool_impl.h
+++ b/base/task_scheduler/scheduler_worker_pool_impl.h
@@ -17,7 +17,6 @@
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/strings/string_piece.h"
-#include "base/synchronization/atomic_flag.h"
 #include "base/synchronization/condition_variable.h"
 #include "base/task_scheduler/priority_queue.h"
 #include "base/task_scheduler/scheduler_lock.h"
@@ -97,14 +96,6 @@ class BASE_EXPORT SchedulerWorkerPoolImpl : public SchedulerWorkerPool {
   // allowed to complete their execution. This can only be called once.
   void JoinForTesting();
 
-  // Disallows worker thread detachment. If the suggested reclaim time is not
-  // TimeDelta::Max(), then the test should call this before the detach code can
-  // run. The safest place to do this is before the a set of work is dispatched
-  // (the worker pool is idle and steady state) or before the last
-  // synchronization point for all workers (all threads are busy and can't be
-  // reclaimed).
-  void DisallowWorkerDetachmentForTesting();

[robliao, 2017/01/17 19:01:27]

From what I can tell, we need to disallow detachment to properly handle joining.

Without this, workers may detach during the join. This means task scheduler
threads could still be running around after TaskScheduler joining (which is
undesirable for tests because tests that do leak checking after test
completion).

If threads detach before joining, they are less likely to hang around because
Joining is a "long" process".

[fdoray, 2017/01/17 20:43:13]

After JoinForTesting() returns, the thread is either:
- Joined, or,
- Past "detached_thread = outer_->Detach();" in
SchedulerWorker::Thread::ThreadMain()

I don't think it matters if a thread is still running after
SchedulerWorker::Join has returned if we know that it will exit shortly after
and that it can't run tasks anymore. It shouldn't break LSan because LSan simply
checks that all memory is referenced by global variables or variables on the
stack.

There will be unit tests for code that initializes TaskScheduler in a process
(required for https://codereview.chromium.org/2610493002/). There should be an
easy way to unregister TaskScheduler at the end of these tests (i.e. shouldn't
have to call DisallowWorkerDetachmentForTesting() from somewhere "safe").

[robliao, 2017/01/17 21:09:03]

Can JoinForTesting call DisallowWorkerDetachmentForTesting as part of its
process? That way callers don't have to worry about this.

[fdoray, 2017/01/17 23:51:00]

The hard part is making sure that DisallowWorkerDetachmentForTesting is called
at the right time (see
https://cs.chromium.org/chromium/src/base/task_scheduler/scheduler_worker_poo...).
With the current CL, you can call JoinForTesting() whenever you want.

[robliao, 2017/01/18 00:58:19]

With this code change, you can now call it at any time.

Previously, we would could deadlock during the detach call because we hold the
lock during the entirety of a Join call. This is no longer the case with this
code as we do not hold the lock during Join. This means an attempt to Detach
during a Join call will simply return nullptr.

An attempt to Join during a detach similar will cause the Join to immediately
return.

[fdoray, 2017/01/19 16:37:40]

What would be the benefit of calling DisallowWorkerDetachmentForTesting(); from
JoinForTesting()? JoinForTesting() already prevents detachment when it takes
ownership of |thread_|.

[robliao, 2017/01/19 16:57:29]

The intent here would be to first do a disallow detachment pass first and then
perform the subsequent joins. This reduces the likelihood that threads are
detaching during the joining of any single thread and allows us to join and
clear threads than we otherwise would have.

-
   // Returns the number of workers alive in this worker pool. The value may
   // change if workers are woken up or detached during this call.
   size_t NumberOfAliveWorkersForTesting();
@@ -139,9 +130,6 @@ class BASE_EXPORT SchedulerWorkerPoolImpl : public SchedulerWorkerPool {
   // Removes |worker| from |idle_workers_stack_|.
   void RemoveFromIdleWorkersStack(SchedulerWorker* worker);
 
-  // Returns true if worker thread detachment is permitted.
-  bool CanWorkerDetachForTesting();
-
   // The name of this worker pool, used to label its worker threads.
   const std::string name_;
 
@@ -182,10 +170,6 @@ class BASE_EXPORT SchedulerWorkerPoolImpl : public SchedulerWorkerPool {
   // Signaled once JoinForTesting() has returned.
   WaitableEvent join_for_testing_returned_;
 
-  // Indicates to the delegates that workers are not permitted to detach their
-  // threads.
-  AtomicFlag worker_detachment_disallowed_;
-
 #if DCHECK_IS_ON()
   // Signaled when all workers have been created.
   WaitableEvent workers_created_;