Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2204333003:
Add joinable option to SimpleThread::Options as was just done for Thread. (Closed)
|
Created:
4 years, 4 months ago by gab Modified:
4 years, 4 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, kinuko+watch, jam, darin-cc_chromium.org, blink-worker-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@b1_nonjoinable_thread Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd joinable option to SimpleThread::Options as was just done for Thread::Options.
SimpleThread will be more suitable than Thread for
https://codereview.chromium.org/2197753002/ per it not needing a MessageLoop.
Also, cleanup the SimpleThread::Options API, making it more like Thread::Options.
Non-joinable SimpleThreads will have to remain alive so long as their Run() method uses
member state (SimpleThread itself won't after invoking Run()).
DelegateSimpleThread provides a safe way to do this by explicitly supporting
being deleted while Run() is active but ensuring the Delegate itself outlives Run()).
Note: it might seem overkill to let DelegateSimpleThread be deleted early but this is
eventually what I want to do for base::Thread (https://crbug.com/629139#c14) and this
is a good precursor to this paradigm (it really takes all its sense on base::Thread where
the destructor is more meaningful though -- winds down the MessageLoop).
The logic on SimpleThread is fairly simple, the complexity is in the assertions (which
were put behind DCHECK_IS_ON() to clearly isolate them) and the tests.
BUG=629716, 634835, 629139
TBR=avi@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_tsan_rel_ng
Committed: https://crrev.com/8b35b0572443f4a84377a1fbd199a1edabe3d7d6
Cr-Commit-Position: refs/heads/master@{#409830}
Reverted: https://crrev.com/a9094bdb2cda679e8bf3a5b43b27c12efa975d3c
Committed: https://crrev.com/addc8ef1bcae5bff546fba73eca8bea3e640ebcd
Cr-Original-Commit-Position: refs/heads/master@{#410169}
Cr-Commit-Position: refs/heads/master@{#411897}
Patch Set 1 #
Total comments: 8
Patch Set 2 : review:thestig #1 #Patch Set 3 : rebase on r410390 post revert #Patch Set 4 : New lifetime rules around non-joinable SimpleThreads (crbug/634835) #Patch Set 5 : Format and fix ASan #Patch Set 6 : Fix ASan for real? #Patch Set 7 : WaitableEvents owned by test fixture #Patch Set 8 : fix TSan? #Patch Set 9 : ScopedAllowWait for destructor dcheck #Patch Set 10 : +friend for ScopedAllowWait.. #
Total comments: 8
Patch Set 11 : nits:thestig #
Total comments: 9
Patch Set 12 : Remove DCHECK => rely on ASAN/TSAN for early Delegate deletion #
Total comments: 4
Patch Set 13 : simplify Delegate interface further #Patch Set 14 : fix compile #Patch Set 15 : fix TSan and ASan #
Messages
Total messages: 144 (102 generated)
Description was changed from ========== Add joinable option to SimpleThread::Options as was just done for Thread. SimpleThread will be more suitable than Thread for https://codereview.chromium.org/2197753002/ per it not needing a MessageLoop. BUG=629716 ========== to ========== Add joinable option to SimpleThread::Options as was just done for Thread::Options. SimpleThread will be more suitable than Thread for https://codereview.chromium.org/2197753002/ per it not needing a MessageLoop. Also, cleanup the SimpleThread::Options API, making it more like Thread::Options. 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...
gab@chromium.org changed reviewers: + thestig@chromium.org
Lei PTAL, much simpler than the base::Thread version. Thanks! Gab
On 2016/08/03 21:13:07, gab wrote: > Lei PTAL, much simpler than the base::Thread version. PS: base_unittests pass locally if there are bots failures it's probably just missing getters/renames I haven't found through code search, doesn't prevent review, thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Looks ok. I think one of the red bots is yours. https://codereview.chromium.org/2204333003/diff/1/base/threading/simple_threa... File base/threading/simple_thread.cc (right): https://codereview.chromium.org/2204333003/diff/1/base/threading/simple_threa... base/threading/simple_thread.cc:45: DCHECK(!thread_.is_null()) << "A non-joinable thread can't be joined."; Put this before or after the HasBeen twins? https://codereview.chromium.org/2204333003/diff/1/base/threading/simple_thread.h File base/threading/simple_thread.h (right): https://codereview.chromium.org/2204333003/diff/1/base/threading/simple_threa... base/threading/simple_thread.h:122: PlatformThreadId tid_ = 0; // The backing thread's id. Maybe init to kInvalidThreadId? (which is still 0) https://codereview.chromium.org/2204333003/diff/1/base/threading/simple_threa... base/threading/simple_thread.h:195: DISALLOW_COPY_AND_ASSIGN(DelegateSimpleThreadPool); +3 :) https://codereview.chromium.org/2204333003/diff/1/base/threading/simple_threa... File base/threading/simple_thread_unittest.cc (right): https://codereview.chromium.org/2204333003/diff/1/base/threading/simple_threa... base/threading/simple_thread_unittest.cc:24: PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(20)); no need for base:: inside base.
The CQ bit was checked by gab@chromium.org to run a CQ dry run
On 2016/08/03 23:24:12, Lei Zhang wrote: > Looks ok. I think one of the red bots is yours. All bots are green? There are red bots in "More" though but I haven't relaunched any so I guess automatic retries? Those were failures I also saw on other CLs so perhaps a bad commit in tested range? https://codereview.chromium.org/2204333003/diff/1/base/threading/simple_threa... File base/threading/simple_thread.cc (right): https://codereview.chromium.org/2204333003/diff/1/base/threading/simple_threa... base/threading/simple_thread.cc:45: DCHECK(!thread_.is_null()) << "A non-joinable thread can't be joined."; On 2016/08/03 23:24:11, Lei Zhang wrote: > Put this before or after the HasBeen twins? Done. https://codereview.chromium.org/2204333003/diff/1/base/threading/simple_thread.h File base/threading/simple_thread.h (right): https://codereview.chromium.org/2204333003/diff/1/base/threading/simple_threa... base/threading/simple_thread.h:122: PlatformThreadId tid_ = 0; // The backing thread's id. On 2016/08/03 23:24:12, Lei Zhang wrote: > Maybe init to kInvalidThreadId? (which is still 0) Good point, done. https://codereview.chromium.org/2204333003/diff/1/base/threading/simple_threa... base/threading/simple_thread.h:195: DISALLOW_COPY_AND_ASSIGN(DelegateSimpleThreadPool); On 2016/08/03 23:24:11, Lei Zhang wrote: > +3 :) :-) https://codereview.chromium.org/2204333003/diff/1/base/threading/simple_threa... File base/threading/simple_thread_unittest.cc (right): https://codereview.chromium.org/2204333003/diff/1/base/threading/simple_threa... base/threading/simple_thread_unittest.cc:24: PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(20)); On 2016/08/03 23:24:12, Lei Zhang wrote: > no need for base:: inside base. Done.
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.
lgtm
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from ========== Add joinable option to SimpleThread::Options as was just done for Thread::Options. SimpleThread will be more suitable than Thread for https://codereview.chromium.org/2197753002/ per it not needing a MessageLoop. Also, cleanup the SimpleThread::Options API, making it more like Thread::Options. BUG=629716 ========== to ========== Add joinable option to SimpleThread::Options as was just done for Thread::Options. SimpleThread will be more suitable than Thread for https://codereview.chromium.org/2197753002/ per it not needing a MessageLoop. Also, cleanup the SimpleThread::Options API, making it more like Thread::Options. BUG=629716 TBR=avi@chromium.org ==========
gab@chromium.org changed reviewers: + avi@chromium.org
TBR avi for content/renderer/categorized_worker_pool.cc 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...
Message was sent while issue was closed.
Description was changed from ========== Add joinable option to SimpleThread::Options as was just done for Thread::Options. SimpleThread will be more suitable than Thread for https://codereview.chromium.org/2197753002/ per it not needing a MessageLoop. Also, cleanup the SimpleThread::Options API, making it more like Thread::Options. BUG=629716 TBR=avi@chromium.org ========== to ========== Add joinable option to SimpleThread::Options as was just done for Thread::Options. SimpleThread will be more suitable than Thread for https://codereview.chromium.org/2197753002/ per it not needing a MessageLoop. Also, cleanup the SimpleThread::Options API, making it more like Thread::Options. BUG=629716 TBR=avi@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Add joinable option to SimpleThread::Options as was just done for Thread::Options. SimpleThread will be more suitable than Thread for https://codereview.chromium.org/2197753002/ per it not needing a MessageLoop. Also, cleanup the SimpleThread::Options API, making it more like Thread::Options. BUG=629716 TBR=avi@chromium.org ========== to ========== Add joinable option to SimpleThread::Options as was just done for Thread::Options. SimpleThread will be more suitable than Thread for https://codereview.chromium.org/2197753002/ per it not needing a MessageLoop. Also, cleanup the SimpleThread::Options API, making it more like Thread::Options. BUG=629716 TBR=avi@chromium.org Committed: https://crrev.com/8b35b0572443f4a84377a1fbd199a1edabe3d7d6 Cr-Commit-Position: refs/heads/master@{#409830} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/8b35b0572443f4a84377a1fbd199a1edabe3d7d6 Cr-Commit-Position: refs/heads/master@{#409830}
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/2223433003/ by justincohen@chromium.org. The reason for reverting is: Reverting this since it is breaking the iOS base_unittests. gba@ is working on a fix in https://codereview.chromium.org/2218983003/ .
Message was sent while issue was closed.
Description was changed from ========== Add joinable option to SimpleThread::Options as was just done for Thread::Options. SimpleThread will be more suitable than Thread for https://codereview.chromium.org/2197753002/ per it not needing a MessageLoop. Also, cleanup the SimpleThread::Options API, making it more like Thread::Options. BUG=629716 TBR=avi@chromium.org Committed: https://crrev.com/8b35b0572443f4a84377a1fbd199a1edabe3d7d6 Cr-Commit-Position: refs/heads/master@{#409830} ========== to ========== Add joinable option to SimpleThread::Options as was just done for Thread::Options. SimpleThread will be more suitable than Thread for https://codereview.chromium.org/2197753002/ per it not needing a MessageLoop. Also, cleanup the SimpleThread::Options API, making it more like Thread::Options. BUG=629716, 634835 TBR=avi@chromium.org Committed: https://crrev.com/8b35b0572443f4a84377a1fbd199a1edabe3d7d6 Cr-Commit-Position: refs/heads/master@{#409830} ==========
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_asan_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 checked by gab@chromium.org to run a CQ dry run
Patchset #5 (id:80001) has been deleted
Patchset #5 (id:100001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Patchset #5 (id:120001) has been deleted
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 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 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_...)
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.
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...
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_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
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_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
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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 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.
Description was changed from ========== Add joinable option to SimpleThread::Options as was just done for Thread::Options. SimpleThread will be more suitable than Thread for https://codereview.chromium.org/2197753002/ per it not needing a MessageLoop. Also, cleanup the SimpleThread::Options API, making it more like Thread::Options. BUG=629716, 634835 TBR=avi@chromium.org Committed: https://crrev.com/8b35b0572443f4a84377a1fbd199a1edabe3d7d6 Cr-Commit-Position: refs/heads/master@{#409830} ========== to ========== Add joinable option to SimpleThread::Options as was just done for Thread::Options. SimpleThread will be more suitable than Thread for https://codereview.chromium.org/2197753002/ per it not needing a MessageLoop. Also, cleanup the SimpleThread::Options API, making it more like Thread::Options. BUG=629716, 634835, 629139 TBR=avi@chromium.org Committed: https://crrev.com/8b35b0572443f4a84377a1fbd199a1edabe3d7d6 Cr-Commit-Position: refs/heads/master@{#409830} ==========
Description was changed from ========== Add joinable option to SimpleThread::Options as was just done for Thread::Options. SimpleThread will be more suitable than Thread for https://codereview.chromium.org/2197753002/ per it not needing a MessageLoop. Also, cleanup the SimpleThread::Options API, making it more like Thread::Options. BUG=629716, 634835, 629139 TBR=avi@chromium.org Committed: https://crrev.com/8b35b0572443f4a84377a1fbd199a1edabe3d7d6 Cr-Commit-Position: refs/heads/master@{#409830} ========== to ========== Add joinable option to SimpleThread::Options as was just done for Thread::Options. SimpleThread will be more suitable than Thread for https://codereview.chromium.org/2197753002/ per it not needing a MessageLoop. Also, cleanup the SimpleThread::Options API, making it more like Thread::Options. Non-joinable SimpleThread will require to outlive their Run() methods, except for DelegateSimpleThread which will explicitly support being deleted while Run() is active (its Delegate must however outlive Run()). Note: it might seem overkill to let DelegateSimpleThread be deleted early but this is eventually what I want to do for base::Thread (https://crbug.com/629139#c14) and this is a good precursor to this paradigm (it really takes all its sense on base::Thread where the destructor is more meaningful though -- winds down the MessageLoop). The logic on SimpleThread is fairly simple, the complexity is in the assertions (which were put behind DCHECK_IS_ON() to clearly isolate them) and the tests. BUG=629716, 634835, 629139 TBR=avi@chromium.org Committed: https://crrev.com/8b35b0572443f4a84377a1fbd199a1edabe3d7d6 Cr-Commit-Position: refs/heads/master@{#409830} ==========
Description was changed from ========== Add joinable option to SimpleThread::Options as was just done for Thread::Options. SimpleThread will be more suitable than Thread for https://codereview.chromium.org/2197753002/ per it not needing a MessageLoop. Also, cleanup the SimpleThread::Options API, making it more like Thread::Options. Non-joinable SimpleThread will require to outlive their Run() methods, except for DelegateSimpleThread which will explicitly support being deleted while Run() is active (its Delegate must however outlive Run()). Note: it might seem overkill to let DelegateSimpleThread be deleted early but this is eventually what I want to do for base::Thread (https://crbug.com/629139#c14) and this is a good precursor to this paradigm (it really takes all its sense on base::Thread where the destructor is more meaningful though -- winds down the MessageLoop). The logic on SimpleThread is fairly simple, the complexity is in the assertions (which were put behind DCHECK_IS_ON() to clearly isolate them) and the tests. BUG=629716, 634835, 629139 TBR=avi@chromium.org Committed: https://crrev.com/8b35b0572443f4a84377a1fbd199a1edabe3d7d6 Cr-Commit-Position: refs/heads/master@{#409830} ========== to ========== Add joinable option to SimpleThread::Options as was just done for Thread::Options. SimpleThread will be more suitable than Thread for https://codereview.chromium.org/2197753002/ per it not needing a MessageLoop. Also, cleanup the SimpleThread::Options API, making it more like Thread::Options. Non-joinable SimpleThread will have to remain alive so long as their Run() method uses member state (SimpleThread itself won't after invoking Run()). DelegateSimpleThread provides a safe way to do this by explicitly supporting being deleted while Run() is active but ensuring the Delegate itself outlives Run()). Note: it might seem overkill to let DelegateSimpleThread be deleted early but this is eventually what I want to do for base::Thread (https://crbug.com/629139#c14) and this is a good precursor to this paradigm (it really takes all its sense on base::Thread where the destructor is more meaningful though -- winds down the MessageLoop). The logic on SimpleThread is fairly simple, the complexity is in the assertions (which were put behind DCHECK_IS_ON() to clearly isolate them) and the tests. BUG=629716, 634835, 629139 TBR=avi@chromium.org Committed: https://crrev.com/8b35b0572443f4a84377a1fbd199a1edabe3d7d6 Cr-Commit-Position: refs/heads/master@{#409830} ==========
gab@chromium.org changed reviewers: + jam@chromium.org
Lei PTAnL : issues highlighted in http://crbug.com/634835 when landing patch set 2 have been addressed and now pass all bots (including TSan). +jam PTAL for added friend in thread_restrictions.h (DelegateSimpleThread::Delegate in testing only -- SimpleThread is already an accepted friend so I figured this was okay)
Description was changed from ========== Add joinable option to SimpleThread::Options as was just done for Thread::Options. SimpleThread will be more suitable than Thread for https://codereview.chromium.org/2197753002/ per it not needing a MessageLoop. Also, cleanup the SimpleThread::Options API, making it more like Thread::Options. Non-joinable SimpleThread will have to remain alive so long as their Run() method uses member state (SimpleThread itself won't after invoking Run()). DelegateSimpleThread provides a safe way to do this by explicitly supporting being deleted while Run() is active but ensuring the Delegate itself outlives Run()). Note: it might seem overkill to let DelegateSimpleThread be deleted early but this is eventually what I want to do for base::Thread (https://crbug.com/629139#c14) and this is a good precursor to this paradigm (it really takes all its sense on base::Thread where the destructor is more meaningful though -- winds down the MessageLoop). The logic on SimpleThread is fairly simple, the complexity is in the assertions (which were put behind DCHECK_IS_ON() to clearly isolate them) and the tests. BUG=629716, 634835, 629139 TBR=avi@chromium.org Committed: https://crrev.com/8b35b0572443f4a84377a1fbd199a1edabe3d7d6 Cr-Commit-Position: refs/heads/master@{#409830} ========== to ========== Add joinable option to SimpleThread::Options as was just done for Thread::Options. SimpleThread will be more suitable than Thread for https://codereview.chromium.org/2197753002/ per it not needing a MessageLoop. Also, cleanup the SimpleThread::Options API, making it more like Thread::Options. Non-joinable SimpleThreads will have to remain alive so long as their Run() method uses member state (SimpleThread itself won't after invoking Run()). DelegateSimpleThread provides a safe way to do this by explicitly supporting being deleted while Run() is active but ensuring the Delegate itself outlives Run()). Note: it might seem overkill to let DelegateSimpleThread be deleted early but this is eventually what I want to do for base::Thread (https://crbug.com/629139#c14) and this is a good precursor to this paradigm (it really takes all its sense on base::Thread where the destructor is more meaningful though -- winds down the MessageLoop). The logic on SimpleThread is fairly simple, the complexity is in the assertions (which were put behind DCHECK_IS_ON() to clearly isolate them) and the tests. BUG=629716, 634835, 629139 TBR=avi@chromium.org Committed: https://crrev.com/8b35b0572443f4a84377a1fbd199a1edabe3d7d6 Cr-Commit-Position: refs/heads/master@{#409830} ==========
Description was changed from ========== Add joinable option to SimpleThread::Options as was just done for Thread::Options. SimpleThread will be more suitable than Thread for https://codereview.chromium.org/2197753002/ per it not needing a MessageLoop. Also, cleanup the SimpleThread::Options API, making it more like Thread::Options. Non-joinable SimpleThreads will have to remain alive so long as their Run() method uses member state (SimpleThread itself won't after invoking Run()). DelegateSimpleThread provides a safe way to do this by explicitly supporting being deleted while Run() is active but ensuring the Delegate itself outlives Run()). Note: it might seem overkill to let DelegateSimpleThread be deleted early but this is eventually what I want to do for base::Thread (https://crbug.com/629139#c14) and this is a good precursor to this paradigm (it really takes all its sense on base::Thread where the destructor is more meaningful though -- winds down the MessageLoop). The logic on SimpleThread is fairly simple, the complexity is in the assertions (which were put behind DCHECK_IS_ON() to clearly isolate them) and the tests. BUG=629716, 634835, 629139 TBR=avi@chromium.org Committed: https://crrev.com/8b35b0572443f4a84377a1fbd199a1edabe3d7d6 Cr-Commit-Position: refs/heads/master@{#409830} ========== to ========== Add joinable option to SimpleThread::Options as was just done for Thread::Options. SimpleThread will be more suitable than Thread for https://codereview.chromium.org/2197753002/ per it not needing a MessageLoop. Also, cleanup the SimpleThread::Options API, making it more like Thread::Options. Non-joinable SimpleThreads will have to remain alive so long as their Run() method uses member state (SimpleThread itself won't after invoking Run()). DelegateSimpleThread provides a safe way to do this by explicitly supporting being deleted while Run() is active but ensuring the Delegate itself outlives Run()). Note: it might seem overkill to let DelegateSimpleThread be deleted early but this is eventually what I want to do for base::Thread (https://crbug.com/629139#c14) and this is a good precursor to this paradigm (it really takes all its sense on base::Thread where the destructor is more meaningful though -- winds down the MessageLoop). The logic on SimpleThread is fairly simple, the complexity is in the assertions (which were put behind DCHECK_IS_ON() to clearly isolate them) and the tests. BUG=629716, 634835, 629139 TBR=avi@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_tsan_rel_ng Committed: https://crrev.com/8b35b0572443f4a84377a1fbd199a1edabe3d7d6 Cr-Commit-Position: refs/heads/master@{#409830} Reverted: https://crrev.com/a9094bdb2cda679e8bf3a5b43b27c12efa975d3c Cr-Commit-Position: refs/heads/master@{#410169} ==========
lgtm https://codereview.chromium.org/2204333003/diff/240001/base/threading/thread_... File base/threading/thread_restrictions.h (right): https://codereview.chromium.org/2204333003/diff/240001/base/threading/thread_... base/threading/thread_restrictions.h:12: #if DCHECK_IS_ON() tangent: it's odd that this is DCHECK_IS_ON() instead of DCHECK_IS_ON, why is that?
https://codereview.chromium.org/2204333003/diff/240001/base/threading/thread_... File base/threading/thread_restrictions.h (right): https://codereview.chromium.org/2204333003/diff/240001/base/threading/thread_... base/threading/thread_restrictions.h:12: #if DCHECK_IS_ON() On 2016/08/10 17:34:36, jam wrote: > tangent: it's odd that this is DCHECK_IS_ON() instead of DCHECK_IS_ON, why is > that? nvm i see https://chromium.googlesource.com/chromium/src/+/e649f573a38b00bb20fe09250982... now :)
https://codereview.chromium.org/2204333003/diff/240001/base/threading/simple_... File base/threading/simple_thread.cc (right): https://codereview.chromium.org/2204333003/diff/240001/base/threading/simple_... base/threading/simple_thread.cc:93: active_runners_decremented_.TimedWait(remaining_time); Can we just Wait() instead of doing TimedWait() ? https://codereview.chromium.org/2204333003/diff/240001/base/threading/simple_... File base/threading/simple_thread_unittest.cc (right): https://codereview.chromium.org/2204333003/diff/240001/base/threading/simple_... base/threading/simple_thread_unittest.cc:43: explicit ControlledRunner(WaitableEvent* started, no need for explicit https://codereview.chromium.org/2204333003/diff/240001/base/threading/simple_... base/threading/simple_thread_unittest.cc:54: // copy required member state to avoid user-after-frees. s/user/use/
Reply from phone, will address nits tomorrow https://codereview.chromium.org/2204333003/diff/240001/base/threading/simple_... File base/threading/simple_thread.cc (right): https://codereview.chromium.org/2204333003/diff/240001/base/threading/simple_... base/threading/simple_thread.cc:93: active_runners_decremented_.TimedWait(remaining_time); On 2016/08/10 21:12:38, Lei Zhang (Soon to be OOO) wrote: > Can we just Wait() instead of doing TimedWait() ? That would make the check not as good IMO as it would produce a hang instead of A DCHECK on failure.
On 2016/08/10 22:37:00, gab wrote: > That would make the check not as good IMO as it would produce a hang instead of > A DCHECK on failure. Ah, got it.
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...
gab@chromium.org changed reviewers: + danakj@chromium.org
Nits from Lei addressed but he appears to be OOO now, @danakj to finalize this review, thanks! https://codereview.chromium.org/2204333003/diff/240001/base/threading/simple_... File base/threading/simple_thread_unittest.cc (right): https://codereview.chromium.org/2204333003/diff/240001/base/threading/simple_... base/threading/simple_thread_unittest.cc:43: explicit ControlledRunner(WaitableEvent* started, On 2016/08/10 21:12:38, Lei Zhang (OOO) wrote: > no need for explicit Done. https://codereview.chromium.org/2204333003/diff/240001/base/threading/simple_... base/threading/simple_thread_unittest.cc:54: // copy required member state to avoid user-after-frees. On 2016/08/10 21:12:38, Lei Zhang (OOO) wrote: > s/user/use/ Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2204333003/diff/260001/base/threading/simple_... File base/threading/simple_thread.h (right): https://codereview.chromium.org/2204333003/diff/260001/base/threading/simple_... base/threading/simple_thread.h:147: // Run() is private pure virtual because only RunHelper() should invoke it, but.... you can just upcast it and call Run() directly tho (even accidentally).
https://codereview.chromium.org/2204333003/diff/260001/base/threading/simple_... File base/threading/simple_thread.h (right): https://codereview.chromium.org/2204333003/diff/260001/base/threading/simple_... base/threading/simple_thread.h:147: // Run() is private pure virtual because only RunHelper() should invoke it, On 2016/08/12 18:38:00, danakj wrote: > but.... you can just upcast it and call Run() directly tho (even accidentally). You mean because the impl can override it as public? I guess but someone explicitly calling run on a Delegate would be obviously wrong IMO -- even RunHelper is only intended to be called by DelegateSimpleThread. This is mostly hinting people further to do the right thing. Even if someone checks in something terribly wrong, worst case is just that the DCHECKs are skipped (provided they got everything else right).
https://codereview.chromium.org/2204333003/diff/260001/base/threading/simple_... File base/threading/simple_thread.h (right): https://codereview.chromium.org/2204333003/diff/260001/base/threading/simple_... base/threading/simple_thread.h:147: // Run() is private pure virtual because only RunHelper() should invoke it, On 2016/08/12 18:45:04, gab (OOO Aug 15 - Sep 1) wrote: > On 2016/08/12 18:38:00, danakj wrote: > > but.... you can just upcast it and call Run() directly tho (even > accidentally). > > You mean because the impl can override it as public? I guess but someone > explicitly calling run on a Delegate would be obviously wrong IMO -- even > RunHelper is only intended to be called by DelegateSimpleThread. > > This is mostly hinting people further to do the right thing. Even if someone > checks in something terribly wrong, worst case is just that the DCHECKs are > skipped (provided they got everything else right). I mean like SimpleThread* thread = new DelegateSimpleThread; thread->Run();
On 2016/08/12 18:55:17, danakj wrote: > https://codereview.chromium.org/2204333003/diff/260001/base/threading/simple_... > File base/threading/simple_thread.h (right): > > https://codereview.chromium.org/2204333003/diff/260001/base/threading/simple_... > base/threading/simple_thread.h:147: // Run() is private pure virtual because > only RunHelper() should invoke it, > On 2016/08/12 18:45:04, gab (OOO Aug 15 - Sep 1) wrote: > > On 2016/08/12 18:38:00, danakj wrote: > > > but.... you can just upcast it and call Run() directly tho (even > > accidentally). > > > > You mean because the impl can override it as public? I guess but someone > > explicitly calling run on a Delegate would be obviously wrong IMO -- even > > RunHelper is only intended to be called by DelegateSimpleThread. > > > > This is mostly hinting people further to do the right thing. Even if someone > > checks in something terribly wrong, worst case is just that the DCHECKs are > > skipped (provided they got everything else right). > > I mean like > > SimpleThread* thread = new DelegateSimpleThread; > thread->Run(); And I think that's extremely likely. I'd be very likely myself to read and try to use the SimpleThread API when handed a DelegatedSimpleThread unless I happened to notice compiler error.
https://codereview.chromium.org/2204333003/diff/260001/base/threading/simple_... File base/threading/simple_thread.h (right): https://codereview.chromium.org/2204333003/diff/260001/base/threading/simple_... base/threading/simple_thread.h:147: // Run() is private pure virtual because only RunHelper() should invoke it, On 2016/08/12 18:55:17, danakj wrote: > On 2016/08/12 18:45:04, gab (OOO Aug 15 - Sep 1) wrote: > > On 2016/08/12 18:38:00, danakj wrote: > > > but.... you can just upcast it and call Run() directly tho (even > > accidentally). > > > > You mean because the impl can override it as public? I guess but someone > > explicitly calling run on a Delegate would be obviously wrong IMO -- even > > RunHelper is only intended to be called by DelegateSimpleThread. > > > > This is mostly hinting people further to do the right thing. Even if someone > > checks in something terribly wrong, worst case is just that the DCHECKs are > > skipped (provided they got everything else right). > > I mean like > > SimpleThread* thread = new DelegateSimpleThread; > thread->Run(); This is different. The code this comment is on is Delegate::Run(). DelegateSimpleThread::Run() is on line 171. Calling Run() from the owner is always wrong it's intended to be called by ThreadMain (both for DelegateSimpleThread and SimpleThread), this CL doesn't change that.
LGTM for the non-joinable bits. A little bit meh about the DCHECK stuff, like I guess it's better than not having it, probably? https://codereview.chromium.org/2204333003/diff/260001/base/threading/simple_... File base/threading/simple_thread.cc (right): https://codereview.chromium.org/2204333003/diff/260001/base/threading/simple_... base/threading/simple_thread.cc:98: DCHECK(AtomicRefCountIsZero(&active_runners_)) Hm, I guess this is better than nothing? Idk, it's still racey. You can destroy your delegate during run, but as long as you finish before 200ms this DCHECK won't trip. WTB giving ownership of the delegate to the DelegateSimpleThread. https://codereview.chromium.org/2204333003/diff/260001/base/threading/simple_... base/threading/simple_thread.cc:109: would it help to DCHECK here that ~Delegate hasn't started to run yet? https://codereview.chromium.org/2204333003/diff/260001/base/threading/simple_... File base/threading/simple_thread.h (right): https://codereview.chromium.org/2204333003/diff/260001/base/threading/simple_... base/threading/simple_thread.h:147: // Run() is private pure virtual because only RunHelper() should invoke it, On 2016/08/12 19:05:49, gab (OOO Aug 15 - Sep 1) wrote: > On 2016/08/12 18:55:17, danakj wrote: > > On 2016/08/12 18:45:04, gab (OOO Aug 15 - Sep 1) wrote: > > > On 2016/08/12 18:38:00, danakj wrote: > > > > but.... you can just upcast it and call Run() directly tho (even > > > accidentally). > > > > > > You mean because the impl can override it as public? I guess but someone > > > explicitly calling run on a Delegate would be obviously wrong IMO -- even > > > RunHelper is only intended to be called by DelegateSimpleThread. > > > > > > This is mostly hinting people further to do the right thing. Even if someone > > > checks in something terribly wrong, worst case is just that the DCHECKs are > > > skipped (provided they got everything else right). > > > > I mean like > > > > SimpleThread* thread = new DelegateSimpleThread; > > thread->Run(); > > This is different. The code this comment is on is Delegate::Run(). > DelegateSimpleThread::Run() is on line 171. Calling Run() from the owner is > always wrong it's intended to be called by ThreadMain (both for > DelegateSimpleThread and SimpleThread), this CL doesn't change that. Oooh, nested class, I see. rip reading
https://codereview.chromium.org/2204333003/diff/260001/base/threading/simple_... File base/threading/simple_thread.cc (right): https://codereview.chromium.org/2204333003/diff/260001/base/threading/simple_... base/threading/simple_thread.cc:98: DCHECK(AtomicRefCountIsZero(&active_runners_)) On 2016/08/12 20:49:10, danakj wrote: > Hm, I guess this is better than nothing? Idk, it's still racey. You can destroy > your delegate during run, but as long as you finish before 200ms this DCHECK > won't trip. Right... I can't think of a way around this. My thinking is if something incorrectly deletes the Delegate while it's running it won't typically finish at the same time (and if it's truly wrong -- i.e. if the Run() method uses state from its Delegate after it was deleted -- then it would trip ASan... I guess this being covered by ASan could be an argument about not having the dchecks at all). > > WTB giving ownership of the delegate to the DelegateSimpleThread. The current API allows non-joinable DelegateSimpleThread to be deleted *during* Run() so it's lifetime doesn't let it take Delegate. As described in the CL description, this is the API I eventually think makes sense for base::Thread. https://codereview.chromium.org/2204333003/diff/260001/base/threading/simple_... base/threading/simple_thread.cc:109: On 2016/08/12 20:49:09, danakj wrote: > would it help to DCHECK here that ~Delegate hasn't started to run yet? It's correct for a Delegate to say "DeleteSoon(this)" at the very end of its Run() method, which could technically execute before we make it here. So a hard DCHECK is incorrect for the same reason as the race described on line 80 (this is why this DCHECK is kind of hard and needs to assume "reasonable unwind timing").
Patchset #12 (id:280001) has been deleted
Removed complex DCHECK_IS_ON() logic. As we discussed offline ASAN/TSAN should be able to catch cases where Run() uses its Delegate's member state after it is destroyed.
LGTM https://codereview.chromium.org/2204333003/diff/300001/base/threading/simple_... File base/threading/simple_thread.h (right): https://codereview.chromium.org/2204333003/diff/300001/base/threading/simple_... base/threading/simple_thread.h:138: Delegate() = default; this not needed at all right? https://codereview.chromium.org/2204333003/diff/300001/base/threading/simple_... base/threading/simple_thread.h:143: DISALLOW_COPY_AND_ASSIGN(Delegate); not needed, just an interface
Thanks, done :-) https://codereview.chromium.org/2204333003/diff/300001/base/threading/simple_... File base/threading/simple_thread.h (right): https://codereview.chromium.org/2204333003/diff/300001/base/threading/simple_... base/threading/simple_thread.h:138: Delegate() = default; On 2016/08/12 23:12:25, danakj wrote: > this not needed at all right? Done. https://codereview.chromium.org/2204333003/diff/300001/base/threading/simple_... base/threading/simple_thread.h:143: DISALLOW_COPY_AND_ASSIGN(Delegate); On 2016/08/12 23:12:25, danakj wrote: > not needed, just an interface Done.
The CQ bit was checked by gab@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org, jam@chromium.org, danakj@chromium.org Link to the patchset: https://codereview.chromium.org/2204333003/#ps320001 (title: "simplify Delegate interface further")
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: 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
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org, danakj@chromium.org, jam@chromium.org Link to the patchset: https://codereview.chromium.org/2204333003/#ps340001 (title: "fix compile")
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
Exceeded global retry quota
The CQ bit was checked by gab@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org, danakj@chromium.org, jam@chromium.org Link to the patchset: https://codereview.chromium.org/2204333003/#ps360001 (title: "fix ASan")
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
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org, danakj@chromium.org, jam@chromium.org Link to the patchset: https://codereview.chromium.org/2204333003/#ps380001 (title: "typo")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #15 (id:360001) has been deleted
Patchset #15 (id:380001) has been deleted
Patchset #15 (id:400001) has been deleted
FYI, fixed ASan and TSan: - API has to be that DelegateSimpleThread can be deleted *after Run() was invoked* not *after Start()* as Run() still has to be invoked when the "started event" is fired. - DelegateSimpleThread::Run() must not access its member state after Run() as it's allowed to be deleted during Run(). Swapped logic to use a local Delegate* to invoke Run() -- allowing it to reset delegate_ = nullptr *before* doing so.
Patchset #15 (id:420001) has been deleted
The CQ bit was checked by gab@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org, danakj@chromium.org, jam@chromium.org Link to the patchset: https://codereview.chromium.org/2204333003/#ps440001 (title: "fix TSan and ASan")
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...
Message was sent while issue was closed.
Description was changed from ========== Add joinable option to SimpleThread::Options as was just done for Thread::Options. SimpleThread will be more suitable than Thread for https://codereview.chromium.org/2197753002/ per it not needing a MessageLoop. Also, cleanup the SimpleThread::Options API, making it more like Thread::Options. Non-joinable SimpleThreads will have to remain alive so long as their Run() method uses member state (SimpleThread itself won't after invoking Run()). DelegateSimpleThread provides a safe way to do this by explicitly supporting being deleted while Run() is active but ensuring the Delegate itself outlives Run()). Note: it might seem overkill to let DelegateSimpleThread be deleted early but this is eventually what I want to do for base::Thread (https://crbug.com/629139#c14) and this is a good precursor to this paradigm (it really takes all its sense on base::Thread where the destructor is more meaningful though -- winds down the MessageLoop). The logic on SimpleThread is fairly simple, the complexity is in the assertions (which were put behind DCHECK_IS_ON() to clearly isolate them) and the tests. BUG=629716, 634835, 629139 TBR=avi@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_tsan_rel_ng Committed: https://crrev.com/8b35b0572443f4a84377a1fbd199a1edabe3d7d6 Cr-Commit-Position: refs/heads/master@{#409830} Reverted: https://crrev.com/a9094bdb2cda679e8bf3a5b43b27c12efa975d3c Cr-Commit-Position: refs/heads/master@{#410169} ========== to ========== Add joinable option to SimpleThread::Options as was just done for Thread::Options. SimpleThread will be more suitable than Thread for https://codereview.chromium.org/2197753002/ per it not needing a MessageLoop. Also, cleanup the SimpleThread::Options API, making it more like Thread::Options. Non-joinable SimpleThreads will have to remain alive so long as their Run() method uses member state (SimpleThread itself won't after invoking Run()). DelegateSimpleThread provides a safe way to do this by explicitly supporting being deleted while Run() is active but ensuring the Delegate itself outlives Run()). Note: it might seem overkill to let DelegateSimpleThread be deleted early but this is eventually what I want to do for base::Thread (https://crbug.com/629139#c14) and this is a good precursor to this paradigm (it really takes all its sense on base::Thread where the destructor is more meaningful though -- winds down the MessageLoop). The logic on SimpleThread is fairly simple, the complexity is in the assertions (which were put behind DCHECK_IS_ON() to clearly isolate them) and the tests. BUG=629716, 634835, 629139 TBR=avi@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_tsan_rel_ng Committed: https://crrev.com/8b35b0572443f4a84377a1fbd199a1edabe3d7d6 Cr-Commit-Position: refs/heads/master@{#409830} Reverted: https://crrev.com/a9094bdb2cda679e8bf3a5b43b27c12efa975d3c Cr-Commit-Position: refs/heads/master@{#410169} ==========
Message was sent while issue was closed.
Committed patchset #15 (id:440001)
Message was sent while issue was closed.
Description was changed from ========== Add joinable option to SimpleThread::Options as was just done for Thread::Options. SimpleThread will be more suitable than Thread for https://codereview.chromium.org/2197753002/ per it not needing a MessageLoop. Also, cleanup the SimpleThread::Options API, making it more like Thread::Options. Non-joinable SimpleThreads will have to remain alive so long as their Run() method uses member state (SimpleThread itself won't after invoking Run()). DelegateSimpleThread provides a safe way to do this by explicitly supporting being deleted while Run() is active but ensuring the Delegate itself outlives Run()). Note: it might seem overkill to let DelegateSimpleThread be deleted early but this is eventually what I want to do for base::Thread (https://crbug.com/629139#c14) and this is a good precursor to this paradigm (it really takes all its sense on base::Thread where the destructor is more meaningful though -- winds down the MessageLoop). The logic on SimpleThread is fairly simple, the complexity is in the assertions (which were put behind DCHECK_IS_ON() to clearly isolate them) and the tests. BUG=629716, 634835, 629139 TBR=avi@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_tsan_rel_ng Committed: https://crrev.com/8b35b0572443f4a84377a1fbd199a1edabe3d7d6 Cr-Commit-Position: refs/heads/master@{#409830} Reverted: https://crrev.com/a9094bdb2cda679e8bf3a5b43b27c12efa975d3c Cr-Commit-Position: refs/heads/master@{#410169} ========== to ========== Add joinable option to SimpleThread::Options as was just done for Thread::Options. SimpleThread will be more suitable than Thread for https://codereview.chromium.org/2197753002/ per it not needing a MessageLoop. Also, cleanup the SimpleThread::Options API, making it more like Thread::Options. Non-joinable SimpleThreads will have to remain alive so long as their Run() method uses member state (SimpleThread itself won't after invoking Run()). DelegateSimpleThread provides a safe way to do this by explicitly supporting being deleted while Run() is active but ensuring the Delegate itself outlives Run()). Note: it might seem overkill to let DelegateSimpleThread be deleted early but this is eventually what I want to do for base::Thread (https://crbug.com/629139#c14) and this is a good precursor to this paradigm (it really takes all its sense on base::Thread where the destructor is more meaningful though -- winds down the MessageLoop). The logic on SimpleThread is fairly simple, the complexity is in the assertions (which were put behind DCHECK_IS_ON() to clearly isolate them) and the tests. BUG=629716, 634835, 629139 TBR=avi@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_tsan_rel_ng Committed: https://crrev.com/8b35b0572443f4a84377a1fbd199a1edabe3d7d6 Cr-Commit-Position: refs/heads/master@{#409830} Reverted: https://crrev.com/a9094bdb2cda679e8bf3a5b43b27c12efa975d3c Committed: https://crrev.com/addc8ef1bcae5bff546fba73eca8bea3e640ebcd Cr-Original-Commit-Position: refs/heads/master@{#410169} Cr-Commit-Position: refs/heads/master@{#411897} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/addc8ef1bcae5bff546fba73eca8bea3e640ebcd Cr-Commit-Position: refs/heads/master@{#411897} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
