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

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

SequencedWorkerPool tasks with explicit shutdown behaviours should have the following semantics:

For tasks posted, but not started, before Shutdown is called:
BLOCK_: MUST run
SKIP_: MUST NOT run
CONTINUE_: MUST NOT run

For tasks started, but not completed, when Shutdown is called:
BLOCK_: MUST block Shutdown
SKIP_ MUST block Shutdown
CONTINUE_: MUST NOT block Shutdown

For tasks posted *AFTER* Shutdown is called, for all three cases, the new tasks MUST NOT be run.

BUG=138120
TEST=base_unittests:SequencedWorkerPoolTest.SkipOnShutdown


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=147776

[Ryan Sleevi, 2012-07-19 22:05:48]

jar: Review
akalin: FYI and maybe sanity check :)

[jar (doing other things), 2012-07-20 01:46:33]

IMO, you should also edit the header file, where SKIP_ON_SHUTDOWN is described. 
You should illuminate the caveat that this will block shutdown (said more
explicitly than "it will continue executing.")

Good catch!!!!!!

http://codereview.chromium.org/10807045/diff/3001/base/threading/sequenced_wo...
File base/threading/sequenced_worker_pool.cc (right):

http://codereview.chromium.org/10807045/diff/3001/base/threading/sequenced_wo...
base/threading/sequenced_worker_pool.cc:376: size_t
blocking_shutdown_thread_count_;
nit: You need to correct the comment.

Suggest:

"... that have the BLOCK_SHUTDOWN or SKIP_ON_SHUTDOWN flag set."

[Ryan Sleevi, 2012-07-20 01:58:24]

Comments about comments addressed. Was that a LGTY, or were you waiting for the
comment updates?

[Ryan Sleevi, 2012-07-20 02:03:07]

On 2012/07/20 01:58:24, Ryan Sleevi wrote:
> Comments about comments addressed. Was that a LGTY, or were you waiting for
the
> comment updates?

Updated the CL description as well to make it clear where the intersections are
between the three.

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

http://codereview.chromium.org/10807045/diff/4003/base/threading/sequenced_wo...
File base/threading/sequenced_worker_pool_unittest.cc (right):

http://codereview.chromium.org/10807045/diff/4003/base/threading/sequenced_wo...
base/threading/sequenced_worker_pool_unittest.cc:301: EXPECT_EQ(2*kNumTasks,
result.size());
nit: (though again, not yours) spaces around *

http://codereview.chromium.org/10807045/diff/4003/base/threading/sequenced_wo...
base/threading/sequenced_worker_pool_unittest.cc:349: std::vector<int> result =
tracker()->WaitUntilTasksComplete(3);
kNumWorkerThreads

http://codereview.chromium.org/10807045/diff/4003/base/threading/sequenced_wo...
base/threading/sequenced_worker_pool_unittest.cc:439:
tracker()->WaitUntilTasksBlocked(3);
Should this be kNumWorkerThreads instead of 3?

http://codereview.chromium.org/10807045/diff/4003/base/threading/sequenced_wo...
base/threading/sequenced_worker_pool_unittest.cc:458: std::vector<int> result =
tracker()->WaitUntilTasksComplete(3);
nit: kNumWorkerThreads?

http://codereview.chromium.org/10807045/diff/4003/base/threading/sequenced_wo...
base/threading/sequenced_worker_pool_unittest.cc:498: pool()->Shutdown();
Do you think there is a another test where we post a BLOCK_SHUTDOWN task after
calling shutdown?

http://codereview.chromium.org/10807045/diff/4003/base/threading/sequenced_wo...
base/threading/sequenced_worker_pool_unittest.cc:504: // Only taks marked
SKIP_ON_SHUTDOWN that were already started should be
nit: taks-->tasks

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

http://codereview.chromium.org/10807045/diff/4003/base/threading/sequenced_wo...
File base/threading/sequenced_worker_pool_unittest.cc (right):

http://codereview.chromium.org/10807045/diff/4003/base/threading/sequenced_wo...
base/threading/sequenced_worker_pool_unittest.cc:349: std::vector<int> result =
tracker()->WaitUntilTasksComplete(3);
On 2012/07/20 02:21:17, jar wrote:
> kNumWorkerThreads

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)

http://codereview.chromium.org/10807045/diff/4003/base/threading/sequenced_wo...
base/threading/sequenced_worker_pool_unittest.cc:439:
tracker()->WaitUntilTasksBlocked(3);
On 2012/07/20 02:21:17, jar wrote:
> Should this be kNumWorkerThreads instead of 3?

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

http://codereview.chromium.org/10807045/diff/4003/base/threading/sequenced_wo...
base/threading/sequenced_worker_pool_unittest.cc:458: std::vector<int> result =
tracker()->WaitUntilTasksComplete(3);
On 2012/07/20 02:21:17, jar wrote:
> nit: kNumWorkerThreads?

Same 'magic' constant from above.

http://codereview.chromium.org/10807045/diff/4003/base/threading/sequenced_wo...
base/threading/sequenced_worker_pool_unittest.cc:498: pool()->Shutdown();
On 2012/07/20 02:21:17, jar wrote:
> Do you think there is a another test where we post a BLOCK_SHUTDOWN task after
> calling shutdown?

No, but I've added one.

[Ryan Sleevi, 2012-07-20 23:53:53]

jar: Addressed a few of your comments from in-person.

I tried to be conservative here in order to ensure that failures remain
deterministic (eg: they hang), rather than relying on any conditions that may
race between the test thread and the workers (eg: FAIL() in a worker thread,
checking in the main thread that the worker hasn't set some CV, etc).

Checking that no work has been signaled seems a reasonable and safe way to
further guarantee the underlying conditions, and to make sure that none of the
PostTask* variants are returning false but actually posting tasks.

PTAL

[jar (doing other things), 2012-07-21 00:39:51]

lgtm

[commit-bot: I haz the power, 2012-07-21 00:55:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsleevi@chromium.org/10807045/1005

[commit-bot: I haz the power, 2012-07-21 01:07:54]

Try job failure for 10807045-1005 (retry) on mac_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-07-21 01:32:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsleevi@chromium.org/10807045/8003

[commit-bot: I haz the power, 2012-07-21 04:58:06]

Change committed as 147776