TaskScheduler [6] SchedulerWorkerThread

base/task_scheduler/worker_thread_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/worker_thread.h"
+
+#include "base/bind.h"
+#include "base/bind_helpers.h"
+#include "base/callback_forward.h"
+#include "base/logging.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/synchronization/condition_variable.h"
+#include "base/synchronization/waitable_event.h"
+#include "base/task_scheduler/priority_queue.h"
+#include "base/task_scheduler/scheduler_lock.h"
+#include "base/task_scheduler/task_tracker.h"
+#include "base/task_scheduler/utils.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace base {
+namespace internal {
+
+class TaskSchedulerWorkerThreadTest : public testing::Test {
+ protected:
+  TaskSchedulerWorkerThreadTest()
+      : shared_priority_queue_(Bind(&DoNothing)),
+        run_task_cv_(lock_.CreateConditionVariable()),
+        becomes_idle_callback_cv_(lock_.CreateConditionVariable()),
+        num_becomes_idle_callback_invocations_(0),
+        last_posted_task_index_(0),
+        last_run_task_index_(0),
+        ran_task_that_should_not_run_(false),
+        ran_tasks_in_wrong_order_(true) {
+    worker_thread_ = WorkerThread::CreateWorkerThread(
+        ThreadPriority::NORMAL,
+        &shared_priority_queue_,
+        Bind(&TaskSchedulerWorkerThreadTest::ReinsertSequenceCallback,
+             Unretained(this)),
+        Bind(&TaskSchedulerWorkerThreadTest::BecomesIdleCallback,
+             Unretained(this)),
+        &task_tracker_);
+  }
+
+  Closure GetTaskThatShouldRunClosure() {
+    ++last_posted_task_index_;
+    return Bind(&TaskSchedulerWorkerThreadTest::RunTaskThatShouldRun,
+                Unretained(this), last_posted_task_index_);
+  }
+
+  Closure GetTaskThatShouldNotRunClosure() {
+    return Bind(&TaskSchedulerWorkerThreadTest::RunTaskThatShouldNotRun,
+                Unretained(this));
+  }
+
+  void WaitUntilLastPostedTaskHasRun() {

[gab, 2016/03/21 19:11:55]

s/HasRun/Ran/

(here and below)

[fdoray, 2016/03/24 19:21:11]

Done.

+    AutoSchedulerLock auto_lock(lock_);
+    while (last_posted_task_index_ != last_run_task_index_)
+      run_task_cv_->Wait();
+  }
+
+  void ExpectLastPostedTaskHasRun() {
+    AutoSchedulerLock auto_lock(lock_);
+    EXPECT_EQ(last_posted_task_index_, last_run_task_index_);
+  }
+
+  void WaitUntilNumBecomesIdleCallbackInvocations(
+      size_t expected_num_becomes_idle_callback_invocations) {
+    AutoSchedulerLock auto_lock(lock_);
+    while (num_becomes_idle_callback_invocations_ !=
+           expected_num_becomes_idle_callback_invocations) {
+      EXPECT_LT(num_becomes_idle_callback_invocations_,
+                expected_num_becomes_idle_callback_invocations);
+      becomes_idle_callback_cv_->Wait();
+    }
+  }
+
+  bool ran_task_that_should_not_run() const {
+    return ran_task_that_should_not_run_;
+  }
+
+ protected:
+  PriorityQueue shared_priority_queue_;
+  TaskTracker task_tracker_;
+  scoped_ptr<WorkerThread> worker_thread_;
+
+ private:
+  void ReinsertSequenceCallback(scoped_refptr<Sequence> sequence,
+                                const WorkerThread* worker_thread) {
+    const SequenceSortKey sort_key = sequence->GetSortKey();
+    shared_priority_queue_.BeginTransaction()->Push(make_scoped_ptr(
+        new PriorityQueue::SequenceAndSortKey(std::move(sequence), sort_key)));
+  }
+
+  void BecomesIdleCallback(WorkerThread* worker_thread) {
+    AutoSchedulerLock auto_lock(lock_);
+    ++num_becomes_idle_callback_invocations_;
+    becomes_idle_callback_cv_->Signal();
+  }
+
+  void RunTaskThatShouldRun(size_t index) {
+    AutoSchedulerLock auto_lock(lock_);
+
+    if (index != last_run_task_index_ + 1)
+      ran_tasks_in_wrong_order_ = true;

[gab, 2016/03/21 19:11:55]

ADD_FAILURE here too instead of maintaining extra state (see below)

[fdoray, 2016/03/24 19:21:10]

Done.

+
+    last_run_task_index_ = index;
+    run_task_cv_->Signal();
+  }
+
+  void RunTaskThatShouldNotRun() { ran_task_that_should_not_run_ = true; }

[gab, 2016/03/21 19:11:55]

Instead of having extra state here and checking for it, this method's body could
simply be:

{
  ADD_FAILURE() << "Unexpected task execution.";
}

which is equivalent to EXPECT_FALSE(ran_task_that_should_not_run()); after +
makes error report synchronous instead of after the fact :-)

[fdoray, 2016/03/24 19:21:11]

Done.

+
+  // Lock protecting |run_task_cv_|.

[gab, 2016/03/21 19:11:55]

Hmm, it does more than that it seems?

[fdoray, 2016/03/24 19:21:11]

Done.

+  SchedulerLock lock_;
+
+  // Condition variable signaled each time a task completes its execution.
+  scoped_ptr<ConditionVariable> run_task_cv_;
+
+  // Condition variable signaled when BecomesIdleCallback() is invoked.
+  scoped_ptr<ConditionVariable> becomes_idle_callback_cv_;
+
+  // Number of times that BecomesIdleCallback() has been called.
+  size_t num_becomes_idle_callback_invocations_;
+
+  // Index of the last posted task.
+  size_t last_posted_task_index_;
+
+  // Index of the last run task.
+  size_t last_run_task_index_;
+
+  // True if a task that shouldn't run has run.
+  bool ran_task_that_should_not_run_;
+
+  // True if tasks were run in the wrong order.
+  bool ran_tasks_in_wrong_order_;
+
+};
+
+TEST_F(TaskSchedulerWorkerThreadTest, PostOneSingleThreadedTask) {
+  ASSERT_NE(nullptr, worker_thread_.get());

[gab, 2016/03/21 19:11:55]

ASSERT_TRUE(worker_thread_);

[fdoray, 2016/03/24 19:21:11]

Done.

+  WaitUntilNumBecomesIdleCallbackInvocations(1);

[gab, 2016/03/21 19:11:55]

Move these two lines to constructor instead of at beginning of each test?

[fdoray, 2016/03/24 19:21:11]

Done.

+
+  worker_thread_->CreateTaskRunnerWithTraits(TaskTraits())
+      ->PostTask(FROM_HERE, GetTaskThatShouldRunClosure());
+
+  WaitUntilNumBecomesIdleCallbackInvocations(2);

[gab, 2016/03/21 19:11:55]

Since WaitUntilNumBecomesIdleCallbackInvocations(1); will move to constructor,
it will be less obvious why this is "2".

So maybe instead it should just be WaitForNextBecomesIdleCallbackInvocation(); ?

[fdoray, 2016/03/24 19:21:10]

With WaitForNextBecomesIdleCallbackInvocation(), it's harder to handle the case
where the callback invocation occurs before we wait for it. Anyways, the unit
tests changed a lot and probably need a new review.

+  ExpectLastPostedTaskHasRun();
+  worker_thread_->JoinForTesting();

[gab, 2016/03/21 19:11:55]

Since the thread is created by the fixture I think JoinForTesting() belongs in
the fixture's TearDown().

(and maybe |worker_thread_| should be created in SetUp() instead for symmetry
but I don't feel strongly there, up to you)

[gab, 2016/03/21 19:11:55]

In fact I think TearDown() should do both:

ExpectLastPostedTaskHasRun();
worker_thread_->JoinForTesting();

Though ExpectLastPostedTaskHasRun(); probably also still belongs in this test's
body to make it clear what the test is verifying (the one in TearDown() merely
being a double-check that all tests do wait and verify all their things before
wind down).

In that spirit TearDown() could also check that all BecomesIdleCallback
invocations were observed (maybe loosening it up to having observing >= N-1
invocations to avoid forcing a check in tests that don't care about the last
idle notification).
i.e. I don't think this test currently catches a bug where too many
notifications are fired and that's one way to.

WDYT?

[fdoray, 2016/03/24 19:21:11]

Done.

[fdoray, 2016/03/24 19:21:11]

ExpectLastPostedTaskHasRun() is not done in the destructor of
TaskClosureFactory. I had to create a factory for
TaskSchedulerWorkerThreadTest.PostSingleThreadedTasksFromMultipleThreads.

+}
+
+TEST_F(TaskSchedulerWorkerThreadTest,
+       PostMultipleSingleThreadedTasksNoWaitBetweenPosts) {
+  ASSERT_NE(nullptr, worker_thread_.get());
+  WaitUntilNumBecomesIdleCallbackInvocations(1);
+
+  auto task_runner = worker_thread_->CreateTaskRunnerWithTraits(TaskTraits());
+
+  for (size_t i = 0; i < 100; ++i)
+    task_runner->PostTask(FROM_HERE, GetTaskThatShouldRunClosure());
+
+  WaitUntilLastPostedTaskHasRun();
+  worker_thread_->JoinForTesting();
+}
+
+TEST_F(TaskSchedulerWorkerThreadTest,
+       PostMultipleSingleThreadedTasksWaitBetweenPosts) {
+  ASSERT_NE(nullptr, worker_thread_.get());
+  WaitUntilNumBecomesIdleCallbackInvocations(1);
+
+  auto task_runner = worker_thread_->CreateTaskRunnerWithTraits(TaskTraits());
+
+  for (size_t i = 0; i < 100; ++i) {
+    task_runner->PostTask(FROM_HERE, GetTaskThatShouldRunClosure());
+    WaitUntilLastPostedTaskHasRun();
+    WaitUntilNumBecomesIdleCallbackInvocations(2 + i);
+  }
+
+  worker_thread_->JoinForTesting();
+}
+
+TEST_F(TaskSchedulerWorkerThreadTest,
+       PostMultipleTasksTwoSingleThreadedTaskRunners) {
+  ASSERT_NE(nullptr, worker_thread_.get());
+  WaitUntilNumBecomesIdleCallbackInvocations(1);
+
+  auto task_runner_a = worker_thread_->CreateTaskRunnerWithTraits(TaskTraits());
+  auto task_runner_b = worker_thread_->CreateTaskRunnerWithTraits(TaskTraits());
+
+  for (size_t i = 0; i < 100; ++i) {
+    task_runner_a->PostTask(FROM_HERE, GetTaskThatShouldRunClosure());
+    task_runner_b->PostTask(FROM_HERE, GetTaskThatShouldRunClosure());
+  }
+
+  WaitUntilLastPostedTaskHasRun();
+  worker_thread_->JoinForTesting();
+}
+
+TEST_F(TaskSchedulerWorkerThreadTest, PostOneTaskInSharedPriorityQueue) {
+  ASSERT_NE(nullptr, worker_thread_.get());
+  WaitUntilNumBecomesIdleCallbackInvocations(1);
+
+  PostTaskHelper(make_scoped_ptr(new Task(
+                     FROM_HERE, GetTaskThatShouldRunClosure(), TaskTraits())),
+                 new Sequence, &shared_priority_queue_, &task_tracker_);

[gab, 2016/03/21 19:11:55]

make_scoped_refptr(new Sequence)

here and below (Chromium is moving towards making scoped_refptr(T*) explicit --
long story:
https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/TlL1D-Djta0...)

[fdoray, 2016/03/24 19:21:11]

Done.

+  EXPECT_TRUE(worker_thread_->WakeUp());
+
+  WaitUntilNumBecomesIdleCallbackInvocations(2);
+  ExpectLastPostedTaskHasRun();
+  worker_thread_->JoinForTesting();
+}
+
+TEST_F(TaskSchedulerWorkerThreadTest, PostSharedAndSingleThreadedTasks) {
+  ASSERT_NE(nullptr, worker_thread_.get());
+  WaitUntilNumBecomesIdleCallbackInvocations(1);
+
+  // Post a task in the shared priority queue.
+  PostTaskHelper(make_scoped_ptr(new Task(
+                     FROM_HERE, GetTaskThatShouldRunClosure(), TaskTraits())),
+                 new Sequence, &shared_priority_queue_, &task_tracker_);
+
+  // Post a task in the single-threaded priority queue. The TaskRunner will wake
+  // up the WorkerThread.
+  worker_thread_->CreateTaskRunnerWithTraits(TaskTraits())
+      ->PostTask(FROM_HERE, GetTaskThatShouldRunClosure());
+
+  WaitUntilNumBecomesIdleCallbackInvocations(2);
+  ExpectLastPostedTaskHasRun();
+  worker_thread_->JoinForTesting();
+}
+
+TEST_F(TaskSchedulerWorkerThreadTest, WakeUpIdleThread) {
+  ASSERT_NE(nullptr, worker_thread_.get());
+  WaitUntilNumBecomesIdleCallbackInvocations(1);
+
+  EXPECT_TRUE(worker_thread_->WakeUp());
+  WaitUntilNumBecomesIdleCallbackInvocations(2);
+
+  worker_thread_->JoinForTesting();
+}
+
+TEST_F(TaskSchedulerWorkerThreadTest, WakeUpBusyThread) {
+  ASSERT_NE(nullptr, worker_thread_.get());
+  WaitUntilNumBecomesIdleCallbackInvocations(1);
+
+  // Post a task.
+  WaitableEvent event(false, false);
+  worker_thread_->CreateTaskRunnerWithTraits(TaskTraits())
+      ->PostTask(FROM_HERE, Bind(&WaitableEvent::Wait, Unretained(&event)));
+
+  // Because the thread is busy, WakeUp() should return false.
+  EXPECT_FALSE(worker_thread_->WakeUp());
+
+  event.Signal();
+  WaitUntilNumBecomesIdleCallbackInvocations(2);
+  worker_thread_->JoinForTesting();
+}

[gab, 2016/03/21 19:11:55]

A good chunk of ThreadMain is about handling race conditions between sleeping
and receiving work.

Can we come up with a test that stresses this? We probably can't in a
deterministic way, but can we spam the queue somehow and make these conditions a
possibility that would be caught sometimes if it's handled wrong? Flaky test
failures are bad, but they're better than flaky product, so long as we can make
failures actionable enough to get the bug fixed instead of the test disabled..!

[fdoray, 2016/03/24 19:21:11]

Done.

+
+}  // namespace internal
+}  // namespace base