Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(195)

Issue 2694723004: Making AsyncInvoker destructor thread-safe. (Closed)

Created:
3 years, 10 months ago by Taylor Brandstetter
Modified:
3 years, 10 months ago
CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com
Target Ref:
refs/heads/master
Project:
webrtc
Visibility:
Public.

Description

Making AsyncInvoker destructor thread-safe. The documentation for AsyncInvoker states that it owns the lifetime of calls, and when its destructor is called, all in-flight calls are cancelled or finish executing. The "cancelled" part is working, but if a call is in the middle of executing, the destructor does *not* wait. This is fixed by keeping a count of pending invocations, which is decremented when a call is either cleared from a message queue or finishes executing. BUG=webrtc:3914, webrtc:3911 Review-Url: https://codereview.webrtc.org/2694723004 Cr-Commit-Position: refs/heads/master@{#16811} Committed: https://chromium.googlesource.com/external/webrtc/+/162cb53e7b6b892ef5201c5ce242520694bab9c1

Patch Set 1 #

Patch Set 2 : Adding back suppressions for races that seem to still exist. #

Patch Set 3 : Fixing deadlock in ThreeThreadsTest #

Patch Set 4 : Fixing a rare potential deadlock. #

Patch Set 5 : Switch to MessageQueueManager::Clear, for when multiple threads are blocked. #

Total comments: 7

Patch Set 6 : Merge with master #

Unified diffs Side-by-side diffs Delta from patch set Stats (+82 lines, -19 lines) Patch
M tools-webrtc/sanitizers/tsan_suppressions_webrtc.cc View 1 1 chunk +0 lines, -4 lines 0 comments Download
M webrtc/base/asyncinvoker.h View 1 2 3 4 5 4 chunks +7 lines, -3 lines 0 comments Download
M webrtc/base/asyncinvoker.cc View 1 2 3 4 5 4 chunks +20 lines, -2 lines 0 comments Download
M webrtc/base/asyncinvoker-inl.h View 1 2 3 4 5 3 chunks +9 lines, -4 lines 0 comments Download
M webrtc/base/event.h View 2 chunks +3 lines, -0 lines 0 comments Download
M webrtc/base/thread_unittest.cc View 1 2 3 chunks +43 lines, -6 lines 0 comments Download

Messages

Total messages: 32 (19 generated)
Taylor Brandstetter
pthatcher: PTAL kjellander: need your lgtm for removed TSan suppressions
3 years, 10 months ago (2017-02-14 04:05:49 UTC) #4
kjellander_webrtc
tools-webrtc/sanitizers/tsan_suppressions_webrtc.cc: lgtm Awesome work fixing this old sourdough (Swedish saying for something that's been around ...
3 years, 10 months ago (2017-02-14 06:40:57 UTC) #11
Taylor Brandstetter
Ping
3 years, 10 months ago (2017-02-16 01:58:23 UTC) #15
pthatcher1
I think tommi should review this code as well. I know he was focused on ...
3 years, 10 months ago (2017-02-17 18:19:38 UTC) #17
Taylor Brandstetter
https://codereview.webrtc.org/2694723004/diff/80001/webrtc/base/asyncinvoker.cc File webrtc/base/asyncinvoker.cc (right): https://codereview.webrtc.org/2694723004/diff/80001/webrtc/base/asyncinvoker.cc#newcode90 webrtc/base/asyncinvoker.cc:90: AtomicOps::Increment(&pending_invocations_); On 2017/02/17 18:19:38, pthatcher1 wrote: > Do we ...
3 years, 10 months ago (2017-02-17 19:00:46 UTC) #18
Taylor Brandstetter
tommi: Can you review or suggest another reviewer?
3 years, 10 months ago (2017-02-22 01:12:44 UTC) #19
tommi
lgtm - one question below as I think we might still have a slim chance ...
3 years, 10 months ago (2017-02-22 11:19:53 UTC) #20
Taylor Brandstetter
https://codereview.webrtc.org/2694723004/diff/80001/webrtc/base/asyncinvoker.cc File webrtc/base/asyncinvoker.cc (right): https://codereview.webrtc.org/2694723004/diff/80001/webrtc/base/asyncinvoker.cc#newcode28 webrtc/base/asyncinvoker.cc:28: while (AtomicOps::AcquireLoad(&pending_invocations_)) { On 2017/02/22 11:19:53, tommi (webrtc) wrote: ...
3 years, 10 months ago (2017-02-23 01:19:00 UTC) #21
tommi
On 2017/02/23 01:19:00, Taylor Brandstetter wrote: > https://codereview.webrtc.org/2694723004/diff/80001/webrtc/base/asyncinvoker.cc > File webrtc/base/asyncinvoker.cc (right): > > https://codereview.webrtc.org/2694723004/diff/80001/webrtc/base/asyncinvoker.cc#newcode28 ...
3 years, 10 months ago (2017-02-23 09:03:40 UTC) #22
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2694723004/100001
3 years, 10 months ago (2017-02-23 23:10:42 UTC) #25
commit-bot: I haz the power
Try jobs failed on following builders: linux_tsan2 on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_tsan2/builds/20154)
3 years, 10 months ago (2017-02-23 23:29:25 UTC) #27
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2694723004/100001
3 years, 10 months ago (2017-02-24 00:54:15 UTC) #29
commit-bot: I haz the power
3 years, 10 months ago (2017-02-24 01:10:12 UTC) #32
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/external/webrtc/+/162cb53e7b6b892ef5201c5ce...

Powered by Google App Engine
This is Rietveld 408576698