DESIGN DISCUSSION ONLY Task Scheduler Single Thread Task Runner Manager for Dedicated Threads per S…

base/task_scheduler/scheduler_worker_unittest.cc

 // 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.
 
 #include "base/task_scheduler/scheduler_worker.h"
 
 #include <stddef.h>
 
 #include <memory>
 #include <vector>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/synchronization/condition_variable.h"
 #include "base/synchronization/waitable_event.h"
 #include "base/task_scheduler/scheduler_lock.h"
 #include "base/task_scheduler/sequence.h"
 #include "base/task_scheduler/task.h"
 #include "base/task_scheduler/task_tracker.h"
+#include "base/test/test_timeouts.h"
 #include "base/threading/platform_thread.h"
 #include "base/time/time.h"
 #include "build/build_config.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 #if defined(OS_WIN)
 #include <objbase.h>
 #endif
 
@@ skipping 316 matching lines @@
                         TaskSchedulerWorkerTest,
                         ::testing::Values(1));
 INSTANTIATE_TEST_CASE_P(TwoTasksPerSequence,
                         TaskSchedulerWorkerTest,
                         ::testing::Values(2));
 
 namespace {
 
 class ControllableDetachDelegate : public SchedulerWorkerDefaultDelegate {
  public:
-  ControllableDetachDelegate(TaskTracker* task_tracker)
-      : task_tracker_(task_tracker),
-        work_processed_(WaitableEvent::ResetPolicy::MANUAL,
+  ControllableDetachDelegate()
+      : work_processed_(WaitableEvent::ResetPolicy::MANUAL,
                         WaitableEvent::InitialState::NOT_SIGNALED),
         detach_requested_(WaitableEvent::ResetPolicy::MANUAL,
                           WaitableEvent::InitialState::NOT_SIGNALED),
         detached_(WaitableEvent::ResetPolicy::MANUAL,
-                  WaitableEvent::InitialState::NOT_SIGNALED) {
-    EXPECT_TRUE(task_tracker_);
-  }
+                  WaitableEvent::InitialState::NOT_SIGNALED) {}
 
   ~ControllableDetachDelegate() override = default;
 
   // SchedulerWorker::Delegate:
   MOCK_METHOD1(OnMainEntry, void(SchedulerWorker* worker));
 
   scoped_refptr<Sequence> GetWork(SchedulerWorker* worker)
       override {
     // Sends one item of work to signal |work_processed_|. On subsequent calls,
     // sends nullptr to indicate there's no more work to be done.
     if (work_requested_)
       return nullptr;
 
     work_requested_ = true;
     scoped_refptr<Sequence> sequence(new Sequence);
     std::unique_ptr<Task> task(new Task(
         FROM_HERE, Bind(&WaitableEvent::Signal, Unretained(&work_processed_)),
         TaskTraits(), TimeDelta()));
-    EXPECT_TRUE(task_tracker_->WillPostTask(task.get()));
+    EXPECT_TRUE(task_tracker_.WillPostTask(task.get()));
     sequence->PushTask(std::move(task));
     return sequence;
   }
 
   void DidRunTask() override {}
 
   bool CanDetach(SchedulerWorker* worker) override {
     detach_requested_.Signal();
     return can_detach_;
   }
@@ skipping 15 matching lines @@
   void WaitForDetach() { detached_.Wait(); }
 
   void ResetState() {
     work_requested_ = false;
     work_processed_.Reset();
     detach_requested_.Reset();
   }
 
   void set_can_detach(bool can_detach) { can_detach_ = can_detach; }
 
+  // This delegate owns the TaskTracker because during detachment, threads may
+  // outlive the test case.

[fdoray, 2017/02/08 17:59:02]

I think this is no longer true if you do
https://codereview.chromium.org/2686593003/diff/20001/base/task_scheduler/sch...

WorkerDetaches, WorkerDetachesAndSelfDestroys: The thread is detached at the end
of the test. SchedulerWorker does not reference its TaskTracker after a detach
https://cs.chromium.org/chromium/src/base/task_scheduler/scheduler_worker.cc?....

WorkerDetachesAndWakes, WorkerDetachesAndWakes: The thread is joined at the end
of the test.

[robliao, 2017/02/11 02:13:34]

That is true. Removed.

+  TaskTracker* task_tracker() { return &task_tracker_; }
+
  private:
-  TaskTracker* const task_tracker_;
+  TaskTracker task_tracker_;
   bool work_requested_ = false;
   bool can_detach_ = false;
   WaitableEvent work_processed_;
   WaitableEvent detach_requested_;
   WaitableEvent detached_;
 
   DISALLOW_COPY_AND_ASSIGN(ControllableDetachDelegate);
 };
 
 }  // namespace
 
 TEST(TaskSchedulerWorkerTest, WorkerDetaches) {
-  TaskTracker task_tracker;
   // Will be owned by SchedulerWorker.
   ControllableDetachDelegate* delegate =
-      new StrictMock<ControllableDetachDelegate>(&task_tracker);
+      new StrictMock<ControllableDetachDelegate>();
   delegate->set_can_detach(true);
   EXPECT_CALL(*delegate, OnMainEntry(_));
-  std::unique_ptr<SchedulerWorker> worker =
-      SchedulerWorker::Create(
-          ThreadPriority::NORMAL, WrapUnique(delegate), &task_tracker,
-          SchedulerWorker::InitialState::ALIVE);
+  std::unique_ptr<SchedulerWorker> worker = SchedulerWorker::Create(
+      ThreadPriority::NORMAL, WrapUnique(delegate), delegate->task_tracker(),
+      SchedulerWorker::InitialState::ALIVE);
   worker->WakeUp();
   delegate->WaitForWorkToRun();
   Mock::VerifyAndClear(delegate);
   delegate->WaitForDetachRequest();
   delegate->WaitForDetach();
   ASSERT_FALSE(worker->ThreadAliveForTesting());
 }
 
