Support off-thread RunLoop::Quit |
|||
Issue descriptionRunLoop::Quit currently thread-checks it's called on the same thread as RunLoop::Run. While it's true that these methods are thread-unsafe, quitting from another thread is technically sequenced as RunLoop's thread is blocked (as far as RunLoop is concerned) in RunLoop::Run (this has the same effect as a WaitableEvent chain making things "sequenced" across sequences). Tests often post a QuitClosure to another thread and execute it there. This works today because TestBrowserThreadBundle happens to run every BrowserThread on a single MessageLoop by default... As we migrate things off the BrowserThreads into TaskScheduler threads/sequences this is no longer true. base::ScopedTaskEnvironment provides everything that's required for tests that truly need to flush both the TaskScheduler and the main loop but in the use cases where the QuitClosure is merely fired as a signal that the test may continue, allowing RunLoop::Quit from off-thread during RunLoop::Run achieves the desired outcome. The alternative would be to force every test to bind ThreadTaskRunnerHandle::Get() for the QuitClosure to be posted back on main loop when done but that's just ugly and requires way too many test tweaks as part of the migration to TaskScheduler...
,
May 18 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/980a5271bd212db2ad7a050c3d49628aed42deb1 commit 980a5271bd212db2ad7a050c3d49628aed42deb1 Author: gab <gab@chromium.org> Date: Thu May 18 16:20:16 2017 Loosen thread-safety checks and update documentation on RunLoop. RunLoop::Delegate now being the thread-affine part. RunLoop itself is now merely thread-unsafe (->sequence checks). And make it safe to access a RunLoop's state from another sequence while it's being Run() as that access is technically "sequenced" (any access will rebind the SequenceChecker to that sequence for the remainder of the run and should still catch undesired concurrent accesses). The lack of detach during Run() might also be the cause of issue 715235 (will try to remove TODOs after this lands). BUG= 722537 , 715235 Review-Url: https://codereview.chromium.org/2886913003 Cr-Commit-Position: refs/heads/master@{#472835} [modify] https://crrev.com/980a5271bd212db2ad7a050c3d49628aed42deb1/base/run_loop.cc [modify] https://crrev.com/980a5271bd212db2ad7a050c3d49628aed42deb1/base/run_loop.h
,
May 19 2017
,
May 19 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/cf5e4ce0c0eff930821b2459ee03d161ab4d6550 commit cf5e4ce0c0eff930821b2459ee03d161ab4d6550 Author: gab <gab@chromium.org> Date: Fri May 19 22:56:57 2017 Make RunLoop::Quit() thread-safe. It was already safe to call it off-sequence as of r472835. But making it thread-safe is key to enabling use cases like: RunLoop run_loop_; <post a task binding run_loop_.QuitClosure() to task scheduler> run_loop_.Run(); e.g. happens in https://codereview.chromium.org/2881143002/ without this CL Run() and Quit() are racing in this situation as the posted task may run at any point after being posted. FYI, RunLoop() already supports being Quit() prior to being Run() so the race resolving either way is fine, it just needs to be thread-safe. BUG= 722537 Review-Url: https://codereview.chromium.org/2892993002 Cr-Commit-Position: refs/heads/master@{#473378} [modify] https://crrev.com/cf5e4ce0c0eff930821b2459ee03d161ab4d6550/base/run_loop.cc [modify] https://crrev.com/cf5e4ce0c0eff930821b2459ee03d161ab4d6550/base/run_loop.h [modify] https://crrev.com/cf5e4ce0c0eff930821b2459ee03d161ab4d6550/base/run_loop_unittest.cc
,
May 20 2017
|
|||
►
Sign in to add a comment |
|||
Comment 1 by gab@chromium.org
, May 15 2017