Use TaskScheduler instead of blocking pool in version_handler_chromeos.cc.

Use TaskScheduler instead of blocking pool in version_handler_chromeos.cc.

The blocking pool is being deprecated in favor of TaskScheduler.

BUG=667892
R=bauerb@chromium.org

Review-Url: https://codereview.chromium.org/2672023003
Cr-Commit-Position: refs/heads/master@{#448608}
Committed: https://chromium.googlesource.com/chromium/src/+/a8e8f1139422ceeef5ff8e0901a87eeafeb81f65

[fdoray, 2017-02-03 18:00:57]

Hi! I'm a Python script responsible for migrating tasks from the
blocking pool to TaskScheduler. In this CL, I used these traits to post to
TaskScheduler a task that was previously posted to the blocking pool:

.WithPriority(base::TaskPriority::BACKGROUND)
  User won't notice if the task takes an arbitrarily long time to complete.
  Making your task BACKGROUND allows non-BACKGROUND tasks to run faster :)

*No* .WithShutdownBehavior()

.MayBlock()
  The task may block. This includes but is not limited to tasks that wait on
  synchronous file I/O operations: read or write a file from disk, interact
  with a pipe or a socket, rename or delete a file, enumerate files in a
  directory, etc. This trait isn't required for the mere use of locks.

*No* .WithBaseSyncPrimitives()
  The task is not allowed to use these functions:
  - base::WaitableEvent::Wait
  - base::ConditionVariable::Wait
  - base::PlatformThread::Join
  - base::PlatformThread::Sleep
  - base::Process::WaitForExit
  - base::Process::WaitForExitWithTimeout

If you think this is correct, please LGTM and CQ this CL.

Otherwise, tell me which traits I should have used and I'll automatically
upload a new patch. You can find documentation at
https://cs.chromium.org/chromium/src/base/task_scheduler/task_traits.h

WithPriority()
[default/BACKGROUND/USER_VISIBLE/USER_BLOCKING]:

WithShutdownBehavior()
[default/CONTINUE_ON_SHUTDOWN/SKIP_ON_SHUTDOWN/BLOCK_SHUTDOWN]:

MayBlock()
[yes/no]:

WithBaseSyncPrimitives()
[yes/no]:

Comments for a human:
[A human will read this]

FAQ:

What should I do if the CQ dry run didn't pass?
You can ignore this CL. A human will take a look and get back to you.

Why are you doing this?
Browser threads, the blocking pool and base::WorkerPool are being deprecated
in favor of TaskScheduler. Design doc:
https://docs.google.com/document/d/1S2AAeoo1xa_vsLbDYBsDHCqhrkfiMgoIPlyRi6kxa...

What is the default priority?
A task posted to TaskScheduler without an explicit priority inherits its
priority from the calling context (e.g. a task posted to TaskScheduler
from a BACKGROUND task without an explicit .WithPriority() will have a
BACKGROUND priority).

What is the default shutdown behavior?
It is not documented on purpose. If your task shouldn't be skipped on shutdown
(e.g. a task that persists user data on disk), it should have an explicit
BLOCK_SHUTDOWN shutdown behavior. If it's important not to wait on your task
on shutdown (e.g. it takes a long time to run and could cause a shutdown
hang), it should have an explicit CONTINUE_ON_SHUTDOWN shutdown behavior.

[fdoray, 2017-02-03 18:00:58]

The CQ bit was checked by fdoray@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-03 18:01:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-03 18:41:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-03 18:41:53]

Dry run: This issue passed the CQ dry run.

[Bernhard Bauer, 2017-02-06 14:40:01]

bauerb@chromium.org changed reviewers:
+ stevenjb@chromium.org

[Bernhard Bauer, 2017-02-06 14:40:02]

+stevenjb

This change seems fine to me, but I'm a bit confused about the method that ends
up being called in the background. The header comment for
chromeos::version_loader::GetVersion() and GetARCVersion() says it should be
called on a background thread, but the only blocking operation I've found inside
is reading the /etc/lsb-release file in ChromeOSVersionInfo::Parse(), which has
an explicit ScopedAllowIO, which would allow it to run on a non-background
thread. There also isn't anything in there that would check correct usage (and
with the ScopedAllowIO, even reading the file from the UI thread wouldn't
trigger). Should that ScopedAllowIO be removed, and maybe an assertion added
that we are in an execution context that allows blocking?

[stevenjb, 2017-02-06 17:24:48]

On 2017/02/06 14:40:02, Bernhard Bauer wrote:
> +stevenjb
> 
> This change seems fine to me, but I'm a bit confused about the method that
ends
> up being called in the background. The header comment for
> chromeos::version_loader::GetVersion() and GetARCVersion() says it should be
> called on a background thread, but the only blocking operation I've found
inside
> is reading the /etc/lsb-release file in ChromeOSVersionInfo::Parse(), which
has
> an explicit ScopedAllowIO, which would allow it to run on a non-background
> thread. There also isn't anything in there that would check correct usage (and
> with the ScopedAllowIO, even reading the file from the UI thread wouldn't
> trigger). Should that ScopedAllowIO be removed, and maybe an assertion added
> that we are in an execution context that allows blocking?

Yes, the ScopedAllowIO should be removed. I suspect this was re-factored at some
point to remove the blocking call but the ScopedAllowIO was not removed.

