Add TaskScheduler::JoinForTesting().

base/task_scheduler/scheduler_worker_pool_impl.h

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef BASE_TASK_SCHEDULER_SCHEDULER_WORKER_POOL_IMPL_H_
 #define BASE_TASK_SCHEDULER_SCHEDULER_WORKER_POOL_IMPL_H_
 
 #include <stddef.h>
 
 #include <memory>
 #include <string>
 #include <vector>
 
 #include "base/base_export.h"
 #include "base/callback.h"
 #include "base/logging.h"
 #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"
 #include "base/task_scheduler/scheduler_worker.h"
 #include "base/task_scheduler/scheduler_worker_pool.h"
 #include "base/task_scheduler/scheduler_worker_pool_params.h"
 #include "base/task_scheduler/scheduler_worker_stack.h"
 #include "base/task_scheduler/sequence.h"
 #include "base/task_scheduler/task.h"
 #include "base/threading/platform_thread.h"
@@ skipping 59 matching lines @@
 
   void GetHistograms(std::vector<const HistogramBase*>* histograms) const;
 
   // Waits until all workers are idle.
   void WaitForAllWorkersIdleForTesting();
 
   // Joins all workers of this worker pool. Tasks that are already running are
   // 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();
 
  private:
   class SchedulerSingleThreadTaskRunner;
   class SchedulerWorkerDelegateImpl;
 
   SchedulerWorkerPoolImpl(StringPiece name,
                           TimeDelta suggested_reclaim_time,
@@ skipping 14 matching lines @@
 
   // Adds |worker| to |idle_workers_stack_|.
   void AddToIdleWorkersStack(SchedulerWorker* worker);
 
   // Peeks from |idle_workers_stack_|.
   const SchedulerWorker* PeekAtIdleWorkersStack() const;
 
   // 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_;
 
   // All worker owned by this worker pool. Only modified during initialization
   // of the worker pool.
   std::vector<std::unique_ptr<SchedulerWorker>> workers_;
 
   // Synchronizes access to |next_worker_index_|.
   SchedulerLock next_worker_index_lock_;
 
@@ skipping 20 matching lines @@
   // timeout expires, even if its WakeUp() method hasn't been called). A worker
   // is pushed on this stack when it receives nullptr from GetWork().
   SchedulerWorkerStack idle_workers_stack_;
 
   // Signaled when all workers become idle.
   std::unique_ptr<ConditionVariable> idle_workers_stack_cv_for_testing_;
 
   // 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_;
 #endif
 
   // TaskScheduler.DetachDuration.[worker pool name] histogram. Intentionally
   // leaked.
   HistogramBase* const detach_duration_histogram_;
 
   // TaskScheduler.NumTasksBeforeDetach.[worker pool name] histogram.
   // Intentionally leaked.
   HistogramBase* const num_tasks_before_detach_histogram_;
 
   // TaskScheduler.NumTasksBetweenWaits.[worker pool name] histogram.
   // Intentionally leaked.
   HistogramBase* const num_tasks_between_waits_histogram_;
 
   TaskTracker* const task_tracker_;
   DelayedTaskManager* const delayed_task_manager_;
 
   DISALLOW_COPY_AND_ASSIGN(SchedulerWorkerPoolImpl);
 };
 
 }  // namespace internal
 }  // namespace base
 
 #endif  // BASE_TASK_SCHEDULER_SCHEDULER_WORKER_POOL_IMPL_H_