SchedulerWorker Refcounting for Destruction in Production

base/task_scheduler/scheduler_worker.h

Index: base/task_scheduler/scheduler_worker.h
diff --git a/base/task_scheduler/scheduler_worker.h b/base/task_scheduler/scheduler_worker.h
index f8174de971761910533b463730d993642bd2093f..6a9ba001d91e031091fa174de124f03f58b4acf5 100644
--- a/base/task_scheduler/scheduler_worker.h
+++ b/base/task_scheduler/scheduler_worker.h
@@ -37,7 +37,8 @@ class TaskTracker;
 // guidance from the delegate.
 //
 // This class is thread-safe.
-class BASE_EXPORT SchedulerWorker {
+class BASE_EXPORT SchedulerWorker
+    : public RefCountedThreadSafe<SchedulerWorker> {
  public:
   // Delegate interface for SchedulerWorker. The methods are always called from
   // the thread managed by the SchedulerWorker instance.
@@ -96,7 +97,7 @@ class BASE_EXPORT SchedulerWorker {
   // Returns nullptr if creating the underlying platform thread fails during
   // Create(). |backward_compatibility| indicates whether backward compatibility
   // is enabled.
-  static std::unique_ptr<SchedulerWorker> Create(
+  static scoped_refptr<SchedulerWorker> Create(
       ThreadPriority priority_hint,
       std::unique_ptr<Delegate> delegate,
       TaskTracker* task_tracker,
@@ -104,11 +105,6 @@ class BASE_EXPORT SchedulerWorker {
       SchedulerBackwardCompatibility backward_compatibility =
           SchedulerBackwardCompatibility::DISABLED);
 
-  // Destroying a SchedulerWorker in production is not allowed; it is always
-  // leaked. In tests, it can only be destroyed after JoinForTesting() has
-  // returned.
-  ~SchedulerWorker();
-
   // Wakes up this SchedulerWorker if it wasn't already awake. After this
   // is called, this SchedulerWorker will run Tasks from Sequences
   // returned by the GetWork() method of its delegate until it returns nullptr.
@@ -129,27 +125,49 @@ class BASE_EXPORT SchedulerWorker {
   // Returns true if the worker is alive.
   bool ThreadAliveForTesting() const;
 
+  // Makes a request to terminate the worker. This may be called from any
+  // thread. All further method calls are undefined after this calling this
+  // function.
+  void RequestThreadTermination();
+
  private:
+  friend class RefCountedThreadSafe<SchedulerWorker>;
   class Thread;
+  enum class DetachNotify {
+    // Do not notify any component.
+    SILENT,
+    // Notify the delegate.
+    DELEGATE,
+  };
 
   SchedulerWorker(ThreadPriority thread_priority,
                   std::unique_ptr<Delegate> delegate,
                   TaskTracker* task_tracker,
                   SchedulerBackwardCompatibility backward_compatibility);
+  ~SchedulerWorker();
 
-  // Returns the thread instance if the detach was successful so that it can be
-  // freed upon termination of the thread.
-  // If the detach is not possible, returns nullptr.
-  std::unique_ptr<SchedulerWorker::Thread> Detach();
+  // Returns ownership of the thread instance when appropriate so that it can be
+  // freed upon termination of the thread. If ownership transfer is not
+  // possible, returns nullptr.
+  std::unique_ptr<SchedulerWorker::Thread> DetachThreadObject(
+      DetachNotify detach_notify);
 
   void CreateThread();
 
   void CreateThreadAssertSynchronized();
 
-  // Synchronizes access to |thread_|.
+  bool ShouldExit();
+
+  // Synchronizes access to |thread_| and |should_exit_|.
   mutable SchedulerLock thread_lock_;
 
+  // AtomicFlag isn't used here as |should_exit_| may be set by any thread.
+  bool should_exit_ = false;

[fdoray, 2017/02/17 16:43:41]

Since it is only set once [1], the fact that it may be set from any thread
shouldn't be a problem. It doesn't have to be set from the sequence from which
it was constructed
https://cs.chromium.org/chromium/src/base/synchronization/atomic_flag.cc?sq=p...

[1] Enforced by the DHCECK(!should_exit_) in ShouldExit().

[robliao, 2017/02/17 19:24:36]

Gotcha. I must have misread something somewhere as an earlier iteration did
assert here.

+
   // The underlying thread for this SchedulerWorker.
+  // The thread object will be cleaned up by the running thread unless we join
+  // against the thread. Joining requires the thread object to remain alive for
+  // the Thread::Join() call.
   std::unique_ptr<Thread> thread_;
 
   const ThreadPriority priority_hint_;
@@ -162,7 +180,7 @@ class BASE_EXPORT SchedulerWorker {
 #endif
 
   // Set once JoinForTesting() has been called.
-  AtomicFlag should_exit_for_testing_;
+  AtomicFlag join_called_for_testing_;
 
   DISALLOW_COPY_AND_ASSIGN(SchedulerWorker);
 };