Make SchedulerSingleThreadTaskRunner::RunsTasksOnCurrentThread sequence-affine.

base/task_scheduler/scheduler_worker_pool_impl_unittest.cc

Index: base/task_scheduler/scheduler_worker_pool_impl_unittest.cc
diff --git a/base/task_scheduler/scheduler_worker_pool_impl_unittest.cc b/base/task_scheduler/scheduler_worker_pool_impl_unittest.cc
index 1858ac5cdbd972cd80e88adb6bf1de8631c4c3e4..fb25da745acc9ddfcdf5bf7ab3f7084d97fd1768 100644
--- a/base/task_scheduler/scheduler_worker_pool_impl_unittest.cc
+++ b/base/task_scheduler/scheduler_worker_pool_impl_unittest.cc
@@ -364,6 +364,42 @@ TEST_P(TaskSchedulerWorkerPoolImplTest, SequencedRunsTasksOnCurrentThread) {
   task_ran.Wait();
 }
 
+// Verify that the RunsTasksOnCurrentThread() method of a SINGLE_THREADED

[fdoray, 2016/11/04 15:20:43]

s/SINGLE_THREADED TaskRunner/SingleThreadTaskRunner/

I can remove remaining ExecutionModes from base/task_scheduler/ comments in a
separate CL.

[gab, 2016/11/04 17:56:13]

Okay but I used SchedulerSingleThreadTaskRunner as otherwise it's too generic.

+// TaskRunner returns false when called from a task that isn't part of its
+// sequence. Even when there are enough TaskRunners of the tested types for some
+// tasks to be assigned to the same worker.

[robliao, 2016/11/03 22:59:56]

If we stop round robining the workers for single threaded tasks, this test would
no longer work, right?

Could use a TLS to assert that this is the first task to run on a particular
worker to detect overlapping tasks and have the tasks saturate the pool.

[gab, 2016/11/03 23:17:44]

It would still work. Note that this test is parametrized and the flood is tested
in all 3 ExecutionModes. So long as one of these tasks as *a chance* to run on
the thread assigned to |single_thread_task_runner| then this test would at least
become flaky. But with 50 tasks I think it's pretty much guaranteed to fail (it
currently reliably fails in all 3 modes without the matching impl change in this
CL).

Using TLS to ensure that at least one of the task has touched this thread would
on the other hand make the test susceptible to false positive flakes (i.e. every
now and then it's possible that no tasks use the assigned thread).

[robliao, 2016/11/03 23:29:32]

Ah, in that case would it be sufficient to simply saturate the pool instead with
kNumWorkersInWorkerPool and have each task wait?

We then signal all of the tasks after saturation.

[gab, 2016/11/03 23:39:50]

This could deadlock in the SINGLE_THREADED case (e.g. if we don't assign workers
in round-robin then it's possible that we don't fill the threads and wait
forever for them to be).

[robliao, 2016/11/04 00:56:47]

I think I would prefer a deadlocked test if/when we change the policy rather
than a flaky test.

I don't think it's possible for this test to be robust to any scheduling policy
we throw at it. However, it would be good to know that the coverage is similar
in the face of a changing policy. If the test deadlocks, then at least we know
this test should be updated.

[gab, 2016/11/04 14:47:59]

I disagree, the worker assignment policy is orthogonal to this test and as such
it's incorrect to intentionally have this test depend on the existing policy
IMO.

With 50 tasks I seriously doubt that this test would incorrectly pass as a false
negative flake very often if it became broken.

[fdoray, 2016/11/04 15:20:43]

After reading the "Resilience" section from
https://testing.googleblog.com/2014/05/testing-on-toilet-effective-testing.html
, I don't think this test should have to change if the thread assignment policy
changes.

A good compromise to be absolutely sure that this test fails when
RunsTasksOnCurrentThread() returns true for tasks posted to different
SingleThreadTaskRunners that happen to run on the same thread would be to use a
SchedulerWorkerPoolImpl with |max_threads| = 1.

[robliao, 2016/11/04 16:59:36]

I think I would be okay with this. That's a good compromise!

[gab, 2016/11/04 17:56:13]

I like this too, done.

+TEST_P(TaskSchedulerWorkerPoolImplTest, SingleThreadRunsTasksOnCurrentThread) {
+  constexpr size_t kNumFloodingTaskRunners = 50;

[robliao, 2016/11/03 22:59:56]

Nit: Move this constexpr to the group near the top of the file.

[gab, 2016/11/03 23:17:44]

No, it's specific to this test so it's preferred to keep it local (same is true
of say a string used in only one test body or a single method in a regular .cc
file).

+  std::vector<scoped_refptr<TaskRunner>> task_runners;
+  std::vector<std::unique_ptr<WaitableEvent>> task_signals;
+  for (size_t i = 0; i < kNumFloodingTaskRunners; ++i) {
+    task_runners.push_back(
+        CreateTaskRunnerWithExecutionMode(worker_pool_.get(), GetParam()));
+    task_signals.emplace_back(
+        new WaitableEvent(WaitableEvent::ResetPolicy::MANUAL,
+                          WaitableEvent::InitialState::NOT_SIGNALED));
+  }
+
+  scoped_refptr<SingleThreadTaskRunner> single_thread_task_runner(
+      worker_pool_->CreateSingleThreadTaskRunnerWithTraits(TaskTraits()));
+
+  for (size_t i = 0; i < kNumFloodingTaskRunners; ++i) {
+    task_runners[i]->PostTask(
+        FROM_HERE,
+        Bind(
+            [](scoped_refptr<TaskRunner> single_thread_task_runner,
+               WaitableEvent* task_signal) {
+              EXPECT_FALSE(
+                  single_thread_task_runner->RunsTasksOnCurrentThread());
+              task_signal->Signal();
+            },
+            single_thread_task_runner, Unretained(task_signals[i].get())));
+  }
+
+  for (auto& task_signal : task_signals)
+    task_signal->Wait();
+}
+
 INSTANTIATE_TEST_CASE_P(Parallel,
                         TaskSchedulerWorkerPoolImplTest,
                         ::testing::Values(test::ExecutionMode::PARALLEL));