[stevenjb, 2017-02-06 17:26:42]

On 2017/02/06 17:24:48, stevenjb wrote:
> On 2017/02/06 14:40:02, Bernhard Bauer wrote:
> > +stevenjb
> > 
> > This change seems fine to me, but I'm a bit confused about the method that
> ends
> > up being called in the background. The header comment for
> > chromeos::version_loader::GetVersion() and GetARCVersion() says it should be
> > called on a background thread, but the only blocking operation I've found
> inside
> > is reading the /etc/lsb-release file in ChromeOSVersionInfo::Parse(), which
> has
> > an explicit ScopedAllowIO, which would allow it to run on a non-background
> > thread. There also isn't anything in there that would check correct usage
(and
> > with the ScopedAllowIO, even reading the file from the UI thread wouldn't
> > trigger). Should that ScopedAllowIO be removed, and maybe an assertion added
> > that we are in an execution context that allows blocking?
> 
> Yes, the ScopedAllowIO should be removed. I suspect this was re-factored at
some
> point to remove the blocking call but the ScopedAllowIO was not removed.

Actually, looking more closely, we call those from the UI thread in
about_handler.cc, so we can't remove the blocking calls until we deprecate the
old options/settings UI.

[stevenjb, 2017-02-06 17:28:37]

On 2017/02/06 17:26:42, stevenjb wrote:
> On 2017/02/06 17:24:48, stevenjb wrote:
> > On 2017/02/06 14:40:02, Bernhard Bauer wrote:
> > > +stevenjb
> > > 
> > > This change seems fine to me, but I'm a bit confused about the method that
> > ends
> > > up being called in the background. The header comment for
> > > chromeos::version_loader::GetVersion() and GetARCVersion() says it should
be
> > > called on a background thread, but the only blocking operation I've found
> > inside
> > > is reading the /etc/lsb-release file in ChromeOSVersionInfo::Parse(),
which
> > has
> > > an explicit ScopedAllowIO, which would allow it to run on a non-background
> > > thread. There also isn't anything in there that would check correct usage
> (and
> > > with the ScopedAllowIO, even reading the file from the UI thread wouldn't
> > > trigger). Should that ScopedAllowIO be removed, and maybe an assertion
added
> > > that we are in an execution context that allows blocking?
> > 
> > Yes, the ScopedAllowIO should be removed. I suspect this was re-factored at
> some
> > point to remove the blocking call but the ScopedAllowIO was not removed.
> 
> Actually, looking more closely, we call those from the UI thread in
> about_handler.cc, so we can't remove the blocking calls until we deprecate the
> old options/setting

Actually, ignore that last comment, we do call it on the blocking pool in
about_handler.cc, so we should be able to just remove ScopedAllowIO. Obviously
look at the code more closely than I have first... :)

[fdoray, 2017-02-06 18:09:33]

> Actually, ignore that last comment, we do call it on the blocking pool in
> about_handler.cc, so we should be able to just remove ScopedAllowIO. Obviously
> look at the code more closely than I have first... :)

This CL is part of a large refactoring of all post task call sites in Chrome.
Unfortunately, it would be unreasonable for me to start addressing OWNERS
comments beyond adjusting TaskTraits and comments around the refactored call
sites. If this task does file I/O, it is appropriate to post it to TaskScheduler
with .MayBlock(). If ChromeOSVersionInfo::Parse() is never called from a thread
that disallows IO, feel free to remove the ScopedAllowIO from it in a separate
CL.

If you have any questions/comments/concerns about this mass refactoring, feel
free to reach out to me (fdoray), gab or robliao.

Thanks!

[stevenjb, 2017-02-06 18:11:57]

lgtm

[fdoray, 2017-02-06 18:13:25]

The CQ bit was checked by fdoray@chromium.org

[commit-bot: I haz the power, 2017-02-06 18:13:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-06 18:23:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-06 18:23:09]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[fdoray, 2017-02-06 19:11:51]

bauerb@: PTAL

[Bernhard Bauer, 2017-02-07 10:42:47]

lgtm

[fdoray, 2017-02-07 12:57:43]

The CQ bit was checked by fdoray@chromium.org

[commit-bot: I haz the power, 2017-02-07 12:58:01]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-07 13:02:26]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1486472263844730,
"parent_rev": "c09047d16baaa1ddb2f6e1493cfcd5111e49caeb", "commit_rev":
"a8e8f1139422ceeef5ff8e0901a87eeafeb81f65"}

[commit-bot: I haz the power, 2017-02-07 13:02:59]

Description was changed from

==========
Use TaskScheduler instead of blocking pool in version_handler_chromeos.cc.

The blocking pool is being deprecated in favor of TaskScheduler.

BUG=667892
R=bauerb@chromium.org
==========

to

==========
Use TaskScheduler instead of blocking pool in version_handler_chromeos.cc.

The blocking pool is being deprecated in favor of TaskScheduler.

BUG=667892
R=bauerb@chromium.org

Review-Url: https://codereview.chromium.org/2672023003
Cr-Commit-Position: refs/heads/master@{#448608}
Committed:
https://chromium.googlesource.com/chromium/src/+/a8e8f1139422ceeef5ff8e0901a8...
==========

[commit-bot: I haz the power, 2017-02-07 13:03:00]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/a8e8f1139422ceeef5ff8e0901a8...