New issue
Advanced search Search tips

Issue 804930 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Feb 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Feature

Blocking:
issue 749310



Sign in to add a comment

Create An Async Version of base::TaskScheduler::FlushForTesting()

Project Member Reported by robliao@chromium.org, Jan 23 2018

Issue description

As the task scheduler brings in work from other MessageLoops, the likelihood of deadlocking on the current thread by calling base::TaskScheduler::FlushForTesting() increases dramatically. Most of the unit test and integration test hangs originate from base::TaskScheduler::FlushForTesting().

This is primarily because previously, the FlushForTesting() only waited for Task Scheduler tasks to complete. With Message Loop redirection, all tasks that go through a MessageLoop are added into the mix.

The deadlock proceeds as follows
Main thread -> FlushForTesting() (Waits for all undelayed tasks)
Task running on external thread -> Posts task to run on main thread and synchronously waits on completion (Sync does this for example).

FlushForTesting() assumes that all threads that need to process work are processing work. MessageLoop redirection with a synchronous FlushForTesting violates that assumption as one thread that should be processing work is waiting for FlushForTesting.

The proposal here allows callers to call FlushForTestingAsync([callback]). This allows callers who need to process tasks perform the following pseudocode:

RunLoop run_loop;
base::TaskScheduler::FlushForTestingAsync([run_loop.QuitWhenIdle()])
run_loop.Run();

As a result, the current thread will be able to make progress and help prevent deadlocks.



 
Project Member

Comment 1 by bugdroid1@chromium.org, Jan 24 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/089b37fd14d1de4ce71e10842ed1bdb689392b55

commit 089b37fd14d1de4ce71e10842ed1bdb689392b55
Author: Robert Liao <robliao@chromium.org>
Date: Wed Jan 24 22:29:30 2018

Introduce TaskTracker::FlushAsyncForTesting()

This performs a task flush similar to TaskTracker::Flush() except it is
async. It calls the callback when TaskTracker::Flush() would have
returned.

BUG= 804930 

Change-Id: I428fd822104a3c5c0663a05852d7ecc47680c2ae
Reviewed-on: https://chromium-review.googlesource.com/882384
Commit-Queue: Robert Liao <robliao@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#531719}
[modify] https://crrev.com/089b37fd14d1de4ce71e10842ed1bdb689392b55/base/task_scheduler/task_tracker.cc
[modify] https://crrev.com/089b37fd14d1de4ce71e10842ed1bdb689392b55/base/task_scheduler/task_tracker.h
[modify] https://crrev.com/089b37fd14d1de4ce71e10842ed1bdb689392b55/base/task_scheduler/task_tracker_unittest.cc

Project Member

Comment 2 by bugdroid1@chromium.org, Jan 28 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/849757f4888ca5a90734417bdd01b1b0e3898d2a

commit 849757f4888ca5a90734417bdd01b1b0e3898d2a
Author: Robert Liao <robliao@chromium.org>
Date: Sun Jan 28 01:03:49 2018

Add TaskScheduler::FlushAsyncForTesting()

This performs a task flush similar to TaskScheduler::FlushForTesting()
except it is async. It calls the callback if TaskScheduler::Flush()
would have returned.

BUG= 804930 

Change-Id: I533ab5a2fbaccd79a220aad3a535fa37ed7df823
Reviewed-on: https://chromium-review.googlesource.com/883607
Commit-Queue: Robert Liao <robliao@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#532258}
[modify] https://crrev.com/849757f4888ca5a90734417bdd01b1b0e3898d2a/base/task_scheduler/task_scheduler.h
[modify] https://crrev.com/849757f4888ca5a90734417bdd01b1b0e3898d2a/base/task_scheduler/task_scheduler_impl.cc
[modify] https://crrev.com/849757f4888ca5a90734417bdd01b1b0e3898d2a/base/task_scheduler/task_scheduler_impl.h
[modify] https://crrev.com/849757f4888ca5a90734417bdd01b1b0e3898d2a/base/task_scheduler/task_scheduler_impl_unittest.cc

Project Member

Comment 3 by bugdroid1@chromium.org, Feb 9 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a567dbaab89add69b0434bc7e0d0b468064da7fe

commit a567dbaab89add69b0434bc7e0d0b468064da7fe
Author: Robert Liao <robliao@chromium.org>
Date: Fri Feb 09 17:59:09 2018

Modify content::RunAllTasksUntilIdle() To Use FlushAsyncForTesting()

This allows RunAllTasksUntilIdle() to pump messages and flush at the
same time instead of as separate steps.

BUG= 804930 

Change-Id: Ie07f320b558abc975f5922e89de33a66f00a48b9
Reviewed-on: https://chromium-review.googlesource.com/884503
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Reviewed-by: François Doray <fdoray@chromium.org>
Commit-Queue: Robert Liao <robliao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#535749}
[modify] https://crrev.com/a567dbaab89add69b0434bc7e0d0b468064da7fe/content/public/test/test_utils.cc
[modify] https://crrev.com/a567dbaab89add69b0434bc7e0d0b468064da7fe/content/public/test/test_utils.h

Status: Fixed (was: Assigned)

Sign in to add a comment