TaskScheduler: Delegate instead of callbacks for SchedulerWorkerThread.

base/task_scheduler/scheduler_worker_thread_unittest.cc

Index: base/task_scheduler/scheduler_worker_thread_unittest.cc
diff --git a/base/task_scheduler/scheduler_worker_thread_unittest.cc b/base/task_scheduler/scheduler_worker_thread_unittest.cc
index fcccda182633678a4a45543e8f7c4274f8d7aa71..9b0eefe4b0af5151701866b8aaf1e70282cf7de8 100644
--- a/base/task_scheduler/scheduler_worker_thread_unittest.cc
+++ b/base/task_scheduler/scheduler_worker_thread_unittest.cc
@@ -26,51 +26,42 @@ namespace {
 
 const size_t kNumSequencesPerTest = 150;
 
-class TaskSchedulerWorkerThreadTest : public testing::Test {
+class TaskSchedulerWorkerThreadTest : public testing::Test,
+                                      public SchedulerWorkerThread::Delegate {
  protected:
   TaskSchedulerWorkerThreadTest()
-      : num_main_entry_callback_cv_(lock_.CreateConditionVariable()),
-        num_get_work_callback_cv_(lock_.CreateConditionVariable()),
+      : main_entry_called_(true, false),
+        num_get_work_cv_(lock_.CreateConditionVariable()),
         run_sequences_cv_(lock_.CreateConditionVariable()) {}
 
   void SetUp() override {
     worker_thread_ = SchedulerWorkerThread::CreateSchedulerWorkerThread(
-        ThreadPriority::NORMAL,
-        Bind(&TaskSchedulerWorkerThreadTest::MainEntryCallback,
-             Unretained(this)),
-        Bind(&TaskSchedulerWorkerThreadTest::GetWorkCallback, Unretained(this)),
-        Bind(&TaskSchedulerWorkerThreadTest::RanTaskFromSequenceCallback,
-             Unretained(this)),
-        &task_tracker_);
+        ThreadPriority::NORMAL, this, &task_tracker_);
     ASSERT_TRUE(worker_thread_);
-    WaitForNumMainEntryCallback(1);
+    main_entry_called_.Wait();
   }
 
   void TearDown() override {
-    worker_thread_->JoinForTesting();
+    {
+      AutoSchedulerLock auto_lock(lock_);
+      EXPECT_FALSE(main_exit_called_);
+    }
 
-    AutoSchedulerLock auto_lock(lock_);
-    EXPECT_EQ(1U, num_main_entry_callback_);
-  }
+    worker_thread_->JoinForTesting();
 
-  // Wait until MainEntryCallback() has been called |num_main_entry_callback|
-  // times.
-  void WaitForNumMainEntryCallback(size_t num_main_entry_callback) {
     AutoSchedulerLock auto_lock(lock_);
-    while (num_main_entry_callback_ < num_main_entry_callback)
-      num_main_entry_callback_cv_->Wait();
+    EXPECT_TRUE(main_exit_called_);

[danakj, 2016/04/07 18:40:59]

What's the point of OnMainExit if it can only happen from JoinForTesting?

[fdoray, 2016/04/07 18:46:58]

In production, the thread managed by a SchedulerWorkerThread can exit during
shutdown (task_tracker_->shutdown_completed()). OnMainExit() will be used to
clear the TLS set by OnMainEntry().

Note that the SchedulerWorkerThread instance is never destroyed in production
(despite the fact that its thread might exit).

[danakj, 2016/04/07 18:49:10]

Ah ok thanks.

[gab, 2016/04/07 18:50:20]

We thought we needed it for TLS destruction but turns out we don't (more details
coming up on SchedulerThreadPool CL when I send my comments :-))

   }
 
