TaskScheduler [5/9] Task Tracker

base/task_scheduler/task_tracker_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/task_tracker.h"
+
+#include <queue>
+
+#include "base/bind.h"
+#include "base/logging.h"
+#include "base/macros.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/synchronization/waitable_event.h"
+#include "base/task_scheduler/task.h"
+#include "base/task_scheduler/task_traits.h"
+#include "base/task_scheduler/test_utils.h"
+#include "base/threading/platform_thread.h"
+#include "base/threading/simple_thread.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace base {
+namespace internal {
+
+namespace {
+
+class ThreadCallingShutdown : public SimpleThread {
+ public:
+  explicit ThreadCallingShutdown(TaskTracker* tracker)
+      : SimpleThread("ThreadCallingShutdown"), tracker_(tracker) {}
+
+  // Returns true once the call to Shutdown() has returned.
+  bool has_returned() const { return has_returned_; }
+
+ private:
+  void Run() override {
+    tracker_->Shutdown();
+    has_returned_ = true;
+  }
+
+  TaskTracker* const tracker_;
+  bool has_returned_ = false;
+
+  DISALLOW_COPY_AND_ASSIGN(ThreadCallingShutdown);
+};
+
+class TaskSchedulerTaskTrackerTest
+    : public testing::TestWithParam<TaskShutdownBehavior> {
+ public:
+  TaskSchedulerTaskTrackerTest() = default;
+
+  void RunNextPostedTaskViaTracker() {
+    ASSERT_FALSE(posted_tasks_.empty());
+    tracker_.RunTask(posted_tasks_.front().get());
+    posted_tasks_.pop();
+  }
+
+ protected:
+  // Creates a task with |shutdown_behavior|.
+  scoped_ptr<Task> CreateTask(TaskShutdownBehavior shutdown_behavior) {
+    return make_scoped_ptr(new Task(
+        FROM_HERE,
+        Bind(&TaskSchedulerTaskTrackerTest::RunTaskCallback, Unretained(this)),
+        TaskTraits().WithShutdownBehavior(shutdown_behavior)));
+  }
+
+  // Tries to post |task| via |tracker_|. If |tracker_| approves the operation,
+  // |task| is added to |posted_tasks_|.
+  void PostTaskViaTracker(scoped_ptr<Task> task) {
+    tracker_.PostTask(
+        Bind(&TaskSchedulerTaskTrackerTest::PostTaskCallback, Unretained(this)),
+        std::move(task));
+  }
+
+  // Calls tracker_->Shutdown() on a new thread. When this returns, the
+  // Shutdown() method has been entered on the new thread, but it hasn't
+  // necessarily returned.
+  void CallShutdownAsync() {
+    DCHECK(!thread_calling_shutdown_.get());
+    thread_calling_shutdown_.reset(new ThreadCallingShutdown(&tracker_));
+    thread_calling_shutdown_->Start();
+    while (!tracker_.IsShuttingDownForTesting() &&
+           !tracker_.shutdown_completed()) {
+      PlatformThread::YieldCurrentThread();
+    }
+  }
+
+  void WaitForAsyncShutdownCompleted() {
+    DCHECK(thread_calling_shutdown_.get());
+    thread_calling_shutdown_->Join();
+    EXPECT_TRUE(thread_calling_shutdown_->has_returned());
+    EXPECT_TRUE(tracker_.shutdown_completed());
+  }
+
+  void VerifyAsyncShutdownInProgress() {
+    DCHECK(thread_calling_shutdown_.get());
+    EXPECT_FALSE(thread_calling_shutdown_->has_returned());
+    EXPECT_FALSE(tracker_.shutdown_completed());
+    EXPECT_TRUE(tracker_.IsShuttingDownForTesting());
+  }
+
+  TaskTracker tracker_;
+  size_t num_tasks_executed_ = 0;
+  std::queue<scoped_ptr<Task>> posted_tasks_;
+
+ private:
+  void PostTaskCallback(scoped_ptr<Task> task) {
+    posted_tasks_.push(std::move(task));
+  }
+
+  void RunTaskCallback() { ++num_tasks_executed_; }
+
+  scoped_ptr<ThreadCallingShutdown> thread_calling_shutdown_;
+
+  DISALLOW_COPY_AND_ASSIGN(TaskSchedulerTaskTrackerTest);
+};
+
+// A thread which calls
+// TaskSchedulerTaskTrackerTest::RunNextPostedTaskViaTracker() asynchronously.
+class ThreadRunningNextPostedTask : public SimpleThread {
+ public:
+  explicit ThreadRunningNextPostedTask(TaskSchedulerTaskTrackerTest* test)
+      : SimpleThread("ThreadRunningNextPostedTask"), test_(test) {}
+
+ private:
+  void Run() override { test_->RunNextPostedTaskViaTracker(); }
+
+  TaskSchedulerTaskTrackerTest* const test_;
+
+  DISALLOW_COPY_AND_ASSIGN(ThreadRunningNextPostedTask);
+};
+
+}  // namespace
+
+TEST_P(TaskSchedulerTaskTrackerTest, PostAndRunBeforeShutdown) {
+  scoped_ptr<Task> task_to_post(CreateTask(GetParam()));
+  const Task* task_to_post_raw = task_to_post.get();
+
+  // Post the task.
+  EXPECT_TRUE(posted_tasks_.empty());

[robliao, 2016/03/17 23:34:21]

This should probably be an ASSERT_TRUE and the next ASSERT_EQ( should be an
EXPECT_EQ.

http://googletesting.blogspot.com/2008/07/tott-expect-vs-assert.html

[fdoray, 2016/03/18 20:35:39]

It is useful to continue running the test even if |posted_tasks_| is non-empty.
The checks at lines 145-147 should succeed. Therefore, I think that EXPECT is
more appropriate than ASSERT for line 139.

I agree with you that EXPECT is more appropriate at line 141.

+  PostTaskViaTracker(std::move(task_to_post));
+  ASSERT_EQ(1U, posted_tasks_.size());
+  EXPECT_EQ(task_to_post_raw, posted_tasks_.front().get());
+
+  // Run the posted task.
+  EXPECT_EQ(0U, num_tasks_executed_);
+  RunNextPostedTaskViaTracker();
+  EXPECT_EQ(1U, num_tasks_executed_);
+
+  // Shutdown() shouldn't block.
+  tracker_.Shutdown();
+}
+
+TEST_P(TaskSchedulerTaskTrackerTest, PostAndRunLongTaskBeforeShutdown) {
+  // Post a task that will block until |event| is signaled.
+  EXPECT_TRUE(posted_tasks_.empty());
+  WaitableEvent event(false, false);
+  PostTaskViaTracker(make_scoped_ptr(
+      new Task(FROM_HERE, Bind(&WaitableEvent::Wait, Unretained(&event)),
+               TaskTraits().WithShutdownBehavior(GetParam()))));
+  ASSERT_EQ(1U, posted_tasks_.size());
+
+  // Run the task asynchronouly.
+  ThreadRunningNextPostedTask thread(this);
+  thread.Start();
+
+  // Initiate shutdown while the task is running.
+  CallShutdownAsync();
+
+  if (GetParam() == TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN) {
+    // Shutdown should complete even with a CONTINUE_ON_SHUTDOWN in progress.
+    WaitForAsyncShutdownCompleted();
+  } else {
+    // Shutdown should block with any non CONTINUE_ON_SHUTDOWN task in progress.
+    VerifyAsyncShutdownInProgress();
+  }
+
+  // Unblock the task.
+  event.Signal();
+  thread.Join();
+
+  if (GetParam() != TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN)
+    WaitForAsyncShutdownCompleted();
+}
+
+TEST_P(TaskSchedulerTaskTrackerTest, PostBeforeShutdownRunDuringShutdown) {
+  scoped_ptr<Task> task_to_post(CreateTask(GetParam()));
+  const Task* task_to_post_raw = task_to_post.get();
+
+  // Post the task.
+  EXPECT_TRUE(posted_tasks_.empty());
+  PostTaskViaTracker(std::move(task_to_post));
+  ASSERT_EQ(1U, posted_tasks_.size());
+  EXPECT_EQ(task_to_post_raw, posted_tasks_.front().get());
+
+  // Post a BLOCK_SHUTDOWN task just to block shutdown.
+  PostTaskViaTracker(CreateTask(TaskShutdownBehavior::BLOCK_SHUTDOWN));
+
+  // Call Shutdown() asynchronously.
+  CallShutdownAsync();
+  VerifyAsyncShutdownInProgress();
+
+  // Try to run task posted at the beginning of this test. Only BLOCK_SHUTDOWN
+  // tasks should run, others should be discarded.
+  EXPECT_EQ(0U, num_tasks_executed_);
+  RunNextPostedTaskViaTracker();
+  EXPECT_EQ(GetParam() == TaskShutdownBehavior::BLOCK_SHUTDOWN ? 1U : 0U,
+            num_tasks_executed_);
+  VerifyAsyncShutdownInProgress();
+
+  // Unblock shutdown by running the remaining BLOCK_SHUTDOWN task.
+  RunNextPostedTaskViaTracker();
+  EXPECT_EQ(GetParam() == TaskShutdownBehavior::BLOCK_SHUTDOWN ? 2U : 1U,
+            num_tasks_executed_);
+  WaitForAsyncShutdownCompleted();
+}
+
+TEST_P(TaskSchedulerTaskTrackerTest, PostBeforeShutdownRunAfterShutdown) {
+  scoped_ptr<Task> task_to_post(CreateTask(GetParam()));
+  const Task* task_to_post_raw = task_to_post.get();
+
+  // Post the task.
+  EXPECT_TRUE(posted_tasks_.empty());
+  PostTaskViaTracker(std::move(task_to_post));
+  ASSERT_EQ(1U, posted_tasks_.size());
+  ASSERT_EQ(task_to_post_raw, posted_tasks_.front().get());

[robliao, 2016/03/17 23:34:21]

Scrub the usages of ASSERT vs EXPECT as well. This specific check is sometimes
EXPECT above.

[fdoray, 2016/03/18 20:35:39]

Done.

+
+  // Call Shutdown() asynchronously.
+  CallShutdownAsync();
+  EXPECT_EQ(0U, num_tasks_executed_);
+
+  if (GetParam() == TaskShutdownBehavior::BLOCK_SHUTDOWN) {
+    VerifyAsyncShutdownInProgress();
+
+    // Run the task to unblock shutdown.
+    RunNextPostedTaskViaTracker();
+    EXPECT_EQ(1U, num_tasks_executed_);
+    WaitForAsyncShutdownCompleted();
+
+    // It is not possible to test running after shutdown a BLOCK_SHUTDOWN task

[gab, 2016/03/18 18:48:17]

I think posting a BLOCK_SHUTDOWN task after shutdown should also DCHECK though.
Given the intent is to catch anybody posting shutdown blocking work after
shutdown we need to catch it on post more than execution.

So we should still keep the DCHECK in execution because that's *really*
unexpected but should also DCHECK in Post and this is what we should test here
IMO.

[fdoray, 2016/03/18 20:35:39]

Failure when a BLOCK_SHUTDOWN task is posted after shutdown is tested in
TaskSchedulerTaskTrackerTest.PostAfterShutdown. Should I keep the
EXPECT_DCHECK_DEATH here to check failure when a task is *run* after shutdown?

[gab, 2016/03/21 17:42:30]

Ok thanks, see comment in product, not sure this is BLOCK_SHUTDOWN only. But
yes, do test whatever the product code ends up implementing (with updated test
comment).

[fdoray, 2016/03/21 19:08:07]

I think it's BLOCK_SHUTDOWN only (see product). I kept the test as-is.

+    // that has been posted before shutdown because Shutdown() won't return if
+    // there are pending BLOCK_SHUTDOWN tasks.
+
+    // Make sure that a BLOCK_SHUTDOWN task doesn't run after shutdown (the
+    // task has never been posted).
+    EXPECT_DCHECK_DEATH(
+        {
+          tracker_.RunTask(
+              CreateTask(TaskShutdownBehavior::BLOCK_SHUTDOWN).get());
+        },
+        "");
+  } else {
+    WaitForAsyncShutdownCompleted();
+
+    // The task shouldn't be allowed to run after shutdown.
+    RunNextPostedTaskViaTracker();
+    EXPECT_EQ(0U, num_tasks_executed_);
+  }
+}
+
+TEST_P(TaskSchedulerTaskTrackerTest, PostAndRunDuringShutdown) {
+  // Post a BLOCK_SHUTDOWN task just to block shutdown.
+  PostTaskViaTracker(CreateTask(TaskShutdownBehavior::BLOCK_SHUTDOWN));
+  scoped_ptr<Task> block_shutdown_task = std::move(posted_tasks_.front());
+  posted_tasks_.pop();
+
+  // Call Shutdown() asynchronously.
+  CallShutdownAsync();
+  VerifyAsyncShutdownInProgress();
+
+  if (GetParam() == TaskShutdownBehavior::BLOCK_SHUTDOWN) {
+    // Post a task. This should succeed.
+    EXPECT_TRUE(posted_tasks_.empty());
+    PostTaskViaTracker(CreateTask(GetParam()));
+    EXPECT_EQ(1U, posted_tasks_.size());
+
+    // Run the task. This should succeed.
+    EXPECT_EQ(0U, num_tasks_executed_);
+    RunNextPostedTaskViaTracker();
+    EXPECT_EQ(1U, num_tasks_executed_);
+  } else {
+    // It shouldn't be possible to post a task which isn't BLOCK_SHUTDOWN.
+    PostTaskViaTracker(CreateTask(GetParam()));
+    EXPECT_TRUE(posted_tasks_.empty());
+
+    // Don't try to run the task, because it hasn't been posted successfully.
+  }
+
+  // Unblock shutdown by running the BLOCK_SHUTDOWN task posted at the beginning
+  // of the test.
+  VerifyAsyncShutdownInProgress();
+  tracker_.RunTask(block_shutdown_task.get());
+  EXPECT_EQ(GetParam() == TaskShutdownBehavior::BLOCK_SHUTDOWN ? 2U : 1U,
+            num_tasks_executed_);
+  WaitForAsyncShutdownCompleted();
+}
+
+TEST_P(TaskSchedulerTaskTrackerTest, PostAfterShutdown) {
+  // It is not possible to post a task after shutdown.
+  tracker_.Shutdown();
+  EXPECT_TRUE(posted_tasks_.empty());
+  PostTaskViaTracker(CreateTask(GetParam()));
+  EXPECT_TRUE(posted_tasks_.empty());
+}
+
+INSTANTIATE_TEST_CASE_P(
+    ContinueOnShutdown,
+    TaskSchedulerTaskTrackerTest,
+    ::testing::Values(TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN));
+INSTANTIATE_TEST_CASE_P(
+    SkipOnShutdown,
+    TaskSchedulerTaskTrackerTest,
+    ::testing::Values(TaskShutdownBehavior::SKIP_ON_SHUTDOWN));
+INSTANTIATE_TEST_CASE_P(
+    BlockShutdown,
+    TaskSchedulerTaskTrackerTest,
+    ::testing::Values(TaskShutdownBehavior::BLOCK_SHUTDOWN));
+
+}  // namespace internal
+}  // namespace base