Skip to content

Commit

Permalink
Fix TaskSchedulerWorkerPoolHistogramTest.NumTasksBeforeCleanup failur…
Browse files Browse the repository at this point in the history
…e on Fuschia.

This test assumes that 3 tasks posted to the same sequence from the
main thread will be scheduled on the same worker. This is incorrect
because of this:

1. Worker #1: Runs a tasks and empties the sequence, without adding
   itself to the idle stack yet.
2. Posting thread: Posts another task to the now empty sequence.
   Wakes up a new worker, since worker #1 isn't on the idle stack yet.
3. Worker #2: Runs the tasks, violating the expectation that the
   3 initial tasks run on the same worker.

This CL fixes the issue by starting the pool *after* the 3 tasks
have been posted.

Bug: 844009
Change-Id: Idcc74e8bea90b94ecba8e3a52abc4091c89044b2
Reviewed-on: https://chromium-review.googlesource.com/1064016
Reviewed-by: Gabriel Charette <gab@chromium.org>
Commit-Queue: François Doray <fdoray@chromium.org>
Cr-Commit-Position: refs/heads/master@{#559534}
  • Loading branch information
fdoray authored and Commit Bot committed May 17, 2018
1 parent aa70371 commit 0181d8b
Showing 1 changed file with 18 additions and 5 deletions.
23 changes: 18 additions & 5 deletions base/task_scheduler/scheduler_worker_pool_impl_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -688,11 +688,7 @@ TEST_F(TaskSchedulerWorkerPoolHistogramTest,
}

TEST_F(TaskSchedulerWorkerPoolHistogramTest, NumTasksBeforeCleanup) {
// Strictly use two workers for this test to avoid depending on the
// scheduler's logic to always keep one extra idle worker.
constexpr size_t kTwoWorkers = 2;
CreateAndStartWorkerPool(kReclaimTimeForCleanupTests, kTwoWorkers);

CreateWorkerPool();
auto histogrammed_thread_task_runner =
worker_pool_->CreateSequencedTaskRunnerWithTraits(
{WithBaseSyncPrimitives()});
Expand Down Expand Up @@ -736,6 +732,23 @@ TEST_F(TaskSchedulerWorkerPoolHistogramTest, NumTasksBeforeCleanup) {
Unretained(&thread_ref), Unretained(&cleanup_thread_running),
Unretained(&cleanup_thread_continue)));

// Start the worker pool with 2 workers, to avoid depending on the scheduler's
// logic to always keep one extra idle worker.
//
// The pool is started after the 3 initial tasks have been posted to ensure
// that they are scheduled on the same worker. If the tasks could run as they
// are posted, there would be a chance that:
// 1. Worker #1: Runs a tasks and empties the sequence, without adding
// itself to the idle stack yet.
// 2. Posting thread: Posts another task to the now empty sequence. Wakes
// up a new worker, since worker #1 isn't on the idle
// stack yet.
// 3: Worker #2: Runs the tasks, violating the expectation that the 3
// initial tasks run on the same worker.
constexpr size_t kTwoWorkers = 2;
StartWorkerPool(kReclaimTimeForCleanupTests, kTwoWorkers);

// Wait until the 3rd task is scheduled.
cleanup_thread_running.Wait();

// To allow the SchedulerWorker associated with
Expand Down

0 comments on commit 0181d8b

Please sign in to comment.