|
|
DescriptionLoosen 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}
Committed: https://chromium.googlesource.com/chromium/src/+/980a5271bd212db2ad7a050c3d49628aed42deb1
Patch Set 1 #Patch Set 2 : rebase dependency #Patch Set 3 : update dependency #Patch Set 4 : proper dependency #
Total comments: 2
Depends on Patchset: Messages
Total messages: 37 (31 generated)
Patchset #1 (id:1) has been deleted
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Patchset #1 (id:20001) has been deleted
Description was changed from ========== Update thread-safety checks and documentation on RunLoop. RunLoop::Delegate is now the thread-affine part. RunLoop itself is now merely thread-unsafe (->sequence checks). BUG=722537 ========== to ========== Update thread-safety checks and documentation on RunLoop. RunLoop::Delegate now being the thread-affine part. RunLoop itself is now merely thread-unsafe (->sequence checks). BUG=722537 ==========
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
gab@chromium.org changed reviewers: + danakj@chromium.org
Dana PTaL, thanks!
Description was changed from ========== Update thread-safety checks and documentation on RunLoop. RunLoop::Delegate now being the thread-affine part. RunLoop itself is now merely thread-unsafe (->sequence checks). BUG=722537 ========== to ========== 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). BUG=722537 ==========
Description was changed from ========== 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). BUG=722537 ========== to ========== 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 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/05/17 03:08:47, gab wrote: > Dana PTaL, thanks! friendly ping :)
LGTM https://codereview.chromium.org/2886913003/diff/100001/base/run_loop.cc File base/run_loop.cc (right): https://codereview.chromium.org/2886913003/diff/100001/base/run_loop.cc#newco... base/run_loop.cc:96: // Rebind this RunLoop to the current thread after Run(). current sequence?
https://codereview.chromium.org/2886913003/diff/100001/base/run_loop.cc File base/run_loop.cc (right): https://codereview.chromium.org/2886913003/diff/100001/base/run_loop.cc#newco... base/run_loop.cc:96: // Rebind this RunLoop to the current thread after Run(). On 2017/05/18 15:25:27, danakj wrote: > current sequence? I debated that but in practice RunLoop always runs on a thread so we can still sequence check to avoid over-specifying but the bound sequence is a thread -- using a SequenceChecker instead of ThreadChecker is still nice as it allows another sequence (not necessarily thread) to do checks on the RunLoop while it's running (detached from thread above)
The CQ bit was checked by gab@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 100001, "attempt_start_ts": 1495124047254010, "parent_rev": "0f24feacbffe77af7d69f64e739c73d834fa19b4", "commit_rev": "980a5271bd212db2ad7a050c3d49628aed42deb1"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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} Committed: https://chromium.googlesource.com/chromium/src/+/980a5271bd212db2ad7a050c3d49... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:100001) as https://chromium.googlesource.com/chromium/src/+/980a5271bd212db2ad7a050c3d49... |