Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1303443003:
Add helper class GuardedAsyncInvoker to protect against thread dying (Closed)
|
Created:
5 years, 4 months ago by magjed_webrtc Modified:
5 years, 4 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd helper class GuardedAsyncInvoker to protect against thread dying
BUG=webrtc:4909
R=tommi@webrtc.org
Committed: https://chromium.googlesource.com/external/webrtc/+/a1f590f3b6d317047d28b6568d1229973bb93f29
Patch Set 1 #Patch Set 2 : Remove AttachToThread() functionality #
Total comments: 12
Patch Set 3 : tommi@s comments #
Messages
Total messages: 29 (15 generated)
The CQ bit was checked by magjed@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1303443003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1303443003/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_gn_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_gn_rel/builds/5018)
The CQ bit was checked by magjed@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1303443003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1303443003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_gn on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_gn/builds/5066) linux_gn_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_gn_rel/builds/5019)
The CQ bit was checked by magjed@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1303443003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1303443003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_rel/builds/8888)
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #1 (id:1) has been deleted
The CQ bit was checked by magjed@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1303443003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1303443003/80001
magjed@webrtc.org changed reviewers: + tommi@webrtc.org
Please take a look.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
tommi@chromium.org changed reviewers: + tommi@chromium.org
Great stuff. Really like the tests. https://codereview.webrtc.org/1303443003/diff/80001/webrtc/base/asyncinvoker.cc File webrtc/base/asyncinvoker.cc (right): https://codereview.webrtc.org/1303443003/diff/80001/webrtc/base/asyncinvoker.... webrtc/base/asyncinvoker.cc:96: thread_ = nullptr; should we DCHECK that thread_ is not nullptr first? Just to catch a weird state where we'd somehow get more than one notification. https://codereview.webrtc.org/1303443003/diff/80001/webrtc/base/asyncinvoker.h File webrtc/base/asyncinvoker.h (right): https://codereview.webrtc.org/1303443003/diff/80001/webrtc/base/asyncinvoker.... webrtc/base/asyncinvoker.h:152: // be safely ignored. Good comment. Can we add something like "...being destroyed while there are outstanding dangling pointers to it" so that the difference between these two classes is perhaps a little more clear? https://codereview.webrtc.org/1303443003/diff/80001/webrtc/base/asyncinvoker.... webrtc/base/asyncinvoker.h:167: void AsyncInvoke(const FunctorT& functor, uint32 id = 0) { what about making this method return bool and return false in the case where thread_ == nullptr? https://codereview.webrtc.org/1303443003/diff/80001/webrtc/base/asyncinvoker.... webrtc/base/asyncinvoker.h:180: if (thread_ != nullptr) {} (and below) https://codereview.webrtc.org/1303443003/diff/80001/webrtc/base/thread_unitte... File webrtc/base/thread_unittest.cc (right): https://codereview.webrtc.org/1303443003/diff/80001/webrtc/base/thread_unitte... webrtc/base/thread_unittest.cc:538: enum { kWaitTimeout = 1000 }; const static int I think using enums this way is generally discouraged although we still do this in places in webrtc. I think pkasting has been fixing this all over. https://codereview.webrtc.org/1303443003/diff/80001/webrtc/base/thread_unitte... webrtc/base/thread_unittest.cc:540: : int_value_(0), invoke_started_(true, false), expected_thread_(NULL) {} nit: nullptr
Please take another look. https://codereview.webrtc.org/1303443003/diff/80001/webrtc/base/asyncinvoker.cc File webrtc/base/asyncinvoker.cc (right): https://codereview.webrtc.org/1303443003/diff/80001/webrtc/base/asyncinvoker.... webrtc/base/asyncinvoker.cc:96: thread_ = nullptr; On 2015/08/19 20:01:54, tommi wrote: > should we DCHECK that thread_ is not nullptr first? Just to catch a weird state > where we'd somehow get more than one notification. Done. https://codereview.webrtc.org/1303443003/diff/80001/webrtc/base/asyncinvoker.h File webrtc/base/asyncinvoker.h (right): https://codereview.webrtc.org/1303443003/diff/80001/webrtc/base/asyncinvoker.... webrtc/base/asyncinvoker.h:152: // be safely ignored. On 2015/08/19 20:01:54, tommi wrote: > Good comment. Can we add something like "...being destroyed while there are > outstanding dangling pointers to it" so that the difference between these two > classes is perhaps a little more clear? Done. https://codereview.webrtc.org/1303443003/diff/80001/webrtc/base/asyncinvoker.... webrtc/base/asyncinvoker.h:167: void AsyncInvoke(const FunctorT& functor, uint32 id = 0) { On 2015/08/19 20:01:54, tommi wrote: > what about making this method return bool and return false in the case where > thread_ == nullptr? Done. https://codereview.webrtc.org/1303443003/diff/80001/webrtc/base/asyncinvoker.... webrtc/base/asyncinvoker.h:180: if (thread_ != nullptr) On 2015/08/19 20:01:54, tommi wrote: > {} (and below) Acknowledged. https://codereview.webrtc.org/1303443003/diff/80001/webrtc/base/thread_unitte... File webrtc/base/thread_unittest.cc (right): https://codereview.webrtc.org/1303443003/diff/80001/webrtc/base/thread_unitte... webrtc/base/thread_unittest.cc:538: enum { kWaitTimeout = 1000 }; On 2015/08/19 20:01:54, tommi wrote: > const static int > > I think using enums this way is generally discouraged although we still do this > in places in webrtc. I think pkasting has been fixing this all over. Done. https://codereview.webrtc.org/1303443003/diff/80001/webrtc/base/thread_unitte... webrtc/base/thread_unittest.cc:540: : int_value_(0), invoke_started_(true, false), expected_thread_(NULL) {} On 2015/08/19 20:01:54, tommi wrote: > nit: nullptr Done.
lgtm!
The CQ bit was checked by magjed@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1303443003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1303443003/100001
The CQ bit was unchecked by magjed@webrtc.org
Message was sent while issue was closed.
Committed patchset #3 (id:100001) manually as a1f590f3b6d317047d28b6568d1229973bb93f29 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
