Ensure SequencedWorkerPool::Shutdown() blocks for already-started SKIP_ON_SHUTDOWN tasks

base/threading/sequenced_worker_pool_unittest.cc

 // Copyright (c) 2012 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/threading/sequenced_worker_pool.h"
 
 #include <algorithm>
 
 #include "base/bind.h"
 #include "base/compiler_specific.h"
@@ skipping 280 matching lines @@
   const size_t kNumTasks = 20;
   for (size_t i = 1; i < kNumTasks; i++) {
     base::Closure fast_task =
         base::Bind(&TestTracker::FastTask, tracker(), i);
     pool1.pool()->PostWorkerTask(FROM_HERE, fast_task);
     pool2.pool()->PostWorkerTask(FROM_HERE, fast_task);
   }
 
   std::vector<int> result =
       tracker()->WaitUntilTasksComplete(2*kNumTasks);
   EXPECT_EQ(2*kNumTasks, result.size());

[jar (doing other things), 2012/07/20 02:21:17]

nit: (though again, not yours) spaces around *

 
   pool2.pool()->Shutdown();
   pool1.pool()->Shutdown();
 }
 
 // Test that tasks with the same sequence token are executed in order but don't
 // affect other tasks.
 TEST_F(SequencedWorkerPoolTest, Sequence) {
   // Fill all the worker threads except one.
   const size_t kNumBackgroundTasks = kNumWorkerThreads - 1;
@@ skipping 27 matching lines @@
       base::Bind(&TestTracker::FastTask, tracker(), 200));
   pool()->PostSequencedWorkerTask(
       token2, FROM_HERE,
       base::Bind(&TestTracker::FastTask, tracker(), 201));
   EXPECT_EQ(0u, tracker()->WaitUntilTasksComplete(0).size());
 
   // Let one background task complete. This should then let both tasks of
   // token2 run to completion in order. The second task of token1 should still
   // be blocked.
   background_blocker.Unblock(1);
   std::vector<int> result = tracker()->WaitUntilTasksComplete(3);

[jar (doing other things), 2012/07/20 02:21:17]

kNumWorkerThreads

[Ryan Sleevi, 2012/07/20 02:53:59]

This is correct as-is. "3" refers to the one parent task and the one child task,
and the one blocker task that is unblocked. No relation to the number of threads
here (and in fact, they all run on a single worker thread)

   ASSERT_EQ(3u, result.size());
   EXPECT_EQ(200, result[1]);
   EXPECT_EQ(201, result[2]);
 
   // Finish the rest of the background tasks. This should leave some workers
   // free with the second token1 task still blocked on the first.
   background_blocker.Unblock(kNumBackgroundTasks - 1);
   EXPECT_EQ(kNumBackgroundTasks + 2,
             tracker()->WaitUntilTasksComplete(kNumBackgroundTasks + 2).size());
 
@@ skipping 32 matching lines @@
       base::Bind(&TestTracker::FastTask, tracker(), 102),
       SequencedWorkerPool::BLOCK_SHUTDOWN);
 
   // Shutdown the worker pool. This should discard all non-blocking tasks.
   SetWillWaitForShutdownCallback(
       base::Bind(&EnsureTasksToCompleteCountAndUnblock,
                  scoped_refptr<TestTracker>(tracker()), 0,
                  &blocker, kNumWorkerThreads));
   pool()->Shutdown();
 
-  std::vector<int> result = tracker()->WaitUntilTasksComplete(4);
+  std::vector<int> result =
+      tracker()->WaitUntilTasksComplete(kNumWorkerThreads + 1);
 
   // The kNumWorkerThread items should have completed, plus the BLOCK_SHUTDOWN
   // one, in no particular order.