-TEST(TaskSchedulerWorkerTest, WorkerDetachesAndWakes) {
-  TaskTracker task_tracker;
+TEST(TaskSchedulerWorkerTest, WorkerDetachesAndSelfDestroys) {
+  // This test shouldn't crash or leak resources.
   // Will be owned by SchedulerWorker.
   ControllableDetachDelegate* delegate =
-      new StrictMock<ControllableDetachDelegate>(&task_tracker);
+      new StrictMock<ControllableDetachDelegate>();
   delegate->set_can_detach(true);
   EXPECT_CALL(*delegate, OnMainEntry(_));
-  std::unique_ptr<SchedulerWorker> worker =
-      SchedulerWorker::Create(
-          ThreadPriority::NORMAL, WrapUnique(delegate), &task_tracker,
-          SchedulerWorker::InitialState::ALIVE);
+  std::unique_ptr<SchedulerWorker> worker = SchedulerWorker::Create(
+      ThreadPriority::NORMAL, WrapUnique(delegate), delegate->task_tracker(),
+      SchedulerWorker::InitialState::ALIVE);
+  worker->WakeUp();
+  SchedulerWorker::DestroyAfterDetachment(std::move(worker));
+  // Give a chance for the other thread to clean up.

[fdoray, 2017/02/08 17:59:02]

You could signal an event in ~ControllableDetachDelegate() and wait for it here.
That way, the test would hang if the SchedulerWorker isn't deleted as expected
(i.e. an error could be catched by a non-ASAN bot). Of course, the event would
be owned by the test body and ControllableDetachDelegate would just have a raw
pointer to it.

[robliao, 2017/02/11 02:13:34]

I went with a refcounted event instead.

+  PlatformThread::Sleep(TestTimeouts::tiny_timeout());
+}
+
+TEST(TaskSchedulerWorkerTest, WorkerDetachesAndWakes) {
+  // Will be owned by SchedulerWorker.
+  ControllableDetachDelegate* delegate =
+      new StrictMock<ControllableDetachDelegate>();
+  delegate->set_can_detach(true);
+  EXPECT_CALL(*delegate, OnMainEntry(_));
+  std::unique_ptr<SchedulerWorker> worker = SchedulerWorker::Create(
+      ThreadPriority::NORMAL, WrapUnique(delegate), delegate->task_tracker(),
+      SchedulerWorker::InitialState::ALIVE);
   worker->WakeUp();
   delegate->WaitForWorkToRun();
   Mock::VerifyAndClear(delegate);
   delegate->WaitForDetachRequest();
   delegate->WaitForDetach();
   ASSERT_FALSE(worker->ThreadAliveForTesting());
 
   delegate->ResetState();
   delegate->set_can_detach(false);
   // Expect OnMainEntry() to be called when SchedulerWorker recreates its
   // thread.
   EXPECT_CALL(*delegate, OnMainEntry(worker.get()));
   worker->WakeUp();
   delegate->WaitForWorkToRun();
   Mock::VerifyAndClear(delegate);
   delegate->WaitForDetachRequest();
   delegate->WaitForDetach();
   ASSERT_TRUE(worker->ThreadAliveForTesting());
   worker->JoinForTesting();
 }
 
 TEST(TaskSchedulerWorkerTest, CreateDetached) {
-  TaskTracker task_tracker;
   // Will be owned by SchedulerWorker.
   ControllableDetachDelegate* delegate =
-      new StrictMock<ControllableDetachDelegate>(&task_tracker);
-  std::unique_ptr<SchedulerWorker> worker =
-      SchedulerWorker::Create(
-          ThreadPriority::NORMAL, WrapUnique(delegate), &task_tracker,
-          SchedulerWorker::InitialState::DETACHED);
+      new StrictMock<ControllableDetachDelegate>();
+  std::unique_ptr<SchedulerWorker> worker = SchedulerWorker::Create(
+      ThreadPriority::NORMAL, WrapUnique(delegate), delegate->task_tracker(),
+      SchedulerWorker::InitialState::DETACHED);
   ASSERT_FALSE(worker->ThreadAliveForTesting());
   EXPECT_CALL(*delegate, OnMainEntry(worker.get()));
   worker->WakeUp();
   delegate->WaitForWorkToRun();
   Mock::VerifyAndClear(delegate);
   delegate->WaitForDetachRequest();
   ASSERT_TRUE(worker->ThreadAliveForTesting());
   worker->JoinForTesting();
 }
 
@@ skipping 171 matching lines @@
   // COM library wasn't already initialized on the thread.
   EXPECT_EQ(S_OK, delegate_raw->coinitialize_hresult());
 
   worker->JoinForTesting();
 }
 
 #endif  // defined(OS_WIN)
 
 }  // namespace internal
 }  // namespace base