Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2135413003:
Add |joinable| to Thread::Options (Closed)
DescriptionAdd |joinable| to Thread::Options
Required for MessageLoop backed non-joinable threads as discussed @
https://codereview.chromium.org/2103883004/#msg14
BUG=622400, 629716, 629139
Committed: https://crrev.com/9a473d5bfb65cffcb6576496232f66023af79fdf
Cr-Commit-Position: refs/heads/master@{#409597}
Patch Set 1 #Patch Set 2 : Extracted cleanup to https://codereview.chromium.org/2145463002 #Patch Set 3 : rebase #Patch Set 4 : self-review #
Total comments: 6
Patch Set 5 : merge up to r407479 #Patch Set 6 : comment on nullptr #
Total comments: 2
Patch Set 7 : Add ThreadTest.StartTwiceNonJoinableNotAllowed #Patch Set 8 : fix compile #
Total comments: 1
Patch Set 9 : fix race in ThreadTest.StartWithOptions_NonJoinable #Patch Set 10 : Document set_message_loop() further #Patch Set 11 : Also skip StopSoon() if there is no |run_loop_| to support set_message_loop() use case for TestBrow… #Patch Set 12 : Checking |run_loop_| doesn't work as ThreadMain could have not hit yet in tests -- introduce |using… #
Total comments: 2
Patch Set 13 : Add ThreadTest.ExternalMessageLoop #Patch Set 14 : Init() => InstallMessageLoop() -- Init was already used on Thread::... #
Total comments: 2
Patch Set 15 : Add ThreadTest.DestroyWhileRunning* tests which fail at the moment (#msg62) #Patch Set 16 : Force non-joinable threads to be leaked for now (or otherwise carefully/undeterministically wound d… #Patch Set 17 : Rebase on dependent AtomicFlag CL #Patch Set 18 : fix |thread_main_exited_| check when thread is destroyed without ever starting #Patch Set 19 : update dependent CL #Patch Set 20 : update dependent CL #
Total comments: 4
Patch Set 21 : DCHECK(message_loop); in SetMessageLoop() + |thread_main_exited_flag_| #Patch Set 22 : Force explicit leak of non-joinable threads for now. #Patch Set 23 : merge up to r408965 #Patch Set 24 : adapt merge with most recent changes #Patch Set 25 : ignore SetMessageLoop(nullptr) for now -- added TODO #Depends on Patchset: Dependent Patchsets: Messages
Total messages: 127 (87 generated)
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-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
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...
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #2 (id:60001) has been deleted
Patchset #3 (id:100001) has been deleted
Description was changed from ========== Add |joinable| to Thread::Options Required for MessageLoop backed non-joinable threads as discussed @ https://codereview.chromium.org/2103883004/#msg14 BUG=622400 ========== to ========== Add |joinable| to Thread::Options Required for MessageLoop backed non-joinable threads as discussed @ https://codereview.chromium.org/2103883004/#msg14 BUG=622400, 629716 ==========
gab@chromium.org changed reviewers: + danakj@chromium.org
Dana PTAL (this is based on https://codereview.chromium.org/2145463002/), thanks!
https://codereview.chromium.org/2135413003/diff/140001/base/threading/platfor... File base/threading/platform_thread_win.cc (right): https://codereview.chromium.org/2135413003/diff/140001/base/threading/platfor... base/threading/platform_thread_win.cc:207: return CreateThreadInternal(stack_size, delegate, nullptr, priority); mind a comment on what the nullptr is? https://codereview.chromium.org/2135413003/diff/140001/base/threading/thread.cc File base/threading/thread.cc (right): https://codereview.chromium.org/2135413003/diff/140001/base/threading/thread.... base/threading/thread.cc:149: AutoLock lock(thread_lock_); does this need to be above StopSoon? https://codereview.chromium.org/2135413003/diff/140001/base/threading/thread.h File base/threading/thread.h (right): https://codereview.chromium.org/2135413003/diff/140001/base/threading/thread.... base/threading/thread.h:140: // Signals the thread to exit and returns once the thread has exited (or right A thought: what if we made it illegal to call Stop on non-joinable threads and you must use StopSoon instead?
Thanks, replies below. https://codereview.chromium.org/2135413003/diff/140001/base/threading/platfor... File base/threading/platform_thread_win.cc (right): https://codereview.chromium.org/2135413003/diff/140001/base/threading/platfor... base/threading/platform_thread_win.cc:207: return CreateThreadInternal(stack_size, delegate, nullptr, priority); On 2016/07/22 21:00:39, danakj wrote: > mind a comment on what the nullptr is? Done. https://codereview.chromium.org/2135413003/diff/140001/base/threading/thread.cc File base/threading/thread.cc (right): https://codereview.chromium.org/2135413003/diff/140001/base/threading/thread.... base/threading/thread.cc:149: AutoLock lock(thread_lock_); On 2016/07/22 21:00:39, danakj wrote: > does this need to be above StopSoon? Well... hopefully it's fully gone soon but in the meantime (while the above check is disabled), it at least makes sure the thread is started before initiating its destruction (whether it involves a Join() or not)... IDK... feels slightly safer and probably doesn't matter? https://codereview.chromium.org/2135413003/diff/140001/base/threading/thread.h File base/threading/thread.h (right): https://codereview.chromium.org/2135413003/diff/140001/base/threading/thread.... base/threading/thread.h:140: // Signals the thread to exit and returns once the thread has exited (or right On 2016/07/22 21:00:40, danakj wrote: > A thought: what if we made it illegal to call Stop on non-joinable threads and > you must use StopSoon instead? That was my first instinct too, but then I realized that Stop() is called by the destructor and there is a strongly documented contract that any subclass must also call Stop() in its destructor (ref. ~Thread() above). Therefore Stop() must be defined and do the right thing for non-joinable threads.
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_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
https://codereview.chromium.org/2135413003/diff/180001/base/threading/thread.h File base/threading/thread.h (right): https://codereview.chromium.org/2135413003/diff/180001/base/threading/thread.... base/threading/thread.h:144: // threads are not re-usable. Is there some DCHECK that will fire if you try to reuse it after calling Stop?
https://codereview.chromium.org/2135413003/diff/180001/base/threading/thread.h File base/threading/thread.h (right): https://codereview.chromium.org/2135413003/diff/180001/base/threading/thread.... base/threading/thread.h:144: // threads are not re-usable. On 2016/07/25 18:32:53, danakj wrote: > Is there some DCHECK that will fire if you try to reuse it after calling Stop? Yes, stopping_ is never reset when not joining (remains |true| forever) and there's a new DCHECK in StartWithOptions() to catch that. I also just added a death test for this to confirm it works (and prevent regressions).
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...
Patchset #8 (id:220001) has been deleted
Patchset #7 (id:200001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
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...)
Thanks, LGTM
https://codereview.chromium.org/2135413003/diff/260001/base/threading/thread.cc File base/threading/thread.cc (right): https://codereview.chromium.org/2135413003/diff/260001/base/threading/thread.... base/threading/thread.cc:151: StopSoon(); Note: this is causing try job failures in unittests because TestBrowserThreadBundle binds the same MessageLoop to multiple Thread objects. Beautiful, will address tomorrow. Cheers!
On 2016/07/25 21:09:00, gab wrote: > https://codereview.chromium.org/2135413003/diff/260001/base/threading/thread.cc > File base/threading/thread.cc (right): > > https://codereview.chromium.org/2135413003/diff/260001/base/threading/thread.... > base/threading/thread.cc:151: StopSoon(); > Note: this is causing try job failures in unittests because > TestBrowserThreadBundle binds the same MessageLoop to multiple Thread objects. > Beautiful, will address tomorrow. Cheers! Problem solved by no-oping StopSoon() if |!run_loop_|. Basically if |set_message_loop()| is used instead of Start(), the |message_loop_| is not owned by this Thread and therefore there is a |message_loop_| but no |run_loop_| and calling ThreadQuitHelper() on a MessageLoop we don't own is a problem. Added a comment clarifying cases when |StopSoon()| is no-op'ed. Will CQ, happy to do a follow-up CL if there's any nits on latest patch set. Cheers, Gab
The CQ bit was checked by gab@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org Link to the patchset: https://codereview.chromium.org/2135413003/#ps320001 (title: "Also skip StopSoon() if there is no |run_loop_| to support set_message_loop() use case for TestBrow…")
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
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
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...
The CQ bit was unchecked by gab@chromium.org
On 2016/07/26 16:36:46, gab wrote: > On 2016/07/25 21:09:00, gab wrote: > > > https://codereview.chromium.org/2135413003/diff/260001/base/threading/thread.cc > > File base/threading/thread.cc (right): > > > > > https://codereview.chromium.org/2135413003/diff/260001/base/threading/thread.... > > base/threading/thread.cc:151: StopSoon(); > > Note: this is causing try job failures in unittests because > > TestBrowserThreadBundle binds the same MessageLoop to multiple Thread objects. > > Beautiful, will address tomorrow. Cheers! > > Problem solved by no-oping StopSoon() if |!run_loop_|. Basically if > |set_message_loop()| is used instead of Start(), the |message_loop_| is not > owned by this Thread and therefore there is a |message_loop_| but no |run_loop_| > and calling ThreadQuitHelper() on a MessageLoop we don't own is a problem. Added > a comment clarifying cases when |StopSoon()| is no-op'ed. Actually that didn't work as |run_loop_| is set in ThreadMain and some tests run fast enough for Stop() to be called before ThreadMain() hits for underlying thread (plus it was racy as |run_loop_| is managed by underlying thread...). Introduced |bool using_external_message_loop_ = false;| to solve the problem of not handling StopSoon() for Threads bound to unowned MessageLoops. This whole "externally provided loop with no underlying thread on base::Thread" thing is kind of silly but this is how BrowserThreadImpl are faked in tests and we have to live with it for now... @dana PTAnL, thanks
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/2135413003/diff/340001/base/threading/thread.cc File base/threading/thread.cc (right): https://codereview.chromium.org/2135413003/diff/340001/base/threading/thread.... base/threading/thread.cc:180: stopping_ = true; Is it not weird that Thread will leave Stop() with stopping_ set to true?
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...
Added ThreadTest.ExternalMessageLoop (which fails on patch set 10 and passes on 12+), PTAnL. Thanks! Gab https://codereview.chromium.org/2135413003/diff/340001/base/threading/thread.cc File base/threading/thread.cc (right): https://codereview.chromium.org/2135413003/diff/340001/base/threading/thread.... base/threading/thread.cc:180: stopping_ = true; On 2016/07/26 19:54:25, danakj wrote: > Is it not weird that Thread will leave Stop() with stopping_ set to true? For non-joinable threads? I don't think so, attempting to confirm that they have stopped is the opposite of the intent of a non-joinable thread (plus only ThreadMain would know when to set |stopping_| but it isn't protected by a lock so it can only be set on the owning sequence (it's not *impossible* to do a TaskRunner dance here to set this bit when done but I really don't think it's necessary).
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.
One last thing that just occurred to me : non-joinable base::Thread instances should never be deleted before their ThreadMain has exited or they would be accessing freed memory... How about a DCHECK in destructor with an AtomicFlag set at end of ThreadMain? On 2016/07/26 20:25:34, gab wrote: > Added ThreadTest.ExternalMessageLoop (which fails on patch set 10 and passes on > 12+), PTAnL. > > Thanks! > Gab > > https://codereview.chromium.org/2135413003/diff/340001/base/threading/thread.cc > File base/threading/thread.cc (right): > > https://codereview.chromium.org/2135413003/diff/340001/base/threading/thread.... > base/threading/thread.cc:180: stopping_ = true; > On 2016/07/26 19:54:25, danakj wrote: > > Is it not weird that Thread will leave Stop() with stopping_ set to true? > > For non-joinable threads? I don't think so, attempting to confirm that they have > stopped is the opposite of the intent of a non-joinable thread (plus only > ThreadMain would know when to set |stopping_| but it isn't protected by a lock > so it can only be set on the owning sequence (it's not *impossible* to do a > TaskRunner dance here to set this bit when done but I really don't think it's > necessary).
On 2016/07/27 18:26:02, gab wrote: > One last thing that just occurred to me : non-joinable base::Thread instances > should never be deleted before their ThreadMain has exited or they would be > accessing freed memory... How about a DCHECK in destructor with an AtomicFlag > set at end of ThreadMain? Hm, so you can't join it, but you have to use a WaitableEvent before destroying it, which sounds a lot like joining. Or I guess you have to leak the Thread? Is that that you're thinking? > On 2016/07/26 20:25:34, gab wrote: > > Added ThreadTest.ExternalMessageLoop (which fails on patch set 10 and passes > on > > 12+), PTAnL. > > > > Thanks! > > Gab > > > > > https://codereview.chromium.org/2135413003/diff/340001/base/threading/thread.cc > > File base/threading/thread.cc (right): > > > > > https://codereview.chromium.org/2135413003/diff/340001/base/threading/thread.... > > base/threading/thread.cc:180: stopping_ = true; > > On 2016/07/26 19:54:25, danakj wrote: > > > Is it not weird that Thread will leave Stop() with stopping_ set to true? > > > > For non-joinable threads? I don't think so, attempting to confirm that they > have > > stopped is the opposite of the intent of a non-joinable thread (plus only > > ThreadMain would know when to set |stopping_| but it isn't protected by a lock > > so it can only be set on the owning sequence (it's not *impossible* to do a > > TaskRunner dance here to set this bit when done but I really don't think it's > > necessary).
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...)
On 2016/07/27 20:25:45, danakj wrote: > On 2016/07/27 18:26:02, gab wrote: > > One last thing that just occurred to me : non-joinable base::Thread instances > > should never be deleted before their ThreadMain has exited or they would be > > accessing freed memory... How about a DCHECK in destructor with an AtomicFlag > > set at end of ThreadMain? > > Hm, so you can't join it, but you have to use a WaitableEvent before destroying > it, which sounds a lot like joining. Or I guess you have to leak the Thread? Is > that that you're thinking? WaitableEvent would indeed not make sense. For now the use case I care about is indeed leaking the Thread (https://codereview.chromium.org/2103883004/). I have a proposal @ https://crbug.com/629139#c14 for a way to make it possible to destroy a live instance of a non-joinable Thread (same technique will be required for a few off-thread users of Thread::Stop() it looks like), but I don't think we need to block this CL on it (added a DCHECK that ThreadMain() has returned when ~Thread() is hit for the time being). PTAL at latest patch set (16), thanks! > > > On 2016/07/26 20:25:34, gab wrote: > > > Added ThreadTest.ExternalMessageLoop (which fails on patch set 10 and passes > > on > > > 12+), PTAnL. > > > > > > Thanks! > > > Gab > > > > > > > > > https://codereview.chromium.org/2135413003/diff/340001/base/threading/thread.cc > > > File base/threading/thread.cc (right): > > > > > > > > > https://codereview.chromium.org/2135413003/diff/340001/base/threading/thread.... > > > base/threading/thread.cc:180: stopping_ = true; > > > On 2016/07/26 19:54:25, danakj wrote: > > > > Is it not weird that Thread will leave Stop() with stopping_ set to true? > > > > > > For non-joinable threads? I don't think so, attempting to confirm that they > > have > > > stopped is the opposite of the intent of a non-joinable thread (plus only > > > ThreadMain would know when to set |stopping_| but it isn't protected by a > lock > > > so it can only be set on the owning sequence (it's not *impossible* to do a > > > TaskRunner dance here to set this bit when done but I really don't think > it's > > > necessary).
Description was changed from ========== Add |joinable| to Thread::Options Required for MessageLoop backed non-joinable threads as discussed @ https://codereview.chromium.org/2103883004/#msg14 BUG=622400, 629716 ========== to ========== Add |joinable| to Thread::Options Required for MessageLoop backed non-joinable threads as discussed @ https://codereview.chromium.org/2103883004/#msg14 BUG=622400, 629716, 629139 ==========
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 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...
Why do you use an AtomicFlag here? The flag is only useful when you've joined, which is a barrier, or done something else nebulous which would be too, right? https://codereview.chromium.org/2135413003/diff/380001/base/threading/thread.cc File base/threading/thread.cc (right): https://codereview.chromium.org/2135413003/diff/380001/base/threading/thread.... base/threading/thread.cc:77: DCHECK(!stopping_) << "Starting a non-joinable thread a second time? That's " This would also fire if you were using an external message loop, cuz Stop would set stopping_=true now, when it didn't before. It's probably not something we need to support, but pointing out this DCHECK isn't only for non-joinable. Would it make sense to DCHECK(!using_external_message_loop_) first? https://codereview.chromium.org/2135413003/diff/500001/base/threading/thread.cc File base/threading/thread.cc (right): https://codereview.chromium.org/2135413003/diff/500001/base/threading/thread.... base/threading/thread.cc:64: DCHECK(!thread_main_exited_.get() || thread_main_exited_->IsSet()); no .get() https://codereview.chromium.org/2135413003/diff/500001/base/threading/thread.... base/threading/thread.cc:93: if (!thread_main_exited_.get()) { no .get()
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_...)
On 2016/07/28 19:07:40, danakj wrote: > Why do you use an AtomicFlag here? The flag is only useful when you've joined, > which is a barrier, or done something else nebulous which would be too, right? There is no possibility for real synchronization here, so the next best thing is "waiting long enough" which for non-atomics means it could take "infinity" to synchronize FWIU. Other option is to have a member variable and DCHECK(is_joinable_); in destructor but that makes it impossible to unittest non-joinable threads... https://codereview.chromium.org/2135413003/diff/380001/base/threading/thread.cc File base/threading/thread.cc (right): https://codereview.chromium.org/2135413003/diff/380001/base/threading/thread.... base/threading/thread.cc:77: DCHECK(!stopping_) << "Starting a non-joinable thread a second time? That's " On 2016/07/28 19:07:40, danakj wrote: > This would also fire if you were using an external message loop, cuz Stop would > set stopping_=true now, when it didn't before. It's probably not something we > need to support, but pointing out this DCHECK isn't only for non-joinable. > > Would it make sense to DCHECK(!using_external_message_loop_) first? DCHECK(!message_loop_) above covers that, right? https://codereview.chromium.org/2135413003/diff/500001/base/threading/thread.cc File base/threading/thread.cc (right): https://codereview.chromium.org/2135413003/diff/500001/base/threading/thread.... base/threading/thread.cc:64: DCHECK(!thread_main_exited_.get() || thread_main_exited_->IsSet()); On 2016/07/28 19:07:40, danakj wrote: > no .get() I intentionally added the .get()'s here and below because DCHECK(!thread_main_exited_ || thread_main_exited_->IsSet()); felt ackward (i.e. "not foo or foo is set" sounds like testing opposites...). https://codereview.chromium.org/2135413003/diff/500001/base/threading/thread.... base/threading/thread.cc:93: if (!thread_main_exited_.get()) { On 2016/07/28 19:07:40, danakj wrote: > no .get() Similarly here: if (!thread_main_exited_) feels like it's testing a boolean whereas the absence of the pointer means the opposite. So I feel like the .get() makes this more obvious and draws attention to the comment on the member variable explaining it.
On Thu, Jul 28, 2016 at 2:40 PM, <gab@chromium.org> wrote: > On 2016/07/28 19:07:40, danakj wrote: > > Why do you use an AtomicFlag here? The flag is only useful when you've > joined, > > which is a barrier, or done something else nebulous which would be too, > right? > > There is no possibility for real synchronization here, so the next best > thing is > "waiting long enough" which for non-atomics means it could take "infinity" > to > synchronize FWIU. > > > Other option is to have a member variable and DCHECK(is_joinable_); in > destructor but that makes it impossible to unittest non-joinable threads... > We'd just have to leave them like production code does? Or are you thinking of something else? > > > > > https://codereview.chromium.org/2135413003/diff/380001/base/threading/thread.cc > File base/threading/thread.cc (right): > > > https://codereview.chromium.org/2135413003/diff/380001/base/threading/thread.... > base/threading/thread.cc:77: DCHECK(!stopping_) << "Starting a > non-joinable thread a second time? That's " > On 2016/07/28 19:07:40, danakj wrote: > > This would also fire if you were using an external message loop, cuz > Stop would > > set stopping_=true now, when it didn't before. It's probably not > something we > > need to support, but pointing out this DCHECK isn't only for > non-joinable. > > > > Would it make sense to DCHECK(!using_external_message_loop_) first? > > DCHECK(!message_loop_) above covers that, right? > > > https://codereview.chromium.org/2135413003/diff/500001/base/threading/thread.cc > File base/threading/thread.cc (right): > > > https://codereview.chromium.org/2135413003/diff/500001/base/threading/thread.... > base/threading/thread.cc:64: DCHECK(!thread_main_exited_.get() || > thread_main_exited_->IsSet()); > On 2016/07/28 19:07:40, danakj wrote: > > no .get() > > I intentionally added the .get()'s here and below because > > DCHECK(!thread_main_exited_ || thread_main_exited_->IsSet()); > > felt ackward (i.e. "not foo or foo is set" sounds like testing > opposites...). > I don't see how the get changes that really? You're testing that if it exists it is set. > > > > https://codereview.chromium.org/2135413003/diff/500001/base/threading/thread.... > base/threading/thread.cc:93: if (!thread_main_exited_.get()) { > On 2016/07/28 19:07:40, danakj wrote: > > no .get() > > Similarly here: > > if (!thread_main_exited_) > > feels like it's testing a boolean whereas the absence of the pointer > means the opposite. So I feel like the .get() makes this more obvious > and draws attention to the comment on the member variable explaining it. > If you need to write .get() to see that maybe the variable should be named differently then. thread_main_exited_flag_ or something? > > https://codereview.chromium.org/2135413003/ > -- 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.
On 2016/07/28 21:44:50, danakj wrote: > On Thu, Jul 28, 2016 at 2:40 PM, <https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=gab@chromium.org> wrote: > > > On 2016/07/28 19:07:40, danakj wrote: > > > Why do you use an AtomicFlag here? The flag is only useful when you've > > joined, > > > which is a barrier, or done something else nebulous which would be too, > > right? > > > > There is no possibility for real synchronization here, so the next best > > thing is > > "waiting long enough" which for non-atomics means it could take "infinity" > > to > > synchronize FWIU. > > > > > > > > > Other option is to have a member variable and DCHECK(is_joinable_); in > > destructor but that makes it impossible to unittest non-joinable threads... > > > > We'd just have to leave them like production code does? Or are you thinking > of something else? Well production code will use LazyInstance::Leaky which isn't suitable for unittests (which can run in parallel AFAIK). Or we could just leak raw pointers, but then we'd have to annotate them for LSAN.... Overall the AtomicFlag sounds easier, I'm happy to put it behind DCHECK_ALWAYS_ON() if that makes you more comfortable with it. > > > > > > > > > > > > > https://codereview.chromium.org/2135413003/diff/380001/base/threading/thread.cc > > File base/threading/thread.cc (right): > > > > > > > https://codereview.chromium.org/2135413003/diff/380001/base/threading/thread.... > > base/threading/thread.cc:77: DCHECK(!stopping_) << "Starting a > > non-joinable thread a second time? That's " > > On 2016/07/28 19:07:40, danakj wrote: > > > This would also fire if you were using an external message loop, cuz > > Stop would > > > set stopping_=true now, when it didn't before. It's probably not > > something we > > > need to support, but pointing out this DCHECK isn't only for > > non-joinable. > > > > > > Would it make sense to DCHECK(!using_external_message_loop_) first? > > > > DCHECK(!message_loop_) above covers that, right? > > > > > > > https://codereview.chromium.org/2135413003/diff/500001/base/threading/thread.cc > > File base/threading/thread.cc (right): > > > > > > > https://codereview.chromium.org/2135413003/diff/500001/base/threading/thread.... > > base/threading/thread.cc:64: DCHECK(!thread_main_exited_.get() || > > thread_main_exited_->IsSet()); > > On 2016/07/28 19:07:40, danakj wrote: > > > no .get() > > > > I intentionally added the .get()'s here and below because > > > > DCHECK(!thread_main_exited_ || thread_main_exited_->IsSet()); > > > > felt ackward (i.e. "not foo or foo is set" sounds like testing > > opposites...). > > > > I don't see how the get changes that really? You're testing that if it > exists it is set. > > > > > > > > > > > https://codereview.chromium.org/2135413003/diff/500001/base/threading/thread.... > > base/threading/thread.cc:93: if (!thread_main_exited_.get()) { > > On 2016/07/28 19:07:40, danakj wrote: > > > no .get() > > > > Similarly here: > > > > if (!thread_main_exited_) > > > > feels like it's testing a boolean whereas the absence of the pointer > > means the opposite. So I feel like the .get() makes this more obvious > > and draws attention to the comment on the member variable explaining it. > > > > If you need to write .get() to see that maybe the variable should be named > differently then. thread_main_exited_flag_ or something? I like that very much, renamed (and removed .get()). > > > > > > https://codereview.chromium.org/2135413003/ > > > > -- > 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 https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=chromium-reviews+unsubscri....
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_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On Thu, Jul 28, 2016 at 4:35 PM, <gab@chromium.org> wrote: > On 2016/07/28 21:44:50, danakj wrote: > > On Thu, Jul 28, 2016 at 2:40 PM, > <https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=gab@chromium.org> > wrote: > > > > > On 2016/07/28 19:07:40, danakj wrote: > > > > Why do you use an AtomicFlag here? The flag is only useful when > you've > > > joined, > > > > which is a barrier, or done something else nebulous which would be > too, > > > right? > > > > > > There is no possibility for real synchronization here, so the next best > > > thing is > > > "waiting long enough" which for non-atomics means it could take > "infinity" > > > to > > > synchronize FWIU. > > > > > > > > > > > > > > > Other option is to have a member variable and DCHECK(is_joinable_); in > > > destructor but that makes it impossible to unittest non-joinable > threads... > > > > > > > We'd just have to leave them like production code does? Or are you > thinking > > of something else? > > > Well production code will use LazyInstance::Leaky which isn't suitable for > unittests (which can run in parallel AFAIK). > The unit test launcher spawns N processes to run M tests in each process, but those M run serially per process, is my understanding. > Or we could just leak raw pointers, > but then we'd have to annotate them for LSAN.... > That seems okay, it also annotates it for the reader of the test that the leak is intentional. > Overall the AtomicFlag sounds > easier, I'm happy to put it behind DCHECK_ALWAYS_ON() if that makes you > more > comfortable with it. > I'm almost always a fan of adding things to test over adding things to production. > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2135413003/diff/380001/base/threading/thread.cc > > > File base/threading/thread.cc (right): > > > > > > > > > > > > > https://codereview.chromium.org/2135413003/diff/380001/base/threading/thread.... > > > base/threading/thread.cc:77: DCHECK(!stopping_) << "Starting a > > > non-joinable thread a second time? That's " > > > On 2016/07/28 19:07:40, danakj wrote: > > > > This would also fire if you were using an external message loop, cuz > > > Stop would > > > > set stopping_=true now, when it didn't before. It's probably not > > > something we > > > > need to support, but pointing out this DCHECK isn't only for > > > non-joinable. > > > > > > > > Would it make sense to DCHECK(!using_external_message_loop_) first? > > > > > > DCHECK(!message_loop_) above covers that, right? > > > > > > > > > > > > > https://codereview.chromium.org/2135413003/diff/500001/base/threading/thread.cc > > > File base/threading/thread.cc (right): > > > > > > > > > > > > > https://codereview.chromium.org/2135413003/diff/500001/base/threading/thread.... > > > base/threading/thread.cc:64: DCHECK(!thread_main_exited_.get() || > > > thread_main_exited_->IsSet()); > > > On 2016/07/28 19:07:40, danakj wrote: > > > > no .get() > > > > > > I intentionally added the .get()'s here and below because > > > > > > DCHECK(!thread_main_exited_ || thread_main_exited_->IsSet()); > > > > > > felt ackward (i.e. "not foo or foo is set" sounds like testing > > > opposites...). > > > > > > > I don't see how the get changes that really? You're testing that if it > > exists it is set. > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2135413003/diff/500001/base/threading/thread.... > > > base/threading/thread.cc:93: if (!thread_main_exited_.get()) { > > > On 2016/07/28 19:07:40, danakj wrote: > > > > no .get() > > > > > > Similarly here: > > > > > > if (!thread_main_exited_) > > > > > > feels like it's testing a boolean whereas the absence of the pointer > > > means the opposite. So I feel like the .get() makes this more obvious > > > and draws attention to the comment on the member variable explaining > it. > > > > > > > If you need to write .get() to see that maybe the variable should be > named > > differently then. thread_main_exited_flag_ or something? > > I like that very much, renamed (and removed .get()). > > > > > > > > > > > https://codereview.chromium.org/2135413003/ > > > > > > > -- > > 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 > > https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=chromium-reviews+unsubscri... > . > > > > https://codereview.chromium.org/2135413003/ > -- 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.
Patchset #22 (id:540001) has been deleted
Ok, replaced AtomicFlag with |bool joinable_|, added checks for it (prevents Stop() from being called (and updated documentation as such until I fix the requirement to leak), and annotated tests. PTAnL, thanks!
gab@chromium.org changed reviewers: + thestig@chromium.org
On 2016/08/01 15:04:31, gab wrote: > Ok, replaced AtomicFlag with |bool joinable_|, added checks for it (prevents > Stop() from being called (and updated documentation as such until I fix the > requirement to leak), and annotated tests. > > PTAnL, thanks! @thestig in Dana's absence : she was already fine with this whole CL % a last minute change which I addressed in latest patch set.
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...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
On 2016/08/01 18:21:55, gab wrote: > On 2016/08/01 15:04:31, gab wrote: > > Ok, replaced AtomicFlag with |bool joinable_|, added checks for it (prevents > > Stop() from being called (and updated documentation as such until I fix the > > requirement to leak), and annotated tests. > > > > PTAnL, thanks! > > @thestig in Dana's absence : she was already fine with this whole CL % a last > minute change which I addressed in latest patch set. Is there something in particular I should look at, or just the entire CL? There are red trybots.
On 2016/08/01 21:12:07, Lei Zhang wrote: > On 2016/08/01 18:21:55, gab wrote: > > On 2016/08/01 15:04:31, gab wrote: > > > Ok, replaced AtomicFlag with |bool joinable_|, added checks for it (prevents > > > Stop() from being called (and updated documentation as such until I fix the > > > requirement to leak), and annotated tests. > > > > > > PTAnL, thanks! > > > > @thestig in Dana's absence : she was already fine with this whole CL % a last > > minute change which I addressed in latest patch set. > > Is there something in particular I should look at, or just the entire CL? Dana has lgtm'ed the overall CL's concept @ PS 10ish, but a few quirks were discovered since. She was fine with everything in PS 21 besides the use of the AtomicFlag which I've replaced by a bool and explicit leak in latest PS. It's perhaps easier to look at the whole thing from scratch though... > > There are red trybots. Oops, documented issue @ https://bugs.chromium.org/p/chromium/issues/detail?id=629139#c15 (this CL and previous ones are highlighting surprising/problematic existing usage of the base::Thread API. I'll address those as a follow-up. I don't have access to my desktop to update another PS right now but I'll just have to early return in SetMessageLoop() |if (!message_loop)| for now: https://bugs.chromium.org/p/chromium/issues/detail?id=629139#c15 Thanks! Gab
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...
On 2016/08/02 01:43:37, gab wrote: > On 2016/08/01 21:12:07, Lei Zhang wrote: > > On 2016/08/01 18:21:55, gab wrote: > > > On 2016/08/01 15:04:31, gab wrote: > > > > Ok, replaced AtomicFlag with |bool joinable_|, added checks for it > (prevents > > > > Stop() from being called (and updated documentation as such until I fix > the > > > > requirement to leak), and annotated tests. > > > > > > > > PTAnL, thanks! > > > > > > @thestig in Dana's absence : she was already fine with this whole CL % a > last > > > minute change which I addressed in latest patch set. > > > > Is there something in particular I should look at, or just the entire CL? > > Dana has lgtm'ed the overall CL's concept @ PS 10ish, but a few quirks were > discovered since. She was fine with everything in PS 21 besides the use of the > AtomicFlag which I've replaced by a bool and explicit leak in latest PS. > > It's perhaps easier to look at the whole thing from scratch though... > > > > > There are red trybots. > > Oops, documented issue @ > https://bugs.chromium.org/p/chromium/issues/detail?id=629139#c15 (this CL and > previous ones are highlighting surprising/problematic existing usage of the > base::Thread API. I'll address those as a follow-up. > > I don't have access to my desktop to update another PS right now but I'll just > have to early return in SetMessageLoop() |if (!message_loop)| for now: > https://bugs.chromium.org/p/chromium/issues/detail?id=629139#c15 Done, try bots are green now :-). > > Thanks! > Gab
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/08/02 15:11:56, gab wrote: > On 2016/08/02 01:43:37, gab wrote: > > On 2016/08/01 21:12:07, Lei Zhang wrote: > > > On 2016/08/01 18:21:55, gab wrote: > > > > On 2016/08/01 15:04:31, gab wrote: > > > > > Ok, replaced AtomicFlag with |bool joinable_|, added checks for it > > (prevents > > > > > Stop() from being called (and updated documentation as such until I fix > > the > > > > > requirement to leak), and annotated tests. > > > > > > > > > > PTAnL, thanks! > > > > > > > > @thestig in Dana's absence : she was already fine with this whole CL % a > > last > > > > minute change which I addressed in latest patch set. > > > > > > Is there something in particular I should look at, or just the entire CL? > > > > Dana has lgtm'ed the overall CL's concept @ PS 10ish, but a few quirks were > > discovered since. She was fine with everything in PS 21 besides the use of the > > AtomicFlag which I've replaced by a bool and explicit leak in latest PS. > > > > It's perhaps easier to look at the whole thing from scratch though... > > > > > > > > There are red trybots. > > > > Oops, documented issue @ > > https://bugs.chromium.org/p/chromium/issues/detail?id=629139#c15 (this CL and > > previous ones are highlighting surprising/problematic existing usage of the > > base::Thread API. I'll address those as a follow-up. > > > > I don't have access to my desktop to update another PS right now but I'll just > > have to early return in SetMessageLoop() |if (!message_loop)| for now: > > https://bugs.chromium.org/p/chromium/issues/detail?id=629139#c15 > > Done, try bots are green now :-). ping thestig, thanks!
On 2016/08/02 21:28:05, gab wrote: > ping thestig, thanks! Busy/slow pong. I need a few hours to get through everything else for today.
On 2016/08/02 21:32:24, Lei Zhang wrote: > On 2016/08/02 21:28:05, gab wrote: > > ping thestig, thanks! > > Busy/slow pong. I need a few hours to get through everything else for today. Ok thanks, this is blocking 2 already lgtm'ed CLs and eventually the SequencedWorkerPool redirection to TaskScheduler, that's why I'm making sure it's on your radar.
lgtm
The CQ bit was checked by gab@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org Link to the patchset: https://codereview.chromium.org/2135413003/#ps620001 (title: "ignore SetMessageLoop(nullptr) for now -- added TODO")
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
gab@chromium.org changed reviewers: + jam@chromium.org
TBR jam@ for naming side-effects on content/browser/browser_thread_impl.cc and its iOS twin (ios/web/web_thread_impl.cc)
Description was changed from ========== Add |joinable| to Thread::Options Required for MessageLoop backed non-joinable threads as discussed @ https://codereview.chromium.org/2103883004/#msg14 BUG=622400, 629716, 629139 ========== to ========== Add |joinable| to Thread::Options Required for MessageLoop backed non-joinable threads as discussed @ https://codereview.chromium.org/2103883004/#msg14 BUG=622400, 629716, 629139 TBR=jam (renaming side-effects) ==========
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/08/03 17:24:54, gab wrote: > TBR jam@ for naming side-effects on content/browser/browser_thread_impl.cc and > its iOS twin (ios/web/web_thread_impl.cc) lgtm
Description was changed from ========== Add |joinable| to Thread::Options Required for MessageLoop backed non-joinable threads as discussed @ https://codereview.chromium.org/2103883004/#msg14 BUG=622400, 629716, 629139 TBR=jam (renaming side-effects) ========== to ========== Add |joinable| to Thread::Options Required for MessageLoop backed non-joinable threads as discussed @ https://codereview.chromium.org/2103883004/#msg14 BUG=622400, 629716, 629139 ==========
Message was sent while issue was closed.
Description was changed from ========== Add |joinable| to Thread::Options Required for MessageLoop backed non-joinable threads as discussed @ https://codereview.chromium.org/2103883004/#msg14 BUG=622400, 629716, 629139 ========== to ========== Add |joinable| to Thread::Options Required for MessageLoop backed non-joinable threads as discussed @ https://codereview.chromium.org/2103883004/#msg14 BUG=622400, 629716, 629139 ==========
Message was sent while issue was closed.
Committed patchset #25 (id:620001)
Message was sent while issue was closed.
Description was changed from ========== Add |joinable| to Thread::Options Required for MessageLoop backed non-joinable threads as discussed @ https://codereview.chromium.org/2103883004/#msg14 BUG=622400, 629716, 629139 ========== to ========== Add |joinable| to Thread::Options Required for MessageLoop backed non-joinable threads as discussed @ https://codereview.chromium.org/2103883004/#msg14 BUG=622400, 629716, 629139 Committed: https://crrev.com/9a473d5bfb65cffcb6576496232f66023af79fdf Cr-Commit-Position: refs/heads/master@{#409597} ==========
Message was sent while issue was closed.
Patchset 25 (id:??) landed as https://crrev.com/9a473d5bfb65cffcb6576496232f66023af79fdf Cr-Commit-Position: refs/heads/master@{#409597} |
