TaskScheduler [6] SchedulerWorkerThread

base/task_scheduler/worker_thread_unittest.cc

Index: base/task_scheduler/worker_thread_unittest.cc
diff --git a/base/task_scheduler/worker_thread_unittest.cc b/base/task_scheduler/worker_thread_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..58878ea613b0fc5325ad605023c67329c947f124
--- /dev/null
+++ b/base/task_scheduler/worker_thread_unittest.cc
@@ -0,0 +1,213 @@
+// 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/worker_thread.h"
+
+#include <stddef.h>
+
+#include <vector>
+
+#include "base/bind.h"
+#include "base/bind_helpers.h"
+#include "base/macros.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/synchronization/condition_variable.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 "testing/gtest/include/gtest/gtest.h"
+
+namespace base {
+namespace internal {
+namespace {
+
+const size_t kNumSequencesPerTest = 150;
+
+class TaskSchedulerWorkerThreadTest : public testing::Test {
+ protected:
+  TaskSchedulerWorkerThreadTest()
+      : num_get_work_callback_cv_(lock_.CreateConditionVariable()),
+        run_sequences_cv_(lock_.CreateConditionVariable()) {}
+
+  void SetUp() override {
+    worker_thread_ = WorkerThread::CreateWorkerThread(
+        ThreadPriority::NORMAL,
+        Bind(&TaskSchedulerWorkerThreadTest::GetWorkCallback, Unretained(this)),
+        Bind(&TaskSchedulerWorkerThreadTest::RanTaskFromSequenceCallback,
+             Unretained(this)),
+        &task_tracker_);
+    ASSERT_TRUE(worker_thread_);
+  }
+
+  void TearDown() override { worker_thread_->JoinForTesting(); }
+
+  // Wait until GetWorkCallback() has been called |num_get_work_callback| times.
+  void WaitForNumGetWorkCallback(size_t num_get_work_callback) {
+    AutoSchedulerLock auto_lock(lock_);
+    while (num_get_work_callback_ < num_get_work_callback)
+      num_get_work_callback_cv_->Wait();
+  }
+
+  // Wait until there is no more Sequences to create and
+  // RanTaskFromSequenceCallback() has been invoked once for each Sequence
+  // returned by GetWorkCallback().
+  void WaitForAllSequencesToRun() {
+    AutoSchedulerLock auto_lock(lock_);
+
+    while (num_sequences_to_create_ > 0 ||
+           run_sequences_.size() < created_sequences_.size()) {
+      run_sequences_cv_->Wait();
+    }
+
+    // Verify that RanTaskFromSequenceCallback() has been invoked with the
+    // same Sequences that were returned by GetWorkCallback().
+    EXPECT_EQ(created_sequences_, run_sequences_);
+
+    // Verify that RunTaskCallback() has been invoked once for each Sequence
+    // returned by GetWorkCallback().
+    EXPECT_EQ(created_sequences_.size(), num_run_tasks_);
+  }
+
+  void set_num_sequences_to_create(size_t num_sequences_to_create) {

[danakj, 2016/03/31 00:25:14]

nit: SetNumSequencesToCreate. this isn't just a simple setter.

[fdoray, 2016/03/31 03:26:31]

Done.

+    AutoSchedulerLock auto_lock(lock_);
+    EXPECT_EQ(0U, num_sequences_to_create_);
+    num_sequences_to_create_ = num_sequences_to_create;
+  }
+
+  size_t num_get_work_callback() const {

[danakj, 2016/03/31 00:25:13]

nitto, NumGetWorkCallback, locking make this not "cheap" IMO

[fdoray, 2016/03/31 03:26:31]

Done.

+    AutoSchedulerLock auto_lock(lock_);
+    return num_get_work_callback_;
+  }
+
+  scoped_ptr<WorkerThread> worker_thread_;
+
+ private:
+  // Returns a Sequence that contains 1 Task if |num_sequences_to_create_| is
+  // greater than 0.
+  scoped_refptr<Sequence> GetWorkCallback(const WorkerThread* worker_thread) {
+    EXPECT_EQ(worker_thread_.get(), worker_thread);
+
+    {
+      AutoSchedulerLock auto_lock(lock_);

[danakj, 2016/03/31 00:25:13]

Am I imagining deadlocks here? WaitForNumGetWorkCallback() would hold this lock
and wait on num_get_work_callback_cv_. This can't Signal, until this lock is
released.

Ditto for WaitForAllSequencesToRun and run_sequences_cv_?

[fdoray, 2016/03/31 03:26:31]

It is a requirement of ConditionVariable to acquire its associated lock before
calling Wait or Signal. Wait() releases the lock as it starts to sleep [1]. That
means that the Signal() at line 97 can happen while WaitForNumGetWorkCallback()
is waiting.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/base/synchronizati...

[fdoray, 2016/03/31 13:29:24]

In fact, holding the ConditionVariable's lock is only a requirement for calling
Wait (not Signal).

[danakj, 2016/03/31 22:33:49]

oh.. right I was missing that the CV is tied to that |lock_|, thanks.

+
+      // Increment the number of times that this callback has been invoked.
+      ++num_get_work_callback_;
+      num_get_work_callback_cv_->Signal();
+
+      // Check if a Sequence should be returned.
+      if (num_sequences_to_create_ == 0)
+        return scoped_refptr<Sequence>();

[danakj, 2016/03/31 00:25:13]

return nullptr;

[fdoray, 2016/03/31 03:26:31]

Done.

+      --num_sequences_to_create_;
+    }
+
+    // Create a Sequence that contains 1 Task.
+    scoped_refptr<Sequence> sequence(new Sequence);
+    task_tracker_.PostTask(
+        Bind(IgnoreResult(&Sequence::PushTask), Unretained(sequence.get())),
+        make_scoped_ptr(new Task(
+            FROM_HERE, Bind(&TaskSchedulerWorkerThreadTest::RunTaskCallback,
+                            Unretained(this)),
+            TaskTraits())));
+
+    {
+      // Add the Sequence to the vector of created Sequences.
+      AutoSchedulerLock auto_lock(lock_);
+      created_sequences_.push_back(sequence);
+    }
+
+    return sequence;
+  }
+
+  void RanTaskFromSequenceCallback(const WorkerThread* worker_thread,
+                                   scoped_refptr<Sequence> sequence) {
+    EXPECT_EQ(worker_thread_.get(), worker_thread);
+
+    AutoSchedulerLock auto_lock(lock_);
+    run_sequences_.push_back(std::move(sequence));
+    run_sequences_cv_->Signal();
+  }
+
+  void RunTaskCallback() {
+    AutoSchedulerLock auto_lock(lock_);
+    ++num_run_tasks_;
+  }
+
+  TaskTracker task_tracker_;
+
+  // Synchronizes access to all members below.
+  mutable SchedulerLock lock_;
+
+  // Number of Sequences that should be created by GetWorkCallback(). When this
+  // is 0, GetWorkCallback() returns nullptr.
+  size_t num_sequences_to_create_ = 0;
+
+  // Number of times that GetWorkCallback() has been called.
+  size_t num_get_work_callback_ = 0;
+
+  // Condition variable signaled when |num_get_work_callback_| is incremented.
+  scoped_ptr<ConditionVariable> num_get_work_callback_cv_;
+
+  // Sequences created by GetWorkCallback().
+  std::vector<scoped_refptr<Sequence>> created_sequences_;
+
+  // Sequences passed to RanTaskFromSequenceCallback().
+  std::vector<scoped_refptr<Sequence>> run_sequences_;
+
+  // Condition variable signaled when a Sequence is added to |run_sequences_|.
+  scoped_ptr<ConditionVariable> run_sequences_cv_;
+
+  // Number of times that RunTaskCallback() has been called.
+  size_t num_run_tasks_ = 0;
+
+  DISALLOW_COPY_AND_ASSIGN(TaskSchedulerWorkerThreadTest);
+};
+
+// Verify that when GetWorkCallback() continuously returns Sequences, all Tasks
+// in these Sequences run successfully. The WorkerThread is woken up once.
+TEST_F(TaskSchedulerWorkerThreadTest, ContinousWork) {
+  // Set GetWorkCallback() to return |kNumSequencesPerTest| Sequences before
+  // starting to return nullptr.
+  set_num_sequences_to_create(kNumSequencesPerTest);
+
+  // Wake up |worker_thread_| and wait until it has run all the Tasks returned
+  // by GetWorkCallback().
+  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, num_get_work_callback());
+}
+
+// Verify that when GetWorkCallback() alternates between returning a Sequence
+// and returning nullptr, all Tasks in the returned Sequences run successfully.
+// The WorkerThread 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
+    // nullptr.
+    set_num_sequences_to_create(1);
+
+    // Wake up |worker_thread_| and wait until it has run all the Tasks returned
+    // by GetWorkCallback().
+    worker_thread_->WakeUp();
+    WaitForAllSequencesToRun();
+
+    // Let the WorkerThread go to sleep.

[danakj, 2016/03/31 00:25:13]

?

This looks like a flakey way to avoid races, but I'm not sure what you're trying
to actual wait for to happen here.

[fdoray, 2016/03/31 03:26:31]

Removed it. The goal was to give enough time to WorkerThread to call
wake_up_event_.Wait() before going to the next iteration of this test. However,
it's not absolutely necessary to wait for that (wake_up_event_.Wait() will
return immediately if it is called before line 197 of this test runs).

+    PlatformThread::Sleep(TimeDelta::FromMilliseconds(25));
+
+    // Expect |i| calls to GetWorkCallback() 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, num_get_work_callback());
+  }
+}
+
+}  // namespace
+}  // namespace internal
+}  // namespace base