From fbf4ad29584e703dcb906761589910911a454e5c Mon Sep 17 00:00:00 2001 From: Markus Handell Date: Tue, 26 May 2020 18:43:55 +0200 Subject: [PATCH] AsyncResolver: remove dependency on SignalThread. This change removes dependency on SignalThread which is a heavy user of re-entered mutexes, and is a step to being able to delete SignalThread. The new AsyncResolver is based on task queue instead. Bug: webrtc:11567, webrtc:7723 Change-Id: Iab125ccbc0fb9d72af44341e58f89b3868c952cb Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/175910 Commit-Queue: Markus Handell Reviewed-by: Tommi Cr-Commit-Position: refs/heads/master@{#31361} --- rtc_base/BUILD.gn | 3 ++ rtc_base/net_helpers.cc | 62 +++++++++++++++++++++++++++++++++-------- rtc_base/net_helpers.h | 36 ++++++++++++++++-------- 3 files changed, 77 insertions(+), 24 deletions(-) diff --git a/rtc_base/BUILD.gn b/rtc_base/BUILD.gn index 2d90898ce7..fdb622bc32 100644 --- a/rtc_base/BUILD.gn +++ b/rtc_base/BUILD.gn @@ -761,6 +761,7 @@ rtc_library("rtc_base") { deps = [ ":checks", ":deprecation", + ":rtc_task_queue", ":stringutils", "../api:array_view", "../api:function_view", @@ -768,9 +769,11 @@ rtc_library("rtc_base") { "../api/task_queue", "../system_wrappers:field_trial", "network:sent_packet", + "synchronization:sequence_checker", "system:file_wrapper", "system:inline", "system:rtc_export", + "task_utils:pending_task_safety_flag", "task_utils:to_queued_task", "third_party/base64", "third_party/sigslot", diff --git a/rtc_base/net_helpers.cc b/rtc_base/net_helpers.cc index 6ff3791738..c6685e2a65 100644 --- a/rtc_base/net_helpers.cc +++ b/rtc_base/net_helpers.cc @@ -10,8 +10,6 @@ #include "rtc_base/net_helpers.h" -#include - #if defined(WEBRTC_WIN) #include #include @@ -26,8 +24,11 @@ #endif #endif // defined(WEBRTC_POSIX) && !defined(__native_client__) +#include "api/task_queue/task_queue_base.h" #include "rtc_base/logging.h" #include "rtc_base/signal_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... namespace rtc { @@ -83,18 +84,35 @@ int ResolveHostname(const std::string& hostname, #endif // !__native_client__ } -// AsyncResolver -AsyncResolver::AsyncResolver() : SignalThread(), error_(-1) {} +AsyncResolver::AsyncResolver() : error_(-1) {} -AsyncResolver::~AsyncResolver() = default; +AsyncResolver::~AsyncResolver() { + RTC_DCHECK_RUN_ON(&sequence_checker_); +} void AsyncResolver::Start(const SocketAddress& addr) { + RTC_DCHECK_RUN_ON(&sequence_checker_); + RTC_DCHECK(!destroy_called_); addr_ = addr; - // SignalThred Start will kickoff the resolve process. - SignalThread::Start(); + 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] { + 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); + })); + })); } bool AsyncResolver::GetResolvedAddress(int family, SocketAddress* addr) const { + RTC_DCHECK_RUN_ON(&sequence_checker_); + RTC_DCHECK(!destroy_called_); if (error_ != 0 || addresses_.empty()) return false; @@ -109,20 +127,40 @@ bool AsyncResolver::GetResolvedAddress(int family, SocketAddress* addr) const { } int AsyncResolver::GetError() const { + RTC_DCHECK_RUN_ON(&sequence_checker_); + RTC_DCHECK(!destroy_called_); return error_; } void AsyncResolver::Destroy(bool wait) { - SignalThread::Destroy(wait); + // Some callers have trouble guaranteeing that Destroy is called on the + // sequence guarded by |sequence_checker_|. + // RTC_DCHECK_RUN_ON(&sequence_checker_); + RTC_DCHECK(!destroy_called_); + destroy_called_ = true; + MaybeSelfDestruct(); } -void AsyncResolver::DoWork() { - error_ = - ResolveHostname(addr_.hostname().c_str(), addr_.family(), &addresses_); +const std::vector& AsyncResolver::addresses() const { + RTC_DCHECK_RUN_ON(&sequence_checker_); + RTC_DCHECK(!destroy_called_); + return addresses_; } -void AsyncResolver::OnWorkDone() { +void AsyncResolver::ResolveDone(std::vector addresses, int error) { + addresses_ = addresses; + error_ = error; + recursion_check_ = true; SignalDone(this); + MaybeSelfDestruct(); +} + +void AsyncResolver::MaybeSelfDestruct() { + if (!recursion_check_) { + delete this; + } else { + recursion_check_ = false; + } } const char* inet_ntop(int af, const void* src, char* dst, socklen_t size) { diff --git a/rtc_base/net_helpers.h b/rtc_base/net_helpers.h index 1e06940be7..c6aa4be5b2 100644 --- a/rtc_base/net_helpers.h +++ b/rtc_base/net_helpers.h @@ -21,16 +21,23 @@ #include "rtc_base/async_resolver_interface.h" #include "rtc_base/ip_address.h" -#include "rtc_base/signal_thread.h" #include "rtc_base/socket_address.h" +#include "rtc_base/synchronization/sequence_checker.h" #include "rtc_base/system/rtc_export.h" +#include "rtc_base/task_utils/pending_task_safety_flag.h" +#include "rtc_base/thread.h" +#include "rtc_base/thread_annotations.h" namespace rtc { // AsyncResolver will perform async DNS resolution, signaling the result on // the SignalDone from AsyncResolverInterface when the operation completes. -class RTC_EXPORT AsyncResolver : public SignalThread, - public AsyncResolverInterface { +// +// This class is thread-compatible, and all methods and destruction needs to +// happen from the same rtc::Thread, except for Destroy which is allowed to +// happen on another context provided it's not happening concurrently to another +// public API call, and is the last access to the object. +class RTC_EXPORT AsyncResolver : public AsyncResolverInterface { public: AsyncResolver(); ~AsyncResolver() override; @@ -40,17 +47,22 @@ class RTC_EXPORT AsyncResolver : public SignalThread, int GetError() const override; void Destroy(bool wait) override; - const std::vector& addresses() const { return addresses_; } - void set_error(int error) { error_ = error; } - - protected: - void DoWork() override; - void OnWorkDone() override; + const std::vector& addresses() const; private: - SocketAddress addr_; - std::vector addresses_; - int error_; + void ResolveDone(std::vector addresses, int error) + RTC_EXCLUSIVE_LOCKS_REQUIRED(sequence_checker_); + void MaybeSelfDestruct(); + + 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; + webrtc::SequenceChecker sequence_checker_; }; // rtc namespaced wrappers for inet_ntop and inet_pton so we can avoid