Fixing race between ~AsyncInvoker and ~AsyncClosure, using ref-counting.
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}
diff --git a/webrtc/rtc_base/thread_unittest.cc b/webrtc/rtc_base/thread_unittest.cc
index a8c20d1..e7701e8 100644
--- a/webrtc/rtc_base/thread_unittest.cc
+++ b/webrtc/rtc_base/thread_unittest.cc
@@ -458,19 +458,20 @@
thread->Start();
volatile bool invoker_destroyed = false;
{
+ auto functor = [&functor_started, &functor_continue, &functor_finished,
+ &invoker_destroyed] {
+ functor_started.Set();
+ functor_continue.Wait(Event::kForever);
+ rtc::Thread::Current()->SleepMs(kWaitTimeout);
+ EXPECT_FALSE(invoker_destroyed);
+ functor_finished.Set();
+ };
AsyncInvoker invoker;
- invoker.AsyncInvoke<void>(RTC_FROM_HERE, thread.get(),
- [&functor_started, &functor_continue,
- &functor_finished, &invoker_destroyed] {
- functor_started.Set();
- functor_continue.Wait(Event::kForever);
- rtc::Thread::Current()->SleepMs(kWaitTimeout);
- EXPECT_FALSE(invoker_destroyed);
- functor_finished.Set();
- });
+ invoker.AsyncInvoke<void>(RTC_FROM_HERE, thread.get(), functor);
functor_started.Wait(Event::kForever);
- // Allow the functor to continue and immediately destroy the invoker.
+ // Destroy the invoker while the functor is still executing (doing
+ // SleepMs).
functor_continue.Set();
}
@@ -481,6 +482,37 @@
functor_finished.Wait(Event::kForever);
}
+// Variant of the above test where the async-invoked task calls AsyncInvoke
+// *again*, for the thread on which the AsyncInvoker is currently being
+// destroyed. This shouldn't deadlock or crash; this second invocation should
+// just be ignored.
+TEST_F(AsyncInvokeTest, KillInvokerDuringExecuteWithReentrantInvoke) {
+ Event functor_started(false, false);
+ // Flag used to verify that the recursively invoked task never actually runs.
+ bool reentrant_functor_run = false;
+
+ Thread* main = Thread::Current();
+ Thread thread;
+ thread.Start();
+ {
+ AsyncInvoker invoker;
+ auto reentrant_functor = [&reentrant_functor_run] {
+ reentrant_functor_run = true;
+ };
+ auto functor = [&functor_started, &invoker, main, reentrant_functor] {
+ functor_started.Set();
+ Thread::Current()->SleepMs(kWaitTimeout);
+ invoker.AsyncInvoke<void>(RTC_FROM_HERE, main, reentrant_functor);
+ };
+ // This queues a task on |thread| to sleep for |kWaitTimeout| then queue a
+ // task on |main|. But this second queued task should never run, since the
+ // destructor will be entered before it's even invoked.
+ invoker.AsyncInvoke<void>(RTC_FROM_HERE, &thread, functor);
+ functor_started.Wait(Event::kForever);
+ }
+ EXPECT_FALSE(reentrant_functor_run);
+}
+
TEST_F(AsyncInvokeTest, Flush) {
AsyncInvoker invoker;
AtomicBool flag1;