From 1366b0f841c84d842e3ff5ab9050c2b39a96c2f2 Mon Sep 17 00:00:00 2001 From: Markus Handell Date: Wed, 21 Apr 2021 10:22:34 +0200 Subject: [PATCH] AsyncResolver: avoid hanging the WorkerThread. There's a problem where the destruction of the contained rtc::Thread will join the spawned thread blocked on getaddrinfo(). However, getaddrinfo() is sometimes slow and this behavior hinders packet traffic when it happens. Fix this by using the brand new detachable PlatformThread support. Fixed: b:181572711, webrtc:12659 Change-Id: I0b7e0cca3b8b1b3ed22328d940b1bb95cacb5e24 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/214780 Commit-Queue: Markus Handell Reviewed-by: Tommi Cr-Commit-Position: refs/heads/master@{#33804} --- rtc_base/BUILD.gn | 1 + rtc_base/async_resolver.cc | 67 +++++++++++++++++++++++++++++++------- rtc_base/async_resolver.h | 9 +++-- 3 files changed, 62 insertions(+), 15 deletions(-) diff --git a/rtc_base/BUILD.gn b/rtc_base/BUILD.gn index 480b273d61..7ffd9919bc 100644 --- a/rtc_base/BUILD.gn +++ b/rtc_base/BUILD.gn @@ -795,6 +795,7 @@ rtc_library("threading") { ":socket_server", ":timeutils", "../api:function_view", + "../api:refcountedbase", "../api:scoped_refptr", "../api:sequence_checker", "../api/task_queue", diff --git a/rtc_base/async_resolver.cc b/rtc_base/async_resolver.cc index 198b4984e5..9e6a2bae1c 100644 --- a/rtc_base/async_resolver.cc +++ b/rtc_base/async_resolver.cc @@ -10,9 +10,14 @@ #include "rtc_base/async_resolver.h" +#include #include #include +#include "api/ref_counted_base.h" +#include "rtc_base/synchronization/mutex.h" +#include "rtc_base/thread_annotations.h" + #if defined(WEBRTC_WIN) #include #include @@ -30,6 +35,7 @@ #include "api/task_queue/task_queue_base.h" #include "rtc_base/ip_address.h" #include "rtc_base/logging.h" +#include "rtc_base/platform_thread.h" #include "rtc_base/task_queue.h" #include "rtc_base/task_utils/to_queued_task.h" #include "rtc_base/third_party/sigslot/sigslot.h" // for signal_with_thread... @@ -87,30 +93,67 @@ int ResolveHostname(const std::string& hostname, #endif // !__native_client__ } -AsyncResolver::AsyncResolver() : error_(-1) {} +struct AsyncResolver::State : public RefCountedBase { + webrtc::Mutex mutex; + enum class Status { + kLive, + kDead + } status RTC_GUARDED_BY(mutex) = Status::kLive; +}; + +AsyncResolver::AsyncResolver() : error_(-1), state_(new State) {} AsyncResolver::~AsyncResolver() { RTC_DCHECK_RUN_ON(&sequence_checker_); + + // Ensure the thread isn't using a stale reference to the current task queue, + // or calling into ResolveDone post destruction. + webrtc::MutexLock lock(&state_->mutex); + state_->status = State::Status::kDead; +} + +void RunResolution(void* obj) { + std::function* function_ptr = + static_cast*>(obj); + (*function_ptr)(); + delete function_ptr; } void AsyncResolver::Start(const SocketAddress& addr) { RTC_DCHECK_RUN_ON(&sequence_checker_); RTC_DCHECK(!destroy_called_); addr_ = addr; - webrtc::TaskQueueBase* current_task_queue = webrtc::TaskQueueBase::Current(); - popup_thread_ = Thread::Create(); - popup_thread_->Start(); - popup_thread_->PostTask(webrtc::ToQueuedTask( - [this, flag = safety_.flag(), addr, current_task_queue] { + auto thread_function = + [this, addr, caller_task_queue = webrtc::TaskQueueBase::Current(), + state = state_] { std::vector addresses; int error = ResolveHostname(addr.hostname().c_str(), addr.family(), &addresses); - current_task_queue->PostTask(webrtc::ToQueuedTask( - std::move(flag), [this, error, addresses = std::move(addresses)] { - RTC_DCHECK_RUN_ON(&sequence_checker_); - ResolveDone(std::move(addresses), error); - })); - })); + webrtc::MutexLock lock(&state->mutex); + if (state->status == State::Status::kLive) { + caller_task_queue->PostTask(webrtc::ToQueuedTask( + [this, error, addresses = std::move(addresses), state] { + bool live; + { + // ResolveDone can lead to instance destruction, so make sure + // we don't deadlock. + webrtc::MutexLock lock(&state->mutex); + live = state->status == State::Status::kLive; + } + if (live) { + RTC_DCHECK_RUN_ON(&sequence_checker_); + ResolveDone(std::move(addresses), error); + } + })); + } + }; + PlatformThread thread(RunResolution, + new std::function(std::move(thread_function)), + "NameResolution", ThreadAttributes().SetDetached()); + thread.Start(); + // Although |thread| is detached, the PlatformThread contract mandates to call + // Stop() before destruction. The call doesn't actually stop anything. + thread.Stop(); } bool AsyncResolver::GetResolvedAddress(int family, SocketAddress* addr) const { diff --git a/rtc_base/async_resolver.h b/rtc_base/async_resolver.h index c43685a4d8..0c053eed81 100644 --- a/rtc_base/async_resolver.h +++ b/rtc_base/async_resolver.h @@ -17,12 +17,13 @@ #include // NOLINT #endif -#include #include #include "api/sequence_checker.h" #include "rtc_base/async_resolver_interface.h" +#include "rtc_base/event.h" #include "rtc_base/ip_address.h" +#include "rtc_base/ref_counted_object.h" #include "rtc_base/socket_address.h" #include "rtc_base/system/no_unique_address.h" #include "rtc_base/system/rtc_export.h" @@ -52,6 +53,9 @@ class RTC_EXPORT AsyncResolver : public AsyncResolverInterface { const std::vector& addresses() const; private: + // Fwd decl. + struct State; + void ResolveDone(std::vector addresses, int error) RTC_EXCLUSIVE_LOCKS_REQUIRED(sequence_checker_); void MaybeSelfDestruct(); @@ -59,11 +63,10 @@ class RTC_EXPORT AsyncResolver : public AsyncResolverInterface { SocketAddress addr_ RTC_GUARDED_BY(sequence_checker_); std::vector addresses_ RTC_GUARDED_BY(sequence_checker_); int error_ RTC_GUARDED_BY(sequence_checker_); - webrtc::ScopedTaskSafety safety_ RTC_GUARDED_BY(sequence_checker_); - std::unique_ptr popup_thread_ RTC_GUARDED_BY(sequence_checker_); bool recursion_check_ = false; // Protects against SignalDone calling into Destroy. bool destroy_called_ = false; + scoped_refptr state_; RTC_NO_UNIQUE_ADDRESS webrtc::SequenceChecker sequence_checker_; };