-  ASSERT_EQ(4u, result.size());
+  ASSERT_EQ(kNumWorkerThreads + 1, result.size());
   for (size_t i = 0; i < kNumWorkerThreads; i++) {
     EXPECT_TRUE(std::find(result.begin(), result.end(), static_cast<int>(i)) !=
                 result.end());
   }
   EXPECT_TRUE(std::find(result.begin(), result.end(), 102) != result.end());
 }
 
 // Tests that CONTINUE_ON_SHUTDOWN tasks don't block shutdown.
 TEST_F(SequencedWorkerPoolTest, ContinueOnShutdown) {
   scoped_refptr<TaskRunner> runner(pool()->GetTaskRunnerWithShutdownBehavior(
@@ skipping 11 matching lines @@
       SequencedWorkerPool::CONTINUE_ON_SHUTDOWN);
   runner->PostTask(
       FROM_HERE,
       base::Bind(&TestTracker::BlockTask,
                  tracker(), 1, &blocker));
   sequenced_runner->PostTask(
       FROM_HERE,
       base::Bind(&TestTracker::BlockTask,
                  tracker(), 2, &blocker));
 
   tracker()->WaitUntilTasksBlocked(3);

[jar (doing other things), 2012/07/20 02:21:17]

Should this be kNumWorkerThreads instead of 3?

[Ryan Sleevi, 2012/07/20 02:53:59]

Unfortunately, no. It refers to the three tasks posted here on 425-439.

The reason why they're posted in this manner, as opposed to say, in a loop while
i < kNumWorkerThreads, is that this test is making sure that
CONTINUE_ON_SHUTDOWN works with the three different exposed APIs -
pool()->PostWorkerTaskWithShutdownBehavior,
TaskRunnerWithShutdownBehavior()->PostTask, and
SequencedTaskRunnerWithShutdownBehaviour()->PostTask

 
   // This should not block. If this test hangs, it means it failed.
   pool()->Shutdown();
 
   // The task should not have completed yet.
   EXPECT_EQ(0u, tracker()->WaitUntilTasksComplete(0).size());
 
   // Posting more tasks should fail.
   EXPECT_FALSE(pool()->PostWorkerTaskWithShutdownBehavior(
       FROM_HERE, base::Bind(&TestTracker::FastTask, tracker(), 0),
       SequencedWorkerPool::CONTINUE_ON_SHUTDOWN));
   EXPECT_FALSE(runner->PostTask(
       FROM_HERE, base::Bind(&TestTracker::FastTask, tracker(), 0)));
   EXPECT_FALSE(sequenced_runner->PostTask(
       FROM_HERE, base::Bind(&TestTracker::FastTask, tracker(), 0)));
 
   // Continue the background thread and make sure the tasks can complete.
   blocker.Unblock(3);
   std::vector<int> result = tracker()->WaitUntilTasksComplete(3);

[jar (doing other things), 2012/07/20 02:21:17]

nit: kNumWorkerThreads?

[Ryan Sleevi, 2012/07/20 02:53:59]

Same 'magic' constant from above.

   EXPECT_EQ(3u, result.size());
 }
 
+// Tests that SKIP_ON_SHUTDOWN tasks that have been started block Shutdown
+// until they stop, but tasks not yet started do not.
+TEST_F(SequencedWorkerPoolTest, SkipOnShutdown) {
+  // Start tasks to take all the threads and block them.
+  EnsureAllWorkersCreated();
+  ThreadBlocker blocker;
+
+  // Now block all the threads with SKIP_ON_SHUTDOWN. Shutdown() should not
+  // return until these tasks have completed.
+  for (size_t i = 0; i < kNumWorkerThreads; i++) {
+    pool()->PostWorkerTaskWithShutdownBehavior(
+        FROM_HERE,
+        base::Bind(&TestTracker::BlockTask, tracker(), i, &blocker),
+        SequencedWorkerPool::SKIP_ON_SHUTDOWN);
+  }
+  tracker()->WaitUntilTasksBlocked(kNumWorkerThreads);
+
+  // Now post an additional task as SKIP_ON_SHUTDOWN, which should not be
+  // executed once Shutdown() has been called.
+  pool()->PostWorkerTaskWithShutdownBehavior(
+      FROM_HERE,
+      base::Bind(&TestTracker::BlockTask,
+                 tracker(), 0, &blocker),
+      SequencedWorkerPool::SKIP_ON_SHUTDOWN);
+
+  // This callback will only be invoked if SKIP_ON_SHUTDOWN tasks that have
+  // been started block shutdown.
+  SetWillWaitForShutdownCallback(
+      base::Bind(&EnsureTasksToCompleteCountAndUnblock,
+                 scoped_refptr<TestTracker>(tracker()), 0,
+                 &blocker, kNumWorkerThreads));
+
+  // No tasks should have completed yet.
+  EXPECT_EQ(0u, tracker()->WaitUntilTasksComplete(0).size());
+
+  // This should not block. If this test hangs, it means it failed.
+  pool()->Shutdown();

[jar (doing other things), 2012/07/20 02:21:17]

Do you think there is a another test where we post a BLOCK_SHUTDOWN task after
calling shutdown?

[Ryan Sleevi, 2012/07/20 02:53:59]

No, but I've added one.

+
+  // Shutdown should not return until all of the tasks have completed.
+  std::vector<int> result =
+      tracker()->WaitUntilTasksComplete(kNumWorkerThreads);
+
+  // Only taks marked SKIP_ON_SHUTDOWN that were already started should be

[jar (doing other things), 2012/07/20 02:21:17]

nit: taks-->tasks

+  // allowed to complete. No additional non-blocking tasks should have been
+  // started.
+  ASSERT_EQ(kNumWorkerThreads, result.size());
+  for (size_t i = 0; i < kNumWorkerThreads; i++) {
+    EXPECT_TRUE(std::find(result.begin(), result.end(), static_cast<int>(i)) !=
+                result.end());
+  }
+}
+
 // Ensure all worker threads are created, and then trigger a spurious
 // work signal. This shouldn't cause any other work signals to be
 // triggered. This is a regression test for http://crbug.com/117469.
 TEST_F(SequencedWorkerPoolTest, SpuriousWorkSignal) {
   EnsureAllWorkersCreated();
   int old_has_work_call_count = has_work_call_count();
   pool()->SignalHasWorkForTesting();
   // This is inherently racy, but can only produce false positives.
   base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(100));
   EXPECT_EQ(old_has_work_call_count + 1, has_work_call_count());
@@ skipping 169 matching lines @@
     SequencedWorkerPoolSequencedTaskRunner, TaskRunnerTest,
     SequencedWorkerPoolSequencedTaskRunnerTestDelegate);
 
 INSTANTIATE_TYPED_TEST_CASE_P(
     SequencedWorkerPoolSequencedTaskRunner, SequencedTaskRunnerTest,
     SequencedWorkerPoolSequencedTaskRunnerTestDelegate);
 
 }  // namespace
 
 }  // namespace base