diff --git a/webrtc/base/asyncinvoker-inl.h b/webrtc/base/asyncinvoker-inl.h index a1c2c8e0cf..5f7cd4959a 100644 --- a/webrtc/base/asyncinvoker-inl.h +++ b/webrtc/base/asyncinvoker-inl.h @@ -27,7 +27,7 @@ class AsyncInvoker; // on the calling thread if necessary. class AsyncClosure { public: - explicit AsyncClosure(AsyncInvoker* invoker); + explicit AsyncClosure(AsyncInvoker* invoker) : invoker_(invoker) {} virtual ~AsyncClosure(); // Runs the asynchronous task, and triggers a callback to the calling // thread if needed. Should be called from the target thread. diff --git a/webrtc/base/asyncinvoker.cc b/webrtc/base/asyncinvoker.cc index d2583091cc..bfd13172f7 100644 --- a/webrtc/base/asyncinvoker.cc +++ b/webrtc/base/asyncinvoker.cc @@ -19,7 +19,7 @@ namespace rtc { AsyncInvoker::AsyncInvoker() : invocation_complete_(false, false) {} AsyncInvoker::~AsyncInvoker() { - AtomicOps::Increment(&destroying_); + destroying_ = true; // Messages for this need to be cleared *before* our destructor is complete. MessageQueueManager::Clear(this); // And we need to wait for any invocations that are still in progress on @@ -44,8 +44,7 @@ void AsyncInvoker::OnMessage(Message* msg) { } void AsyncInvoker::Flush(Thread* thread, uint32_t id /*= MQID_ANY*/) { - if (AtomicOps::AcquireLoad(&destroying_)) - return; + if (destroying_) return; // Run this on |thread| to reduce the number of context switches. if (Thread::Current() != thread) { @@ -66,10 +65,11 @@ void AsyncInvoker::DoInvoke(const Location& posted_from, Thread* thread, std::unique_ptr closure, uint32_t id) { - if (AtomicOps::AcquireLoad(&destroying_)) { + if (destroying_) { LOG(LS_WARNING) << "Tried to invoke while destroying the invoker."; return; } + AtomicOps::Increment(&pending_invocations_); thread->Post(posted_from, this, id, new ScopedMessageData(std::move(closure))); } @@ -79,10 +79,11 @@ void AsyncInvoker::DoInvokeDelayed(const Location& posted_from, std::unique_ptr closure, uint32_t delay_ms, uint32_t id) { - if (AtomicOps::AcquireLoad(&destroying_)) { + if (destroying_) { LOG(LS_WARNING) << "Tried to invoke while destroying the invoker."; return; } + AtomicOps::Increment(&pending_invocations_); thread->PostDelayed(posted_from, delay_ms, this, id, new ScopedMessageData(std::move(closure))); } @@ -110,10 +111,6 @@ void GuardedAsyncInvoker::ThreadDestroyed() { thread_ = nullptr; } -AsyncClosure::AsyncClosure(AsyncInvoker* invoker) : invoker_(invoker) { - AtomicOps::Increment(&invoker_->pending_invocations_); -} - AsyncClosure::~AsyncClosure() { AtomicOps::Decrement(&invoker_->pending_invocations_); invoker_->invocation_complete_.Set(); diff --git a/webrtc/base/asyncinvoker.h b/webrtc/base/asyncinvoker.h index 0684f4cfee..5414867418 100644 --- a/webrtc/base/asyncinvoker.h +++ b/webrtc/base/asyncinvoker.h @@ -120,7 +120,7 @@ class AsyncInvoker : public MessageHandler { uint32_t id); volatile int pending_invocations_ = 0; Event invocation_complete_; - int destroying_ = 0; + bool destroying_ = false; friend class AsyncClosure; RTC_DISALLOW_COPY_AND_ASSIGN(AsyncInvoker); diff --git a/webrtc/base/thread_unittest.cc b/webrtc/base/thread_unittest.cc index c8f9e35321..c143120fbc 100644 --- a/webrtc/base/thread_unittest.cc +++ b/webrtc/base/thread_unittest.cc @@ -104,7 +104,7 @@ class MessageClient : public MessageHandler, public TestGenerator { Socket* socket_; }; -class CustomThread : public Thread { +class CustomThread : public rtc::Thread { public: CustomThread() {} virtual ~CustomThread() { Stop(); } @@ -150,7 +150,7 @@ class SignalWhenDestroyedThread : public Thread { // Using std::atomic or std::atomic_flag in C++11 is probably // the right thing to do, but those features are not yet allowed. Or -// AtomicInt, if/when that is added. Since the use isn't +// rtc::AtomicInt, if/when that is added. Since the use isn't // performance critical, use a plain critical section for the time // being. @@ -451,23 +451,27 @@ TEST_F(AsyncInvokeTest, KillInvokerDuringExecute) { // executing, and then to wait for it to finish, ensuring the "EXPECT_FALSE" // is run. Event functor_started(false, false); + Event functor_continue(false, false); Event functor_finished(false, false); Thread thread; thread.Start(); volatile bool invoker_destroyed = false; { - auto functor = [&functor_started, &functor_finished, &invoker_destroyed] { - functor_started.Set(); - Thread::Current()->SleepMs(kWaitTimeout); - EXPECT_FALSE(invoker_destroyed); - functor_finished.Set(); - }; AsyncInvoker invoker; - invoker.AsyncInvoke(RTC_FROM_HERE, &thread, functor); + invoker.AsyncInvoke(RTC_FROM_HERE, &thread, + [&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(); + }); functor_started.Wait(Event::kForever); - // Destroy the invoker while the functor is still executing (doing - // SleepMs). + + // Allow the functor to continue and immediately destroy the invoker. + functor_continue.Set(); } // If the destructor DIDN'T wait for the functor to finish executing, it will @@ -477,35 +481,6 @@ TEST_F(AsyncInvokeTest, KillInvokerDuringExecute) { 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); - 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(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. - invoker.AsyncInvoke(RTC_FROM_HERE, &thread, functor); - functor_started.Wait(Event::kForever); - } - EXPECT_FALSE(reentrant_functor_run); -} - TEST_F(AsyncInvokeTest, Flush) { AsyncInvoker invoker; AtomicBool flag1;