Waiting for multiple thread to complete all tasks.

Денис

unread,

Nov 18, 2016, 9:58:30 AM11/18/16

to Chromium-dev

Hi!

I have a few threads that I want to wait for. I can't just do RunUntilIdle on each one of them - they post tasks on each other. How to wait for all of them to complete?

So far I've found a few solutions, none of which I like.

1) Trying to predict how much bouncing of messages is going to happen based on code (sometimes seems like incorrectly).

For example - history: ui - db communication. As far as I know it goes like this:

ui posts tasks on db: (it's called by the test)
db posts tasks to ui: https://cs.chromium.org/chromium/src/components/history/core/browser/history_service.cc?sq=package:chromium&rcl=1479284732&l=139
which internally can trigger some updates on db: https://cs.chromium.org/chromium/src/components/omnibox/browser/url_index_private_data.cc?sq=package:chromium&rcl=1479284732&l=373

And here are some pieces of code attempting to do that in one form or another:

We can see that they differ and I don't see a single one that would take into account the 3rd point (I'm not 100% sure that it's necessary everywhere but at least some time ago it was). My point is - it's hard and requires difficult maintenance.

2) People just spin many times. Doesn't seem like the best practise available.

For example: https://cs.chromium.org/chromium/src/content/public/test/test_utils.cc?sq=package:chromium&rcl=1479284732&l=48

3) One can pull it off with through some message loop api

TaskObservers
https://cs.chromium.org/chromium/src/content/public/test/test_utils.cc?sq=package:chromium&rcl=1479284732&l=83 (it can be scaled for multiple threads)

MessageLoop::IsIdleForTesting()
https://cs.chromium.org/chromium/src/base/message_loop/message_loop.h?q=IsIdleFo&sq=package:chromium&l=310 (you can do a while loop on these)

Unfortunately since recently not all threads support observers: https://codereview.chromium.org/2419803002
And getting complicated too, because appropriate api was removed: https://codereview.chromium.org/2473823002 The only way I know how to get a message loop for a thread is to schedule a task on it. Quite a hack.

Primiano Tucci

unread,

Nov 18, 2016, 6:32:12 PM11/18/16

to dyar...@yandex-team.ru, Chromium-dev

What do you mean specifically when you say "I have a few threads that I want to wait for"?

Do you have an "event" you want to wait for? Typically a test you want to wait until something happens, regardless of how many thread interactions it involves.

The pattern that I frequently see for this is:

1) Create a nested RunLoop in the test.

2) Take the run_loop.QuitClosure() and once you have reached the event you want to wait for, PostTask the quit closure to the task runner where the test runs.

3) run_loop.Run()

run_loop.Run() will "block" the progress of the test (without blocking the message loop, by means of nesting the ML) until the quit closure has been posted.

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

Денис

unread,

Nov 19, 2016, 3:30:51 AM11/19/16

to Chromium-dev, dyar...@yandex-team.ru

Even if we don't take into account how much work it actually is to write an individual run for each case, sometimes it's not possible at all. In some scenarios I want to test that certain scenarios don't trigger a response. If I want to check that nothing happens there is nothing I can wait for.

By waiting for a few threads I mean that I want to run threads untill all of them are empty.

суббота, 19 ноября 2016 г., 2:32:12 UTC+3 пользователь Primiano Tucci написал:

Mark Pearson

unread,

Nov 19, 2016, 6:58:42 PM11/19/16

to Denis Yaroshevskiy, Chromium-dev

On Sat, Nov 19, 2016 at 12:30 AM, Денис <dyar...@yandex-team.ru> wrote:

In some scenarios I want to test that certain scenarios don't trigger a response. If I want to check that nothing happens there is nothing I can wait for.

If lack of action is meaningful, then perhaps you should trigger a response / callback or send a message / observation in those cases (e.g., "RanFoobarAndDecidedNotToDoAnything").

--mark

Trent Apted

unread,

Nov 20, 2016, 9:07:57 PM11/20/16

to mpea...@chromium.org, Denis Yaroshevskiy, Chromium-dev

You could just have all the tasks reference a base::RefCountedThreadSafe<.., content::BrowserThread::DeleteOnUIThread>. If a task posts another task, the new task references it too. When the reference count reaches zero, all the tasks have run and your destructor will get invoked on the UI thread.

See the "Latch" class in web_app_mac.mm - https://cs.chromium.org/chromium/src/chrome/browser/web_applications/web_app_mac.mm?type=cs&q=Latch&sq=package:chromium&l=71 . It uses a base::Closure to extend the lifetime (so the APIs you're calling don't need to know about `Latch` - just base::Closure). This was useful for UpdateShortcutsForAllApps() since it calls an API that already took a "..AndReply(..)" closure argument, but it has to call that API multiple times, and only wants to know when _all_ the tasks are done.

Message has been deleted

Денис

unread,

Nov 21, 2016, 1:32:29 PM11/21/16

to Chromium-dev

Ok - it seems like I found a solution I needed: TestBrowserThreadBundle can create most of the threads I need on the UI thread's message loop. With this I can run all of them by running my message loop to flush them. With other threads I can verify by inspection that I RunUntilIdle correctly.

Btw, I was a bit wrong about history. I thought that history_backend lived on a DB thread, when actually there is a separate history thread that is not mentioned in BrowserThread. So in the end of flashing history one ought to flash DB thread and then history. We probably should fix this whole history situation.

пятница, 18 ноября 2016 г., 17:58:30 UTC+3 пользователь Денис написал:

Gabriel Charette

unread,

Nov 21, 2016, 1:34:43 PM11/21/16

to tap...@chromium.org, mpea...@chromium.org, Denis Yaroshevskiy, Chromium-dev

In a unit test, when using TestBrowserThreadBundle, all "threads" use the same backing MessageLoop hence RunLoop.RunUntilIdle() truly runs all "browser threads" until idle.

Otherwise if what you're doing is tickling something in an integration (browser) test and wanting to observe the full repercussions that had (and didn't have) on the system after everything it triggered unwound, the best way is Primiano's approach (have your API take a Closure which it invokes when done and in tests pass in the RunLoop's QuitClosure()).

You can't really spin all loops until idle (anything I can think of is racy -- even using TaskObservers, e.g. Thread A signals that it's idle, Thread B posts to A and then signals it's also idle: you can get the signal that both are idle before A starts running the task posted from B).

Perhaps MessageLoop::SetTaskRunner() could allow you to mock some of the browser threads in a browser test but I doubt you need to go that far.

Gabriel Charette

unread,

Nov 21, 2016, 1:37:19 PM11/21/16

to dyar...@yandex-team.ru, Chromium-dev, Francois Pierre Doray

Re. history thread, it's going away in favor of a sequence that will run on TaskScheduler very soon. Tests will be able to run the scheduler until idle via TaskScheduler::FlushForTesting (which ultimately even solves your original problem when, one day, everything runs inside TaskScheduler).

On Mon, Nov 21, 2016 at 1:31 PM Денис <dyar...@yandex-team.ru> wrote:

Ok - it seems like I found a solution I needed: TestBrowserThreadBundle can create most of the threads I need on the UI thread's message loop. With this I can run all of them by running my message loop to synchronise them. With other threads I can verify by inspection that I RunUntilIdle correctly.

Btw, I was a bit wrong about history. I thought that history_backend lived on a DB thread, when actually there is a separate history thread that is not mentioned in BrowserThread. So in the end of flashing history one ought to flash DB thread and then history. We probably should fix this whole history situation.

пятница, 18 ноября 2016 г., 17:58:30 UTC+3 пользователь Денис написал:

Hi!

Reply all

Reply to author

Forward