From 3ba809d6a6bc20931adde485bb04586a6556db11 Mon Sep 17 00:00:00 2001 From: Tommi Date: Thu, 14 Dec 2023 19:49:49 +0100 Subject: [PATCH] Reduce locking in DtlsTransport Access to `internal_dtls_transport_` only occurs on the network thread and doesn't require locking. Access to `info_` still requires a lock but writing to it only occurs on the network thread. If reading from the network thread is needed, that could be done without requiring the lock. The scope of holding the lock is much smaller now. Bug: none Change-Id: Ic284df04196dfcf8b77c66a48e484ca6893de050 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/325283 Reviewed-by: Harald Alvestrand Commit-Queue: Tomas Gunnarsson Cr-Commit-Position: refs/heads/main@{#41387} --- pc/dtls_transport.cc | 34 +++++++++++++++++++--------------- pc/dtls_transport.h | 18 +++++++++++++++--- 2 files changed, 34 insertions(+), 18 deletions(-) diff --git a/pc/dtls_transport.cc b/pc/dtls_transport.cc index 15eed9e47b..4888d9f9e7 100644 --- a/pc/dtls_transport.cc +++ b/pc/dtls_transport.cc @@ -41,8 +41,18 @@ DtlsTransport::DtlsTransport( } DtlsTransport::~DtlsTransport() { + // TODO(tommi): Due to a reference being held by the RtpSenderBase + // implementation, the last reference to the `DtlsTransport` instance can + // be released on the signaling thread. + // RTC_DCHECK_RUN_ON(owner_thread_); + // We depend on the signaling thread to call Clear() before dropping // its last reference to this object. + + // If there are non `owner_thread_` references outstanding, and those + // references are the last ones released, we depend on Clear() having been + // called from the owner_thread before the last reference is deleted. + // `Clear()` is currently called from `JsepTransport::~JsepTransport`. RTC_DCHECK(owner_thread_->IsCurrent() || !internal_dtls_transport_); } @@ -72,14 +82,8 @@ void DtlsTransport::Clear() { RTC_DCHECK(internal()); bool must_send_event = (internal()->dtls_state() != DtlsTransportState::kClosed); - // The destructor of cricket::DtlsTransportInternal calls back - // into DtlsTransport, so we can't hold the lock while releasing. - std::unique_ptr transport_to_release; - { - MutexLock lock(&lock_); - transport_to_release = std::move(internal_dtls_transport_); - ice_transport_->Clear(); - } + internal_dtls_transport_.reset(); + ice_transport_->Clear(); UpdateInformation(); if (observer_ && must_send_event) { observer_->OnStateChange(Information()); @@ -100,7 +104,6 @@ void DtlsTransport::OnInternalDtlsState( void DtlsTransport::UpdateInformation() { RTC_DCHECK_RUN_ON(owner_thread_); - MutexLock lock(&lock_); if (internal_dtls_transport_) { if (internal_dtls_transport_->dtls_state() == DtlsTransportState::kConnected) { @@ -125,23 +128,24 @@ void DtlsTransport::UpdateInformation() { success &= internal_dtls_transport_->GetSslCipherSuite(&ssl_cipher_suite); success &= internal_dtls_transport_->GetSrtpCryptoSuite(&srtp_cipher); if (success) { - info_ = DtlsTransportInformation( + set_info(DtlsTransportInformation( internal_dtls_transport_->dtls_state(), role, tls_version, ssl_cipher_suite, srtp_cipher, - internal_dtls_transport_->GetRemoteSSLCertChain()); + internal_dtls_transport_->GetRemoteSSLCertChain())); } else { RTC_LOG(LS_ERROR) << "DtlsTransport in connected state has incomplete " "TLS information"; - info_ = DtlsTransportInformation( + set_info(DtlsTransportInformation( internal_dtls_transport_->dtls_state(), role, absl::nullopt, absl::nullopt, absl::nullopt, - internal_dtls_transport_->GetRemoteSSLCertChain()); + internal_dtls_transport_->GetRemoteSSLCertChain())); } } else { - info_ = DtlsTransportInformation(internal_dtls_transport_->dtls_state()); + set_info( + DtlsTransportInformation(internal_dtls_transport_->dtls_state())); } } else { - info_ = DtlsTransportInformation(DtlsTransportState::kClosed); + set_info(DtlsTransportInformation(DtlsTransportState::kClosed)); } } diff --git a/pc/dtls_transport.h b/pc/dtls_transport.h index cca4cc980a..a1893297e6 100644 --- a/pc/dtls_transport.h +++ b/pc/dtls_transport.h @@ -12,6 +12,7 @@ #define PC_DTLS_TRANSPORT_H_ #include +#include #include "api/dtls_transport_interface.h" #include "api/ice_transport_interface.h" @@ -40,18 +41,22 @@ class DtlsTransport : public DtlsTransportInterface { std::unique_ptr internal); rtc::scoped_refptr ice_transport() override; + + // Currently called from the signaling thread and potentially Chromium's + // JS thread. DtlsTransportInformation Information() override; + void RegisterObserver(DtlsTransportObserverInterface* observer) override; void UnregisterObserver() override; void Clear(); cricket::DtlsTransportInternal* internal() { - MutexLock lock(&lock_); + RTC_DCHECK_RUN_ON(owner_thread_); return internal_dtls_transport_.get(); } const cricket::DtlsTransportInternal* internal() const { - MutexLock lock(&lock_); + RTC_DCHECK_RUN_ON(owner_thread_); return internal_dtls_transport_.get(); } @@ -63,12 +68,19 @@ class DtlsTransport : public DtlsTransportInterface { DtlsTransportState state); void UpdateInformation(); + // Called when changing `info_`. We only change the values from the + // `owner_thread_` (a.k.a. the network thread). + void set_info(DtlsTransportInformation&& info) RTC_RUN_ON(owner_thread_) { + MutexLock lock(&lock_); + info_ = std::move(info); + } + DtlsTransportObserverInterface* observer_ = nullptr; rtc::Thread* owner_thread_; mutable Mutex lock_; DtlsTransportInformation info_ RTC_GUARDED_BY(lock_); std::unique_ptr internal_dtls_transport_ - RTC_GUARDED_BY(lock_); + RTC_GUARDED_BY(owner_thread_); const rtc::scoped_refptr ice_transport_; };