-  // Wait until GetWorkCallback() has been called |num_get_work_callback| times.
-  void WaitForNumGetWorkCallback(size_t num_get_work_callback) {
+  // Wait until GetWork() has been called |num_get_work| times.
+  void WaitForNumGetWork(size_t num_get_work) {
     AutoSchedulerLock auto_lock(lock_);
-    while (num_get_work_callback_ < num_get_work_callback)
-      num_get_work_callback_cv_->Wait();
+    while (num_get_work_ < num_get_work)
+      num_get_work_cv_->Wait();
   }
 
-  // Wait until there is no more Sequences to create and
-  // RanTaskFromSequenceCallback() has been invoked once for each Sequence
-  // returned by GetWorkCallback().
+  // Wait until there are no more Sequences to create and RanTaskFromSequence()
+  // has been invoked once for each Sequence returned by GetWork().
   void WaitForAllSequencesToRun() {
     AutoSchedulerLock auto_lock(lock_);
 
@@ -79,12 +70,12 @@ class TaskSchedulerWorkerThreadTest : public testing::Test {
       run_sequences_cv_->Wait();
     }
 
-    // Verify that RanTaskFromSequenceCallback() has been invoked with the
-    // same Sequences that were returned by GetWorkCallback().
+    // Verify that RanTaskFromSequence() has been invoked with the same
+    // Sequences that were returned by GetWork().
     EXPECT_EQ(created_sequences_, run_sequences_);
 
     // Verify that RunTaskCallback() has been invoked once for each Sequence
-    // returned by GetWorkCallback().
+    // returned by GetWork().
     EXPECT_EQ(created_sequences_.size(), num_run_tasks_);
   }
 
@@ -94,32 +85,39 @@ class TaskSchedulerWorkerThreadTest : public testing::Test {
     num_sequences_to_create_ = num_sequences_to_create;
   }
 
-  size_t NumGetWorkCallback() const {
+  size_t NumGetWork() const {
     AutoSchedulerLock auto_lock(lock_);
-    return num_get_work_callback_;
+    return num_get_work_;
   }
 
   std::unique_ptr<SchedulerWorkerThread> worker_thread_;
 
  private:
-  void MainEntryCallback() {
+  // SchedulerWorkerThread::Delegate:
+  void OnMainEntry() override {
+    // Without this |auto_lock|, OnMainEntry() could be called twice without
+    // generating an error.
+    AutoSchedulerLock auto_lock(lock_);
+    EXPECT_FALSE(main_entry_called_.IsSignaled());
+    main_entry_called_.Signal();
+  }
+
+  void OnMainExit() override {
     AutoSchedulerLock auto_lock(lock_);
-    ++num_main_entry_callback_;
-    num_main_entry_callback_cv_->Signal();
+    EXPECT_FALSE(main_exit_called_);
+    main_exit_called_ = true;
   }
 
-  // Returns a Sequence that contains 1 Task if |num_sequences_to_create_| is
-  // greater than 0.
-  scoped_refptr<Sequence> GetWorkCallback(
-      SchedulerWorkerThread* worker_thread) {
+  scoped_refptr<Sequence> GetWork(
+      SchedulerWorkerThread* worker_thread) override {
     EXPECT_EQ(worker_thread_.get(), worker_thread);
 
     {
       AutoSchedulerLock auto_lock(lock_);
 
-      // Increment the number of times that this callback has been invoked.
-      ++num_get_work_callback_;
-      num_get_work_callback_cv_->Signal();
+      // Increment the number of times that this method has been called.
+      ++num_get_work_;
+      num_get_work_cv_->Signal();
 
       // Check if a Sequence should be returned.
       if (num_sequences_to_create_ == 0)
@@ -145,10 +143,7 @@ class TaskSchedulerWorkerThreadTest : public testing::Test {
     return sequence;
   }
 
-  void RanTaskFromSequenceCallback(const SchedulerWorkerThread* worker_thread,
-                                   scoped_refptr<Sequence> sequence) {
-    EXPECT_EQ(worker_thread_.get(), worker_thread);
-
+  void RanTaskFromSequence(scoped_refptr<Sequence> sequence) override {
     AutoSchedulerLock auto_lock(lock_);
     run_sequences_.push_back(std::move(sequence));
     run_sequences_cv_->Signal();
@@ -164,26 +159,26 @@ class TaskSchedulerWorkerThreadTest : public testing::Test {
   // Synchronizes access to all members below.
   mutable SchedulerLock lock_;
 
-  // Number of times that MainEntryCallback() has been called.
-  size_t num_main_entry_callback_ = 0;
+  // Signaled once OnMainEntry() has been called.
+  WaitableEvent main_entry_called_;
 
-  // Condition variable signaled when |num_main_entry_callback_| is incremented.
-  std::unique_ptr<ConditionVariable> num_main_entry_callback_cv_;
+  // True once OnMainExit() has been called.
+  bool main_exit_called_ = false;
 
-  // Number of Sequences that should be created by GetWorkCallback(). When this
-  // is 0, GetWorkCallback() returns nullptr.
+  // Number of Sequences that should be created by GetWork(). When this
+  // is 0, GetWork() returns nullptr.
   size_t num_sequences_to_create_ = 0;
 
-  // Number of times that GetWorkCallback() has been called.
-  size_t num_get_work_callback_ = 0;
+  // Number of times that GetWork() has been called.
+  size_t num_get_work_ = 0;
 
-  // Condition variable signaled when |num_get_work_callback_| is incremented.
-  std::unique_ptr<ConditionVariable> num_get_work_callback_cv_;
+  // Condition variable signaled when |num_get_work_| is incremented.
+  std::unique_ptr<ConditionVariable> num_get_work_cv_;
 
-  // Sequences created by GetWorkCallback().
+  // Sequences created by GetWork().
   std::vector<scoped_refptr<Sequence>> created_sequences_;
 
-  // Sequences passed to RanTaskFromSequenceCallback().
+  // Sequences passed to RanTaskFromSequence().
   std::vector<scoped_refptr<Sequence>> run_sequences_;
 
   // Condition variable signaled when a Sequence is added to |run_sequences_|.
@@ -195,45 +190,44 @@ class TaskSchedulerWorkerThreadTest : public testing::Test {
   DISALLOW_COPY_AND_ASSIGN(TaskSchedulerWorkerThreadTest);
 };
 
-// Verify that when GetWorkCallback() continuously returns Sequences, all Tasks
-// in these Sequences run successfully. The SchedulerWorkerThread is woken up
-// once.
+// Verify that when GetWork() continuously returns Sequences, all Tasks in these
+// Sequences run successfully. The SchedulerWorkerThread is woken up once.
 TEST_F(TaskSchedulerWorkerThreadTest, ContinousWork) {
-  // Set GetWorkCallback() to return |kNumSequencesPerTest| Sequences before
-  // starting to return nullptr.
+  // Set GetWork() to return |kNumSequencesPerTest| Sequences before starting to
+  // return nullptr.
   SetNumSequencesToCreate(kNumSequencesPerTest);
 
   // Wake up |worker_thread_| and wait until it has run all the Tasks returned
-  // by GetWorkCallback().
+  // by GetWork().
   worker_thread_->WakeUp();
   WaitForAllSequencesToRun();
 
-  // Expect |kNumSequencesPerTest| calls to GetWorkCallback() in which it
-  // returned a Sequence and 1 call in which it returned nullptr.
-  const size_t expected_num_get_work_callback = kNumSequencesPerTest + 1;
-  WaitForNumGetWorkCallback(expected_num_get_work_callback);
-  EXPECT_EQ(expected_num_get_work_callback, NumGetWorkCallback());
+  // Expect |kNumSequencesPerTest| calls to GetWork() in which it returned a
+  // Sequence and 1 call in which it returned nullptr.
+  const size_t expected_num_get_work = kNumSequencesPerTest + 1;
+  WaitForNumGetWork(expected_num_get_work);
+  EXPECT_EQ(expected_num_get_work, NumGetWork());
 }
 
-// Verify that when GetWorkCallback() alternates between returning a Sequence
-// and returning nullptr, all Tasks in the returned Sequences run successfully.
-// The SchedulerWorkerThread is woken up once for each Sequence.
+// Verify that when GetWork() alternates between returning a Sequence and
+// returning nullptr, all Tasks in the returned Sequences run successfully. The
+// SchedulerWorkerThread is woken up once for each Sequence.
 TEST_F(TaskSchedulerWorkerThreadTest, IntermittentWork) {
   for (size_t i = 0; i < kNumSequencesPerTest; ++i) {
-    // Set GetWorkCallback() to return 1 Sequence before starting to return
+    // Set GetWork() to return 1 Sequence before starting to return
     // nullptr.
     SetNumSequencesToCreate(1);
 
-    // Wake up |worker_thread_| and wait until it has run all the Tasks returned
-    // by GetWorkCallback().
+    // Wake up |worker_thread_| and wait until it has run the Task returned by
+    // GetWork().
     worker_thread_->WakeUp();
     WaitForAllSequencesToRun();
 
-    // Expect |i| calls to GetWorkCallback() in which it returned a Sequence and
+    // Expect |i| calls to GetWork() in which it returned a Sequence and
     // |i| calls in which it returned nullptr.
-    const size_t expected_num_get_work_callback = 2 * (i + 1);
-    WaitForNumGetWorkCallback(expected_num_get_work_callback);
-    EXPECT_EQ(expected_num_get_work_callback, NumGetWorkCallback());
+    const size_t expected_num_get_work = 2 * (i + 1);
+    WaitForNumGetWork(expected_num_get_work);
+    EXPECT_EQ(expected_num_get_work, NumGetWork());
   }
 }