Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2145463002:
Modernize base::Thread (Closed)
DescriptionModernize base::Thread
No-op cleanup extracted from https://codereview.chromium.org/2135413003/#ps40001
In this CL:
- Explicitly document the threading requirements of the API after grokking its use of locks
in some places and lack of in some others)
- (It's mostly thread unsafe; except for a few calls which are okay from the owner
as well as the underlying thread; and one method which is actually thread-safe)
- Add assertions as such (and catch issues on try bots :-O -- http://crbug.com/629139)
- Remove |thread_lock_| which is unnecessary under the documented conditions
(delayed until http://crbug.com/629139 is fixed..).
- C++11 member initializers (makes it cleaner to add new POD members and
makes constructors simpler -- e.g. |run_loop_| was uninitialized)
- Assertions and tests for current API which allows Start/Stop/Start cycles
but wasn't tested... (actually more than that is poorly/not tested in this class
but I'm merely adding tests for parts I'm stressing in my actual follow-up CL).
- Comment nits
- Make event-based tests use WaitableEvent instead of sleeping + toggling a bool.
BUG=629716, 629139
Committed: https://crrev.com/1734a80f1519819eb6a3d8eb43064f8fb2c2e067
Cr-Commit-Position: refs/heads/master@{#407265}
Patch Set 1 #Patch Set 2 : Allow message_loop()/task_runner() getters from underlying thread too #Patch Set 3 : Temporarily disable thread-safe check in Stop() per crbug/629139 #Patch Set 4 : Allow message_loop()/task_runner() access from externally synchronized threads. #Patch Set 5 : use |message_loop_| instead of |running_| to assert Start()'s synchronization #Patch Set 6 : only attach to sequence on Start #Patch Set 7 : merge up to r406471 #Patch Set 8 : Disable check in StopSoon() as in Stop() per crbug/629139 #Patch Set 9 : Disable check in IsRunning() per crbug/629139 and add extra logging. #
Total comments: 19
Patch Set 10 : Disable check in message_loop() per crbug.com/629139#c6 :-( #Patch Set 11 : review:danakj #
Total comments: 2
Patch Set 12 : comment nit #
Total comments: 4
Messages
Total messages: 85 (67 generated)
Description was changed from ========== Modernize base::Thread BUG=622400 (cleanup extracted from https://codereview.chromium.org/2135413003/#ps40001) ========== to ========== Modernize base::Thread No-op cleanup extracted from https://codereview.chromium.org/2135413003/#ps40001 In this CL: - Explicitly document the API as thread-unsafe - Add assertions as such - Remove |thread_lock_| which is unnecessary under such conditions. - C++11 member initializers (makes it cleaner to add new POD members and makes constructors simpler) - Assertions and tests for current API which allows Start/Stop/Start cycles but wasn't tested... - Comment nits - Make event-based tests use WaitableEvent instead of sleeping + toggling a bool. BUG=622400 ==========
Description was changed from ========== Modernize base::Thread No-op cleanup extracted from https://codereview.chromium.org/2135413003/#ps40001 In this CL: - Explicitly document the API as thread-unsafe - Add assertions as such - Remove |thread_lock_| which is unnecessary under such conditions. - C++11 member initializers (makes it cleaner to add new POD members and makes constructors simpler) - Assertions and tests for current API which allows Start/Stop/Start cycles but wasn't tested... - Comment nits - Make event-based tests use WaitableEvent instead of sleeping + toggling a bool. BUG=622400 ========== to ========== Modernize base::Thread No-op cleanup extracted from https://codereview.chromium.org/2135413003/#ps40001 In this CL: - Explicitly document the API as thread-unsafe - Add assertions as such - Remove |thread_lock_| which is unnecessary under such conditions. - C++11 member initializers (makes it cleaner to add new POD members and makes constructors simpler) - Assertions and tests for current API which allows Start/Stop/Start cycles but wasn't tested... - Comment nits - Make event-based tests use WaitableEvent instead of sleeping + toggling a bool. BUG=622400 ==========
Description was changed from ========== Modernize base::Thread No-op cleanup extracted from https://codereview.chromium.org/2135413003/#ps40001 In this CL: - Explicitly document the API as thread-unsafe - Add assertions as such - Remove |thread_lock_| which is unnecessary under such conditions. - C++11 member initializers (makes it cleaner to add new POD members and makes constructors simpler) - Assertions and tests for current API which allows Start/Stop/Start cycles but wasn't tested... - Comment nits - Make event-based tests use WaitableEvent instead of sleeping + toggling a bool. BUG=622400 ========== to ========== Modernize base::Thread No-op cleanup extracted from https://codereview.chromium.org/2135413003/#ps40001 In this CL: - Explicitly document the API as thread-unsafe - Add assertions as such - Remove |thread_lock_| which is unnecessary under such conditions. - C++11 member initializers (makes it cleaner to add new POD members and makes constructors simpler) - Assertions and tests for current API which allows Start/Stop/Start cycles but wasn't tested... (actually more than that isn't tested but I'm merely adding tests for parts I'm stressing in my actual follow-up CL). - Comment nits - Make event-based tests use WaitableEvent instead of sleeping + toggling a bool. BUG=622400 ==========
Description was changed from ========== Modernize base::Thread No-op cleanup extracted from https://codereview.chromium.org/2135413003/#ps40001 In this CL: - Explicitly document the API as thread-unsafe - Add assertions as such - Remove |thread_lock_| which is unnecessary under such conditions. - C++11 member initializers (makes it cleaner to add new POD members and makes constructors simpler) - Assertions and tests for current API which allows Start/Stop/Start cycles but wasn't tested... (actually more than that isn't tested but I'm merely adding tests for parts I'm stressing in my actual follow-up CL). - Comment nits - Make event-based tests use WaitableEvent instead of sleeping + toggling a bool. BUG=622400 ========== to ========== Modernize base::Thread No-op cleanup extracted from https://codereview.chromium.org/2135413003/#ps40001 In this CL: - Explicitly document the API as thread-unsafe - Add assertions as such - Remove |thread_lock_| which is unnecessary under such conditions. - C++11 member initializers (makes it cleaner to add new POD members and makes constructors simpler) - Assertions and tests for current API which allows Start/Stop/Start cycles but wasn't tested... (actually more than that is poorly/not tested in this class but I'm merely adding tests for parts I'm stressing in my actual follow-up CL). - Comment nits - Make event-based tests use WaitableEvent instead of sleeping + toggling a bool. BUG=622400 ==========
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...
Description was changed from ========== Modernize base::Thread No-op cleanup extracted from https://codereview.chromium.org/2135413003/#ps40001 In this CL: - Explicitly document the API as thread-unsafe - Add assertions as such - Remove |thread_lock_| which is unnecessary under such conditions. - C++11 member initializers (makes it cleaner to add new POD members and makes constructors simpler) - Assertions and tests for current API which allows Start/Stop/Start cycles but wasn't tested... (actually more than that is poorly/not tested in this class but I'm merely adding tests for parts I'm stressing in my actual follow-up CL). - Comment nits - Make event-based tests use WaitableEvent instead of sleeping + toggling a bool. BUG=622400 ========== to ========== Modernize base::Thread No-op cleanup extracted from https://codereview.chromium.org/2135413003/#ps40001 In this CL: - Explicitly document the threading requirements of the API after grokking its use of locks in some places and lack of in some others) - (It's mostly thread unsafe; except for a few calls which are okay from the owner as well as the underlying thread; and one method which is actually thread-safe) - Add assertions as such - Remove |thread_lock_| which is unnecessary under the documented conditions. - C++11 member initializers (makes it cleaner to add new POD members and makes constructors simpler) - Assertions and tests for current API which allows Start/Stop/Start cycles but wasn't tested... (actually more than that is poorly/not tested in this class but I'm merely adding tests for parts I'm stressing in my actual follow-up CL). - Comment nits - Make event-based tests use WaitableEvent instead of sleeping + toggling a bool. BUG=622400 ==========
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: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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 to run a CQ dry run
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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 gab@chromium.org
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
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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Patchset #3 (id:40001) 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
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: ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
Description was changed from ========== Modernize base::Thread No-op cleanup extracted from https://codereview.chromium.org/2135413003/#ps40001 In this CL: - Explicitly document the threading requirements of the API after grokking its use of locks in some places and lack of in some others) - (It's mostly thread unsafe; except for a few calls which are okay from the owner as well as the underlying thread; and one method which is actually thread-safe) - Add assertions as such - Remove |thread_lock_| which is unnecessary under the documented conditions. - C++11 member initializers (makes it cleaner to add new POD members and makes constructors simpler) - Assertions and tests for current API which allows Start/Stop/Start cycles but wasn't tested... (actually more than that is poorly/not tested in this class but I'm merely adding tests for parts I'm stressing in my actual follow-up CL). - Comment nits - Make event-based tests use WaitableEvent instead of sleeping + toggling a bool. BUG=622400 ========== to ========== Modernize base::Thread No-op cleanup extracted from https://codereview.chromium.org/2135413003/#ps40001 In this CL: - Explicitly document the threading requirements of the API after grokking its use of locks in some places and lack of in some others) - (It's mostly thread unsafe; except for a few calls which are okay from the owner as well as the underlying thread; and one method which is actually thread-safe) - Add assertions as such - Remove |thread_lock_| which is unnecessary under the documented conditions. - C++11 member initializers (makes it cleaner to add new POD members and makes constructors simpler) - Assertions and tests for current API which allows Start/Stop/Start cycles but wasn't tested... (actually more than that is poorly/not tested in this class but I'm merely adding tests for parts I'm stressing in my actual follow-up CL). - Comment nits - Make event-based tests use WaitableEvent instead of sleeping + toggling a bool. BUG=629716 ==========
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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
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
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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
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...
Description was changed from ========== Modernize base::Thread No-op cleanup extracted from https://codereview.chromium.org/2135413003/#ps40001 In this CL: - Explicitly document the threading requirements of the API after grokking its use of locks in some places and lack of in some others) - (It's mostly thread unsafe; except for a few calls which are okay from the owner as well as the underlying thread; and one method which is actually thread-safe) - Add assertions as such - Remove |thread_lock_| which is unnecessary under the documented conditions. - C++11 member initializers (makes it cleaner to add new POD members and makes constructors simpler) - Assertions and tests for current API which allows Start/Stop/Start cycles but wasn't tested... (actually more than that is poorly/not tested in this class but I'm merely adding tests for parts I'm stressing in my actual follow-up CL). - Comment nits - Make event-based tests use WaitableEvent instead of sleeping + toggling a bool. BUG=629716 ========== to ========== Modernize base::Thread No-op cleanup extracted from https://codereview.chromium.org/2135413003/#ps40001 In this CL: - Explicitly document the threading requirements of the API after grokking its use of locks in some places and lack of in some others) - (It's mostly thread unsafe; except for a few calls which are okay from the owner as well as the underlying thread; and one method which is actually thread-safe) - Add assertions as such (and catch issues on try bots :-O -- http://crbug.com/629139) - Remove |thread_lock_| which is unnecessary under the documented conditions. - C++11 member initializers (makes it cleaner to add new POD members and makes constructors simpler -- e.g. |run_loop_| was uninitialized) - Assertions and tests for current API which allows Start/Stop/Start cycles but wasn't tested... (actually more than that is poorly/not tested in this class but I'm merely adding tests for parts I'm stressing in my actual follow-up CL). - Comment nits - Make event-based tests use WaitableEvent instead of sleeping + toggling a bool. BUG=629716, 629139 ==========
gab@chromium.org changed reviewers: + danakj@chromium.org
Dana PTAL, lots of work went into making this pass (turns out people don't always use APIs as documented :-O!), I think this patch set will work, but even if it doesn't I'd like to have your review to make sure I'm not putting all this work into something you don't want :-) -- it shouldn't change much from here. Cheers, Gab
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...)
This is great https://codereview.chromium.org/2145463002/diff/180001/base/threading/thread.cc File base/threading/thread.cc (left): https://codereview.chromium.org/2145463002/diff/180001/base/threading/thread.... base/threading/thread.cc:113: AutoLock lock(thread_lock_); I think you do need this until http://crbug.com/629139 is fixed, because Stop() uses it for example. https://codereview.chromium.org/2145463002/diff/180001/base/threading/thread.cc File base/threading/thread.cc (right): https://codereview.chromium.org/2145463002/diff/180001/base/threading/thread.... base/threading/thread.cc:51: // Only bind the sequence on Start(): the state is constant between nice comment https://codereview.chromium.org/2145463002/diff/180001/base/threading/thread.... base/threading/thread.cc:93: std::unique_ptr<MessageLoop> message_loop = can you name this so that there is more difference between it and the member variable than an underscore? message_loop_owned maybe? https://codereview.chromium.org/2145463002/diff/180001/base/threading/thread.... base/threading/thread.cc:105: // The ownership of |message_loop| is managed by the newly created thread I think saying "|message_loop_|" (ie the member) here would be even more clear. https://codereview.chromium.org/2145463002/diff/180001/base/threading/thread.h File base/threading/thread.h (right): https://codereview.chromium.org/2145463002/diff/180001/base/threading/thread.... base/threading/thread.h:167: // In addition to this Thread's owner, this can also safely be called from the "Thread's owning sequence" sounds more strictly correct. "owner" sounds more like memory ownership ie via a unique_ptr. https://codereview.chromium.org/2145463002/diff/180001/base/threading/thread.... base/threading/thread.h:191: // In addition to this Thread's owner, this can also safely be called from the ditto "owner" https://codereview.chromium.org/2145463002/diff/180001/base/threading/thread.... base/threading/thread.h:194: // Ref. comment on |message_loop()| DCHECK. "Refer to the DCHECK and comment inside |message_loop()|. https://codereview.chromium.org/2145463002/diff/180001/base/threading/thread.... base/threading/thread.h:280: const std::string name_; Yay const https://codereview.chromium.org/2145463002/diff/180001/base/threading/thread.... base/threading/thread.h:285: // This class is not thread-safe. ...thread-safe, use this to verify access from the owning sequence of the Thread. https://codereview.chromium.org/2145463002/diff/180001/base/threading/thread.... base/threading/thread.h:286: SequenceChecker sequence_checker_; nit: maybe call this owning_sequence_checker_? or something u like better, but i like when checkers say which sequence/thread they are checking for.
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
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Dana, all done, PTAnL, thanks! PS: Had to disable one more test (and still getting random failures in the check in Thread::StartWithOptions() -- trying to figure those out, they don't repro locally...) https://codereview.chromium.org/2145463002/diff/180001/base/threading/thread.cc File base/threading/thread.cc (left): https://codereview.chromium.org/2145463002/diff/180001/base/threading/thread.... base/threading/thread.cc:113: AutoLock lock(thread_lock_); On 2016/07/21 19:43:26, danakj wrote: > I think you do need this until http://crbug.com/629139 is fixed, because Stop() > uses it for example. Hmmm I guess yea :-(, AFAICT there's only one bad test that depends on this (fixed in https://codereview.chromium.org/2170953002/), but no harm keeping the lock for a little while longer while this is fixed (and we confirm that the check passes on all bots after). https://codereview.chromium.org/2145463002/diff/180001/base/threading/thread.cc File base/threading/thread.cc (right): https://codereview.chromium.org/2145463002/diff/180001/base/threading/thread.... base/threading/thread.cc:51: // Only bind the sequence on Start(): the state is constant between On 2016/07/21 19:43:26, danakj wrote: > nice comment :-) https://codereview.chromium.org/2145463002/diff/180001/base/threading/thread.... base/threading/thread.cc:93: std::unique_ptr<MessageLoop> message_loop = On 2016/07/21 19:43:26, danakj wrote: > can you name this so that there is more difference between it and the member > variable than an underscore? message_loop_owned maybe? Done. https://codereview.chromium.org/2145463002/diff/180001/base/threading/thread.... base/threading/thread.cc:105: // The ownership of |message_loop| is managed by the newly created thread On 2016/07/21 19:43:25, danakj wrote: > I think saying "|message_loop_|" (ie the member) here would be even more clear. Done. https://codereview.chromium.org/2145463002/diff/180001/base/threading/thread.h File base/threading/thread.h (right): https://codereview.chromium.org/2145463002/diff/180001/base/threading/thread.... base/threading/thread.h:167: // In addition to this Thread's owner, this can also safely be called from the On 2016/07/21 19:43:26, danakj wrote: > "Thread's owning sequence" sounds more strictly correct. "owner" sounds more > like memory ownership ie via a unique_ptr. Done. https://codereview.chromium.org/2145463002/diff/180001/base/threading/thread.... base/threading/thread.h:191: // In addition to this Thread's owner, this can also safely be called from the On 2016/07/21 19:43:26, danakj wrote: > ditto "owner" Done. https://codereview.chromium.org/2145463002/diff/180001/base/threading/thread.... base/threading/thread.h:194: // Ref. comment on |message_loop()| DCHECK. On 2016/07/21 19:43:26, danakj wrote: > "Refer to the DCHECK and comment inside |message_loop()|. Done. https://codereview.chromium.org/2145463002/diff/180001/base/threading/thread.... base/threading/thread.h:280: const std::string name_; On 2016/07/21 19:43:26, danakj wrote: > Yay const :-) https://codereview.chromium.org/2145463002/diff/180001/base/threading/thread.... base/threading/thread.h:286: SequenceChecker sequence_checker_; On 2016/07/21 19:43:26, danakj wrote: > nit: maybe call this owning_sequence_checker_? or something u like better, but i > like when checkers say which sequence/thread they are checking for. Done.
Description was changed from ========== Modernize base::Thread No-op cleanup extracted from https://codereview.chromium.org/2135413003/#ps40001 In this CL: - Explicitly document the threading requirements of the API after grokking its use of locks in some places and lack of in some others) - (It's mostly thread unsafe; except for a few calls which are okay from the owner as well as the underlying thread; and one method which is actually thread-safe) - Add assertions as such (and catch issues on try bots :-O -- http://crbug.com/629139) - Remove |thread_lock_| which is unnecessary under the documented conditions. - C++11 member initializers (makes it cleaner to add new POD members and makes constructors simpler -- e.g. |run_loop_| was uninitialized) - Assertions and tests for current API which allows Start/Stop/Start cycles but wasn't tested... (actually more than that is poorly/not tested in this class but I'm merely adding tests for parts I'm stressing in my actual follow-up CL). - Comment nits - Make event-based tests use WaitableEvent instead of sleeping + toggling a bool. BUG=629716, 629139 ========== to ========== Modernize base::Thread No-op cleanup extracted from https://codereview.chromium.org/2135413003/#ps40001 In this CL: - Explicitly document the threading requirements of the API after grokking its use of locks in some places and lack of in some others) - (It's mostly thread unsafe; except for a few calls which are okay from the owner as well as the underlying thread; and one method which is actually thread-safe) - Add assertions as such (and catch issues on try bots :-O -- http://crbug.com/629139) - Remove |thread_lock_| which is unnecessary under the documented conditions (delayed until http://crbug.com/629139 is fixed..). - C++11 member initializers (makes it cleaner to add new POD members and makes constructors simpler -- e.g. |run_loop_| was uninitialized) - Assertions and tests for current API which allows Start/Stop/Start cycles but wasn't tested... (actually more than that is poorly/not tested in this class but I'm merely adding tests for parts I'm stressing in my actual follow-up CL). - Comment nits - Make event-based tests use WaitableEvent instead of sleeping + toggling a bool. BUG=629716, 629139 ==========
Description was changed from ========== Modernize base::Thread No-op cleanup extracted from https://codereview.chromium.org/2135413003/#ps40001 In this CL: - Explicitly document the threading requirements of the API after grokking its use of locks in some places and lack of in some others) - (It's mostly thread unsafe; except for a few calls which are okay from the owner as well as the underlying thread; and one method which is actually thread-safe) - Add assertions as such (and catch issues on try bots :-O -- http://crbug.com/629139) - Remove |thread_lock_| which is unnecessary under the documented conditions (delayed until http://crbug.com/629139 is fixed..). - C++11 member initializers (makes it cleaner to add new POD members and makes constructors simpler -- e.g. |run_loop_| was uninitialized) - Assertions and tests for current API which allows Start/Stop/Start cycles but wasn't tested... (actually more than that is poorly/not tested in this class but I'm merely adding tests for parts I'm stressing in my actual follow-up CL). - Comment nits - Make event-based tests use WaitableEvent instead of sleeping + toggling a bool. BUG=629716, 629139 ========== to ========== Modernize base::Thread No-op cleanup extracted from https://codereview.chromium.org/2135413003/#ps40001 In this CL: - Explicitly document the threading requirements of the API after grokking its use of locks in some places and lack of in some others) - (It's mostly thread unsafe; except for a few calls which are okay from the owner as well as the underlying thread; and one method which is actually thread-safe) - Add assertions as such (and catch issues on try bots :-O -- http://crbug.com/629139) - Remove |thread_lock_| which is unnecessary under the documented conditions (delayed until http://crbug.com/629139 is fixed..). - C++11 member initializers (makes it cleaner to add new POD members and makes constructors simpler -- e.g. |run_loop_| was uninitialized) - Assertions and tests for current API which allows Start/Stop/Start cycles but wasn't tested... (actually more than that is poorly/not tested in this class but I'm merely adding tests for parts I'm stressing in my actual follow-up CL). - Comment nits - Make event-based tests use WaitableEvent instead of sleeping + toggling a bool. BUG=629716, 629139 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by gab@chromium.org to run a CQ dry run
On 2016/07/22 18:13:16, gab wrote: > The CQ bit was checked by https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=gab@chromium.org to run a CQ dry run OMG :-)
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/07/22 17:29:31, commit-bot: I haz the power wrote: > Dry run: This issue passed the CQ dry run. I mean ^^^ OMG :-)!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
drive-by grammaring https://codereview.chromium.org/2145463002/diff/220001/base/threading/thread.h File base/threading/thread.h (right): https://codereview.chromium.org/2145463002/diff/220001/base/threading/thread.... base/threading/thread.h:44: // This API is not thread-safe: unless indicated otherwise it's methods are only its
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...
https://codereview.chromium.org/2145463002/diff/220001/base/threading/thread.h File base/threading/thread.h (right): https://codereview.chromium.org/2145463002/diff/220001/base/threading/thread.... base/threading/thread.h:44: // This API is not thread-safe: unless indicated otherwise it's methods are only On 2016/07/22 18:18:58, Avi wrote: > its Thanks, done.
LGTM
The CQ bit was unchecked by gab@chromium.org
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...
On 2016/07/22 16:02:55, gab wrote: > Dana, all done, PTAnL, thanks! > > PS: Had to disable one more test (and still getting random failures in the check > in Thread::StartWithOptions() -- trying to figure those out, they don't repro > locally...) Given that the Thread::StartWithOptions() test seems to be passing since I disabled the Thread::message_loop() check I assume that the issue was that an unrelated thread was incorrectly calling Thread::message_loop() and doing so before Start() and thus grabbing the SequenceChecker for itself... (also explaining why which test was failing was flaky and I couldn't repro locally... will need to debug further as I try to enable each check).
Message was sent while issue was closed.
Description was changed from ========== Modernize base::Thread No-op cleanup extracted from https://codereview.chromium.org/2135413003/#ps40001 In this CL: - Explicitly document the threading requirements of the API after grokking its use of locks in some places and lack of in some others) - (It's mostly thread unsafe; except for a few calls which are okay from the owner as well as the underlying thread; and one method which is actually thread-safe) - Add assertions as such (and catch issues on try bots :-O -- http://crbug.com/629139) - Remove |thread_lock_| which is unnecessary under the documented conditions (delayed until http://crbug.com/629139 is fixed..). - C++11 member initializers (makes it cleaner to add new POD members and makes constructors simpler -- e.g. |run_loop_| was uninitialized) - Assertions and tests for current API which allows Start/Stop/Start cycles but wasn't tested... (actually more than that is poorly/not tested in this class but I'm merely adding tests for parts I'm stressing in my actual follow-up CL). - Comment nits - Make event-based tests use WaitableEvent instead of sleeping + toggling a bool. BUG=629716, 629139 ========== to ========== Modernize base::Thread No-op cleanup extracted from https://codereview.chromium.org/2135413003/#ps40001 In this CL: - Explicitly document the threading requirements of the API after grokking its use of locks in some places and lack of in some others) - (It's mostly thread unsafe; except for a few calls which are okay from the owner as well as the underlying thread; and one method which is actually thread-safe) - Add assertions as such (and catch issues on try bots :-O -- http://crbug.com/629139) - Remove |thread_lock_| which is unnecessary under the documented conditions (delayed until http://crbug.com/629139 is fixed..). - C++11 member initializers (makes it cleaner to add new POD members and makes constructors simpler -- e.g. |run_loop_| was uninitialized) - Assertions and tests for current API which allows Start/Stop/Start cycles but wasn't tested... (actually more than that is poorly/not tested in this class but I'm merely adding tests for parts I'm stressing in my actual follow-up CL). - Comment nits - Make event-based tests use WaitableEvent instead of sleeping + toggling a bool. BUG=629716, 629139 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== Modernize base::Thread No-op cleanup extracted from https://codereview.chromium.org/2135413003/#ps40001 In this CL: - Explicitly document the threading requirements of the API after grokking its use of locks in some places and lack of in some others) - (It's mostly thread unsafe; except for a few calls which are okay from the owner as well as the underlying thread; and one method which is actually thread-safe) - Add assertions as such (and catch issues on try bots :-O -- http://crbug.com/629139) - Remove |thread_lock_| which is unnecessary under the documented conditions (delayed until http://crbug.com/629139 is fixed..). - C++11 member initializers (makes it cleaner to add new POD members and makes constructors simpler -- e.g. |run_loop_| was uninitialized) - Assertions and tests for current API which allows Start/Stop/Start cycles but wasn't tested... (actually more than that is poorly/not tested in this class but I'm merely adding tests for parts I'm stressing in my actual follow-up CL). - Comment nits - Make event-based tests use WaitableEvent instead of sleeping + toggling a bool. BUG=629716, 629139 ========== to ========== Modernize base::Thread No-op cleanup extracted from https://codereview.chromium.org/2135413003/#ps40001 In this CL: - Explicitly document the threading requirements of the API after grokking its use of locks in some places and lack of in some others) - (It's mostly thread unsafe; except for a few calls which are okay from the owner as well as the underlying thread; and one method which is actually thread-safe) - Add assertions as such (and catch issues on try bots :-O -- http://crbug.com/629139) - Remove |thread_lock_| which is unnecessary under the documented conditions (delayed until http://crbug.com/629139 is fixed..). - C++11 member initializers (makes it cleaner to add new POD members and makes constructors simpler -- e.g. |run_loop_| was uninitialized) - Assertions and tests for current API which allows Start/Stop/Start cycles but wasn't tested... (actually more than that is poorly/not tested in this class but I'm merely adding tests for parts I'm stressing in my actual follow-up CL). - Comment nits - Make event-based tests use WaitableEvent instead of sleeping + toggling a bool. BUG=629716, 629139 Committed: https://crrev.com/1734a80f1519819eb6a3d8eb43064f8fb2c2e067 Cr-Commit-Position: refs/heads/master@{#407265} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/1734a80f1519819eb6a3d8eb43064f8fb2c2e067 Cr-Commit-Position: refs/heads/master@{#407265}
Message was sent while issue was closed.
wez@chromium.org changed reviewers: + wez@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2145463002/diff/240001/base/threading/thread.cc File base/threading/thread.cc (right): https://codereview.chromium.org/2145463002/diff/240001/base/threading/thread.... base/threading/thread.cc:140: // |thread_lock_| is required... FWIW, this comment as worded implies that callers were mis-using Start()/Stop() in calling them from different Sequences, where AFAICT the issue is actually that you're specifically changing the API contract in this CL. It would be helpful to have the CL description and/or bug document why imposing the same-Sequence restriction is preferable to the use of locks here. Right now there's a cyclical reference between CL and bug rationale. :P
Message was sent while issue was closed.
https://codereview.chromium.org/2145463002/diff/240001/base/threading/thread.cc File base/threading/thread.cc (right): https://codereview.chromium.org/2145463002/diff/240001/base/threading/thread.... base/threading/thread.cc:203: DCHECK_EQ(id_, PlatformThread::CurrentId()); What is this DCHECK intended to protect against? Comment implies you're concerned about sub-classes calling it on themselves..?
Message was sent while issue was closed.
https://codereview.chromium.org/2145463002/diff/240001/base/threading/thread.cc File base/threading/thread.cc (right): https://codereview.chromium.org/2145463002/diff/240001/base/threading/thread.... base/threading/thread.cc:140: // |thread_lock_| is required... On 2016/07/25 21:20:29, Wez wrote: > FWIW, this comment as worded implies that callers were mis-using Start()/Stop() > in calling them from different Sequences, where AFAICT the issue is actually > that you're specifically changing the API contract in this CL. > > It would be helpful to have the CL description and/or bug document why imposing > the same-Sequence restriction is preferable to the use of locks here. Right now > there's a cyclical reference between CL and bug rationale. :P Generally in Chromium, unless specified otherwise, classes are not to be assumed as thread-safe. It is good practice nowadays to use a Sequence/ThreadChecker to explicitly enforce such requirements when non-obvious (especially in base/ APIs used all over the map). base::Thread being very old was lacking such checks, but it is clear from its design that this was the intent (no locks or synchronization on all state -- except on |thread_handle_| but that's probably the remains of a scheme where it was used more broadly and can go away now). The previous comment on Thread::StopSoon() makes this clear : "We should only be called on the same thread that started us.". Also, on IsRunning() : "Note that stopping_ is touched only on the same thread so we need no locking here." Another example that this was intended to be used as such, TestIOThread which uses the default Thread::Start() and Thread::Stop() documents that : "|Start()|/|Stop()| should only be called from the main (creation) thread." (comment just updated in a tangentially related CL but was otherwise there for ages before : https://codereview.chromium.org/2170953002/). If you want to propose making this API thread-safe, go ahead, but *that* would be a change in API IMO. Whereas this CL merely explicitly states what was the intended API and implicit assumptions of "non-thread-safety unless specified otherwise", it's a no-op production code wise (adding locks wouldn't be). https://codereview.chromium.org/2145463002/diff/240001/base/threading/thread.... base/threading/thread.cc:203: DCHECK_EQ(id_, PlatformThread::CurrentId()); On 2016/07/25 21:22:36, Wez wrote: > What is this DCHECK intended to protect against? Comment implies you're > concerned about sub-classes calling it on themselves..? As is often the case with DCHECKs, it's intended as documentation of the expected contract. Since every other method uses |owning_sequence_checker_|, I wanted to make it clear that it wasn't omitted here by accident but rather because this runs on the underlying thread, not on the owning sequence. The purpose of this CL is essentially that I had to grok this API inside-and-out to write https://codereview.chromium.org/2135413003/ and then decided to drop the knowledge I built doing so in the codebase to help future readers as well as reinforce discovered assumptions with runtime checks (which indeed caught a few things as you saw).
Message was sent while issue was closed.
On 2016/07/26 02:48:23, gab wrote: > https://codereview.chromium.org/2145463002/diff/240001/base/threading/thread.cc > File base/threading/thread.cc (right): > > https://codereview.chromium.org/2145463002/diff/240001/base/threading/thread.... > base/threading/thread.cc:140: // |thread_lock_| is required... > On 2016/07/25 21:20:29, Wez wrote: > > FWIW, this comment as worded implies that callers were mis-using > Start()/Stop() > > in calling them from different Sequences, where AFAICT the issue is actually > > that you're specifically changing the API contract in this CL. > > > > It would be helpful to have the CL description and/or bug document why > imposing > > the same-Sequence restriction is preferable to the use of locks here. Right > now > > there's a cyclical reference between CL and bug rationale. :P > > Generally in Chromium, unless specified otherwise, classes are not to be assumed > as thread-safe. Yup, I'm not arguing with that. Just pointing out that your CL and bug descriptions refer to one another as motivations for changes without providing the motivation context. > The > previous comment on Thread::StopSoon() makes this clear : "We should only be > called on the same thread that started us.". Agreed; there was a change last year that added the unnecessary AutoLock in Stop(), it seems. [snip] > https://codereview.chromium.org/2145463002/diff/240001/base/threading/thread.... > base/threading/thread.cc:203: DCHECK_EQ(id_, PlatformThread::CurrentId()); > On 2016/07/25 21:22:36, Wez wrote: > > What is this DCHECK intended to protect against? Comment implies you're > > concerned about sub-classes calling it on themselves..? > > As is often the case with DCHECKs, it's intended as documentation of the > expected contract. Since every other method uses |owning_sequence_checker_|, I > wanted to make it clear that it wasn't omitted here by accident but rather > because this runs on the underlying thread, not on the owning sequence. That's true of public-facing APIs, but this one is [intended to be] called by the Thread itself. Where there's a contract within an implementation (i.e. no scope for callers breaking things) it's common to omit DCHECKs in the implementation and validate that property via unit-tests.
Message was sent while issue was closed.
On 2016/07/26 17:32:54, Wez wrote: > On 2016/07/26 02:48:23, gab wrote: > > > https://codereview.chromium.org/2145463002/diff/240001/base/threading/thread.cc > > File base/threading/thread.cc (right): > > > > > https://codereview.chromium.org/2145463002/diff/240001/base/threading/thread.... > > base/threading/thread.cc:140: // |thread_lock_| is required... > > On 2016/07/25 21:20:29, Wez wrote: > > > FWIW, this comment as worded implies that callers were mis-using > > Start()/Stop() > > > in calling them from different Sequences, where AFAICT the issue is > actually > > > that you're specifically changing the API contract in this CL. > > > > > > It would be helpful to have the CL description and/or bug document why > > imposing > > > the same-Sequence restriction is preferable to the use of locks here. Right > > now > > > there's a cyclical reference between CL and bug rationale. :P > > > > Generally in Chromium, unless specified otherwise, classes are not to be > assumed > > as thread-safe. > > Yup, I'm not arguing with that. Just pointing out that your CL and bug > descriptions refer to one another as motivations for changes without providing > the motivation context. Right, thanks for pointing this out, hopefully with this discussion on both sides, it's now clarified :-). > > > The > > previous comment on Thread::StopSoon() makes this clear : "We should only be > > called on the same thread that started us.". > > Agreed; there was a change last year that added the unnecessary AutoLock in > Stop(), it seems. > > [snip] > > > > https://codereview.chromium.org/2145463002/diff/240001/base/threading/thread.... > > base/threading/thread.cc:203: DCHECK_EQ(id_, PlatformThread::CurrentId()); > > On 2016/07/25 21:22:36, Wez wrote: > > > What is this DCHECK intended to protect against? Comment implies you're > > > concerned about sub-classes calling it on themselves..? > > > > As is often the case with DCHECKs, it's intended as documentation of the > > expected contract. Since every other method uses |owning_sequence_checker_|, I > > wanted to make it clear that it wasn't omitted here by accident but rather > > because this runs on the underlying thread, not on the owning sequence. > > That's true of public-facing APIs, but this one is [intended to be] called by > the Thread itself. Where there's a contract within an implementation (i.e. no > scope for callers breaking things) it's common to omit DCHECKs in the > implementation and validate that property via unit-tests. Sure, the DCHECK might not be very helpful to catch misuses, but I think it still helps readability for anyone grokking thread.cc without hindering anyone else that doesn't care, so why not?
Message was sent while issue was closed.
On Tue, Jul 26, 2016 at 1:37 PM, <gab@chromium.org> wrote: > On 2016/07/26 17:32:54, Wez wrote: > > On 2016/07/26 02:48:23, gab wrote: > > > > > > > https://codereview.chromium.org/2145463002/diff/240001/base/threading/thread.cc > > > File base/threading/thread.cc (right): > > > > > > > > > > https://codereview.chromium.org/2145463002/diff/240001/base/threading/thread.... > > > base/threading/thread.cc:140: // |thread_lock_| is required... > > > On 2016/07/25 21:20:29, Wez wrote: > > > > FWIW, this comment as worded implies that callers were mis-using > > > Start()/Stop() > > > > in calling them from different Sequences, where AFAICT the issue is > > actually > > > > that you're specifically changing the API contract in this CL. > > > > > > > > It would be helpful to have the CL description and/or bug document > why > > > imposing > > > > the same-Sequence restriction is preferable to the use of locks here. > Right > > > now > > > > there's a cyclical reference between CL and bug rationale. :P > > > > > > Generally in Chromium, unless specified otherwise, classes are not to > be > > assumed > > > as thread-safe. > > > > Yup, I'm not arguing with that. Just pointing out that your CL and bug > > descriptions refer to one another as motivations for changes without > providing > > the motivation context. > > Right, thanks for pointing this out, hopefully with this discussion on both > sides, it's now clarified :-). > > > > > > The > > > previous comment on Thread::StopSoon() makes this clear : "We should > only be > > > called on the same thread that started us.". > > > > Agreed; there was a change last year that added the unnecessary AutoLock > in > > Stop(), it seems. > > > > [snip] > > > > > > > > > https://codereview.chromium.org/2145463002/diff/240001/base/threading/thread.... > > > base/threading/thread.cc:203: DCHECK_EQ(id_, > PlatformThread::CurrentId()); > > > On 2016/07/25 21:22:36, Wez wrote: > > > > What is this DCHECK intended to protect against? Comment implies > you're > > > > concerned about sub-classes calling it on themselves..? > > > > > > As is often the case with DCHECKs, it's intended as documentation of > the > > > expected contract. Since every other method uses > |owning_sequence_checker_|, > I > > > wanted to make it clear that it wasn't omitted here by accident but > rather > > > because this runs on the underlying thread, not on the owning sequence. > > > > That's true of public-facing APIs, but this one is [intended to be] > called by > > the Thread itself. Where there's a contract within an implementation > (i.e. no > > scope for callers breaking things) it's common to omit DCHECKs in the > > implementation and validate that property via unit-tests. > > Sure, the DCHECK might not be very helpful to catch misuses, but I think it > still helps readability for anyone grokking thread.cc without hindering > anyone > else that doesn't care, so why not? > I like it personally. Classes that deal with multiple threads, I like a DCHECK and/or function name to say what thread for everything. -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
