zqz/platform-external-webrtc
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Fixing race between ~AsyncInvoker and ~AsyncClosure, using ref-counting.

Browse Source 
The AsyncInvoker destructor waits for all invoked tasks to be complete
(in other words, all AsyncClosures to be destructed). They were using an
event to wake up the destructor, but a race made it possible for this
event to be dereferenced after it's destroyed.

This CL makes the event reference counted, such that if the destructor
runs right after AsyncClosure decrements "pending_invocations_",
setting the event will be a no-op, and the event will be destructed
in the AsyncClosure destructor.

This CL also fixes a deadlock that may occur for "re-entrant"
invocations. The deadlock occurs if the AsyncInvoker is destroyed on
thread A while a task on thread B is running, which AsyncInvokes a task
back on thread A.

This was causing pending_invocations_ to end up negative, because
an AsyncClosure that's never added to a thread's message queue (due to
the "destroying_" flag) caused the count to be decremented but not
incremented.

BUG=webrtc:7656

Review-Url: https://codereview.webrtc.org/2885143005
Cr-Commit-Position: refs/heads/master@{#19278}
This commit is contained in:
deadbeef
2017-08-08 17:59:47 -07:00
committed by Commit Bot
parent 42f96d53f3
commit 3af63b0dc9
4 changed files with 134 additions and 35 deletions
Download Patch File Download Diff File Expand all files Collapse all files

50
webrtc/rtc_base/asyncinvoker.h
View File

@ -11,6 +11,7 @@
#ifndef WEBRTC_RTC_BASE_ASYNCINVOKER_H_
#define WEBRTC_RTC_BASE_ASYNCINVOKER_H_
#include <atomic>
#include <memory>
#include <utility>
@ -18,6 +19,8 @@
#include "webrtc/rtc_base/bind.h"
#include "webrtc/rtc_base/constructormagic.h"
#include "webrtc/rtc_base/event.h"
#include "webrtc/rtc_base/refcountedobject.h"
#include "webrtc/rtc_base/scoped_ref_ptr.h"
#include "webrtc/rtc_base/sigslot.h"
#include "webrtc/rtc_base/thread.h"
@ -70,6 +73,20 @@ namespace rtc {
// AsyncInvoker invoker_;
// int result_;
// };
//
// More details about threading:
// - It's safe to construct/destruct AsyncInvoker on different threads.
// - It's safe to call AsyncInvoke from different threads.
// - It's safe to call AsyncInvoke recursively from *within* a functor that's
// being AsyncInvoked.
// - However, it's *not* safe to call AsyncInvoke from *outside* a functor
// that's being AsyncInvoked while the AsyncInvoker is being destroyed on
// another thread. This is just inherently unsafe and there's no way to
// prevent that. So, the user of this class should ensure that the start of
// each "chain" of invocations is synchronized somehow with the AsyncInvoker's
// destruction. This can be done by starting each chain of invocations on the
// same thread on which it will be destroyed, or by using some other
// synchronization method.
class AsyncInvoker : public MessageHandler {
public:
AsyncInvoker();
@ -118,9 +135,28 @@ class AsyncInvoker : public MessageHandler {
std::unique_ptr<AsyncClosure> closure,
uint32_t delay_ms,
uint32_t id);
volatile int pending_invocations_ = 0;
Event invocation_complete_;
bool destroying_ = false;
// Used to keep track of how many invocations (AsyncClosures) are still
// alive, so that the destructor can wait for them to finish, as described in
// the class documentation.
//
// TODO(deadbeef): Using a raw std::atomic like this is prone to error and
// difficult to maintain. We should try to wrap this functionality in a
// separate class to reduce the chance of errors being introduced in the
// future.
std::atomic<int> pending_invocations_;
// Reference counted so that if the AsyncInvoker destructor finishes before
// an AsyncClosure's destructor that's about to call
// "invocation_complete_->Set()", it's not dereferenced after being
// destroyed.
scoped_refptr<RefCountedObject<Event>> invocation_complete_;
// This flag is used to ensure that if an application AsyncInvokes tasks that
// recursively AsyncInvoke other tasks ad infinitum, the cycle eventually
// terminates.
std::atomic<bool> destroying_;
friend class AsyncClosure;
RTC_DISALLOW_COPY_AND_ASSIGN(AsyncInvoker);
@ -149,7 +185,7 @@ class GuardedAsyncInvoker : public sigslot::has_slots<> {
bool AsyncInvoke(const Location& posted_from,
const FunctorT& functor,
uint32_t id = 0) {
rtc::CritScope cs(&crit_);
CritScope cs(&crit_);
if (thread_ == nullptr)
return false;
invoker_.AsyncInvoke<ReturnT, FunctorT>(posted_from, thread_, functor, id);
@ -163,7 +199,7 @@ class GuardedAsyncInvoker : public sigslot::has_slots<> {
const FunctorT& functor,
uint32_t delay_ms,
uint32_t id = 0) {
rtc::CritScope cs(&crit_);
CritScope cs(&crit_);
if (thread_ == nullptr)
return false;
invoker_.AsyncInvokeDelayed<ReturnT, FunctorT>(posted_from, thread_,
@ -180,7 +216,7 @@ class GuardedAsyncInvoker : public sigslot::has_slots<> {
void (HostT::*callback)(ReturnT),
HostT* callback_host,
uint32_t id = 0) {
rtc::CritScope cs(&crit_);
CritScope cs(&crit_);
if (thread_ == nullptr)
return false;
invoker_.AsyncInvoke<ReturnT, FunctorT, HostT>(
@ -198,7 +234,7 @@ class GuardedAsyncInvoker : public sigslot::has_slots<> {
void (HostT::*callback)(),
HostT* callback_host,
uint32_t id = 0) {
rtc::CritScope cs(&crit_);
CritScope cs(&crit_);
if (thread_ == nullptr)
return false;
invoker_.AsyncInvoke<ReturnT, FunctorT, HostT>(