WILL BE MERGED Change Ownership of Sequence to the Single Thread SchedulerWorker Delegate

base/task_scheduler/scheduler_single_thread_task_runner_manager_unittest.cc

Index: base/task_scheduler/scheduler_single_thread_task_runner_manager_unittest.cc
diff --git a/base/task_scheduler/scheduler_single_thread_task_runner_manager_unittest.cc b/base/task_scheduler/scheduler_single_thread_task_runner_manager_unittest.cc
index 6db240a743cd85b0f84978cb1483d41075131c66..2439950433ed2c4e2294b551ec0639504d1308d1 100644
--- a/base/task_scheduler/scheduler_single_thread_task_runner_manager_unittest.cc
+++ b/base/task_scheduler/scheduler_single_thread_task_runner_manager_unittest.cc
@@ -14,6 +14,7 @@
 #include "base/task_scheduler/task_tracker.h"
 #include "base/task_scheduler/task_traits.h"
 #include "base/test/test_timeouts.h"
+#include "base/threading/simple_thread.h"
 #include "base/threading/thread.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -22,7 +23,7 @@ namespace internal {
 
 namespace {
 
-enum WorkerPoolType {
+enum WorkerPoolType : size_t {
   BACKGROUND_WORKER_POOL = 0,
   FOREGROUND_WORKER_POOL,
 };
@@ -32,6 +33,22 @@ static size_t GetThreadPoolIndexForTraits(const TaskTraits& traits) {
                                                        : FOREGROUND_WORKER_POOL;
 }
 
+std::vector<SchedulerWorkerPoolParams> GetParamsVector() {
+  using StandbyThreadPolicy = SchedulerWorkerPoolParams::StandbyThreadPolicy;
+
+  std::vector<SchedulerWorkerPoolParams> params_vector;
+
+  DCHECK_EQ(BACKGROUND_WORKER_POOL, params_vector.size());
+  params_vector.emplace_back("Background", ThreadPriority::BACKGROUND,
+                             StandbyThreadPolicy::LAZY, 1U, TimeDelta::Max());
+
+  DCHECK_EQ(FOREGROUND_WORKER_POOL, params_vector.size());

[gab, 2017/03/15 20:00:13]

EXPECT_EQ instead of DCHECK_EQ in tests.

[robliao, 2017/03/15 20:40:16]

I would prefer to fail fast here since there's no point in continuing the test
if any of these checks fail.

If these conditions are violated we're in one of the following scenarios:
1) Someone swapped the order. The tests will fail but won't crash the shard.
2) Someone added some worker pool types but forgot to update the mapper function
and params vector generation. We could crash the shard due to vector out of
bounds accesses.

Since we could still crash the shard, we might as well crash early.

+  params_vector.emplace_back("Foreground", ThreadPriority::NORMAL,
+                             StandbyThreadPolicy::LAZY, 1U, TimeDelta::Max());
+
+  return params_vector;
+}
+
 class TaskSchedulerSingleThreadTaskRunnerManagerTest : public testing::Test {
  public:
   TaskSchedulerSingleThreadTaskRunnerManagerTest()
@@ -40,34 +57,26 @@ class TaskSchedulerSingleThreadTaskRunnerManagerTest : public testing::Test {
   void SetUp() override {
     service_thread_.Start();
 
-    using StandbyThreadPolicy = SchedulerWorkerPoolParams::StandbyThreadPolicy;
-
-    std::vector<SchedulerWorkerPoolParams> params_vector;
-
-    ASSERT_EQ(BACKGROUND_WORKER_POOL, params_vector.size());
-    params_vector.emplace_back("Background", ThreadPriority::BACKGROUND,
-                               StandbyThreadPolicy::LAZY, 1U, TimeDelta::Max());
-
-    ASSERT_EQ(FOREGROUND_WORKER_POOL, params_vector.size());
-    params_vector.emplace_back("Foreground", ThreadPriority::NORMAL,
-                               StandbyThreadPolicy::LAZY, 1U, TimeDelta::Max());
-
     delayed_task_manager_ =
         MakeUnique<DelayedTaskManager>(service_thread_.task_runner());
     single_thread_task_runner_manager_ =
         MakeUnique<SchedulerSingleThreadTaskRunnerManager>(
-            params_vector, Bind(&GetThreadPoolIndexForTraits), &task_tracker_,
-            delayed_task_manager_.get());
+            GetParamsVector(), Bind(&GetThreadPoolIndexForTraits),
+            &task_tracker_, delayed_task_manager_.get());
   }
 
   void TearDown() override {
-    single_thread_task_runner_manager_->JoinForTesting();
-    single_thread_task_runner_manager_.reset();
+    TearDownSingleThreadTaskRunnerManager();
     delayed_task_manager_.reset();
     service_thread_.Stop();
   }
 
  protected:
+  virtual void TearDownSingleThreadTaskRunnerManager() {

[gab, 2017/03/15 20:00:13]

Instead of having this virtual method simply to skip the join call, keep it
inline in TearDown() but only do it if (single_thread_task_runner_manager_),
allowing early reset in fixtures that care? (i.e your other fixture can override
TearDown() and do its thing before handing off to its parent's TearDown()?)

[robliao, 2017/03/15 20:40:16]

That seemed pretty subtle.
It's nicer when a class can fully control the validity of the pointers. A
virtual method call is super cheap in that this is just a test. This way we
don't have to check in each test if the pointers are cleared by the test.

+    single_thread_task_runner_manager_->JoinForTesting();
+    single_thread_task_runner_manager_.reset();
+  }
+
   std::unique_ptr<SchedulerSingleThreadTaskRunnerManager>
       single_thread_task_runner_manager_;
   TaskTracker task_tracker_;
@@ -250,5 +259,101 @@ TEST_F(TaskSchedulerSingleThreadTaskRunnerManagerTest,
   task_tracker_.Shutdown();
 }
 
+namespace {
+
+class CallJoinFromDifferentThread : public SimpleThread {
+ public:
+  CallJoinFromDifferentThread(
+      SchedulerSingleThreadTaskRunnerManager* manager_to_join)
+      : SimpleThread("SchedulerSingleThreadTaskRunnerManagerJoinThread"),
+        manager_to_join_(manager_to_join),
+        run_started_event_(WaitableEvent::ResetPolicy::MANUAL,
+                           WaitableEvent::InitialState::NOT_SIGNALED) {}
+
+  ~CallJoinFromDifferentThread() override = default;
+
+  void Run() override {
+    run_started_event_.Signal();
+    manager_to_join_->JoinForTesting();
+  }
+
+  void WaitForRunToStart() { run_started_event_.Wait(); }
+
+ private:
+  SchedulerSingleThreadTaskRunnerManager* const manager_to_join_;
+  WaitableEvent run_started_event_;
+  DISALLOW_COPY_AND_ASSIGN(CallJoinFromDifferentThread);

[gab, 2017/03/15 20:00:13]

empty line before DISALLOW...() macro

[robliao, 2017/03/15 20:40:16]

Done.

+};
+
+class TaskSchedulerSingleThreadTaskRunnerManagerJoinTest
+    : public TaskSchedulerSingleThreadTaskRunnerManagerTest {
+ public:
+  TaskSchedulerSingleThreadTaskRunnerManagerJoinTest() = default;
+  ~TaskSchedulerSingleThreadTaskRunnerManagerJoinTest() override = default;
+
+ protected:
+  void TearDownSingleThreadTaskRunnerManager() override {
+    // The tests themselves are responsible for calling JoinForTesting().
+    single_thread_task_runner_manager_.reset();
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(TaskSchedulerSingleThreadTaskRunnerManagerJoinTest);
+};
+
+}  // namespace
+
+TEST_F(TaskSchedulerSingleThreadTaskRunnerManagerJoinTest, ConcurrentJoin) {

[gab, 2017/03/15 20:00:13]

How is concurrent join stressing the code added in this CL?

[robliao, 2017/03/15 20:40:16]

Added a comment. Both of these tests cause two different failures without the
fix.

  // Exercises the codepath where the workers are unavailable for unregistration
  // because of a Join call.

+  WaitableEvent task_running(WaitableEvent::ResetPolicy::MANUAL,
+                             WaitableEvent::InitialState::NOT_SIGNALED);
+  WaitableEvent task_blocking(WaitableEvent::ResetPolicy::MANUAL,
+                              WaitableEvent::InitialState::NOT_SIGNALED);
+
+  {
+    auto task_runner = single_thread_task_runner_manager_
+                           ->CreateSingleThreadTaskRunnerWithTraits(
+                               TaskTraits().WithBaseSyncPrimitives());
+    EXPECT_TRUE(task_runner->PostTask(
+        FROM_HERE, Bind(&WaitableEvent::Signal, Unretained(&task_running))));
+    EXPECT_TRUE(task_runner->PostTask(
+        FROM_HERE, Bind(&WaitableEvent::Wait, Unretained(&task_blocking))));
+  }
+
+  task_running.Wait();
+  CallJoinFromDifferentThread join_from_different_thread(
+      single_thread_task_runner_manager_.get());
+  join_from_different_thread.Start();
+  join_from_different_thread.WaitForRunToStart();
+  task_blocking.Signal();
+  join_from_different_thread.Join();
+}
+
+TEST_F(TaskSchedulerSingleThreadTaskRunnerManagerJoinTest,
+       ConcurrentJoinExtraSkippedTask) {

[gab, 2017/03/15 20:00:13]

And how is this one different? Please document what each test is stressing,
otherwise we're just piling tests and it makes future refactorings more
cumbersome.

[robliao, 2017/03/15 20:40:16]

Added a comment.
  // Tests to make sure that tasks are properly cleaned up at Join, allowing
  // SingleThreadTaskRunners to unregister themselves.

+  WaitableEvent task_running(WaitableEvent::ResetPolicy::MANUAL,
+                             WaitableEvent::InitialState::NOT_SIGNALED);
+  WaitableEvent task_blocking(WaitableEvent::ResetPolicy::MANUAL,
+                              WaitableEvent::InitialState::NOT_SIGNALED);
+
+  {
+    auto task_runner = single_thread_task_runner_manager_
+                           ->CreateSingleThreadTaskRunnerWithTraits(
+                               TaskTraits().WithBaseSyncPrimitives());
+    EXPECT_TRUE(task_runner->PostTask(
+        FROM_HERE, Bind(&WaitableEvent::Signal, Unretained(&task_running))));
+    EXPECT_TRUE(task_runner->PostTask(
+        FROM_HERE, Bind(&WaitableEvent::Wait, Unretained(&task_blocking))));
+    EXPECT_TRUE(task_runner->PostTask(FROM_HERE, Bind(&DoNothing)));
+  }
+
+  task_running.Wait();
+  CallJoinFromDifferentThread join_from_different_thread(
+      single_thread_task_runner_manager_.get());
+  join_from_different_thread.Start();
+  join_from_different_thread.WaitForRunToStart();
+  task_blocking.Signal();
+  join_from_different_thread.Join();
+}
+
 }  // namespace internal
 }  // namespace base