From 2545257982a3c27daca1e333f8fdc2e86c486563 Mon Sep 17 00:00:00 2001 From: Tommi Date: Tue, 12 Apr 2022 12:51:40 +0200 Subject: [PATCH] [StunRequestManager] Use unique_ptr in RequestMap This removes the circular delete/remove pattern between the StunRequest and StunRequestManager whereby deleting a request would call the manager from the dtor to do a lookup+erase from the map. Instead the operation of removing from the map, equals delete and is controlled by the manager (owner) class. Bug: webrtc:13892 Change-Id: I1920ac4b98636d38b851f4c2057026bfbd392584 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/258660 Reviewed-by: Danil Chapovalov Commit-Queue: Tomas Gunnarsson Cr-Commit-Position: refs/heads/main@{#36532} --- p2p/base/stun_request.cc | 69 +++++++++++++++------------------------- p2p/base/stun_request.h | 9 +++--- 2 files changed, 29 insertions(+), 49 deletions(-) diff --git a/p2p/base/stun_request.cc b/p2p/base/stun_request.cc index 532e5821a5..0131779755 100644 --- a/p2p/base/stun_request.cc +++ b/p2p/base/stun_request.cc @@ -15,6 +15,7 @@ #include #include +#include "absl/memory/memory.h" #include "rtc_base/checks.h" #include "rtc_base/helpers.h" #include "rtc_base/logging.h" @@ -44,13 +45,7 @@ const int STUN_MAX_RTO = 8000; // milliseconds, or 5 doublings StunRequestManager::StunRequestManager(rtc::Thread* thread) : thread_(thread) {} -StunRequestManager::~StunRequestManager() { - while (requests_.begin() != requests_.end()) { - StunRequest* request = requests_.begin()->second; - requests_.erase(requests_.begin()); - delete request; - } -} +StunRequestManager::~StunRequestManager() = default; void StunRequestManager::Send(StunRequest* request) { SendDelayed(request, 0); @@ -59,31 +54,31 @@ void StunRequestManager::Send(StunRequest* request) { void StunRequestManager::SendDelayed(StunRequest* request, int delay) { RTC_DCHECK_RUN_ON(thread_); RTC_DCHECK_EQ(this, request->manager()); - RTC_DCHECK(requests_.find(request->id()) == requests_.end()); request->Construct(); - requests_[request->id()] = request; + auto [iter, was_inserted] = + requests_.emplace(request->id(), absl::WrapUnique(request)); + RTC_DCHECK(was_inserted); if (delay > 0) { - thread_->PostDelayed(RTC_FROM_HERE, delay, request, MSG_STUN_SEND, NULL); + thread_->PostDelayed(RTC_FROM_HERE, delay, iter->second.get(), + MSG_STUN_SEND, NULL); } else { - thread_->Send(RTC_FROM_HERE, request, MSG_STUN_SEND, NULL); + thread_->Send(RTC_FROM_HERE, iter->second.get(), MSG_STUN_SEND, NULL); } } void StunRequestManager::FlushForTest(int msg_type) { RTC_DCHECK_RUN_ON(thread_); - for (const auto& kv : requests_) { - StunRequest* request = kv.second; + for (const auto& [unused, request] : requests_) { if (msg_type == kAllRequests || msg_type == request->type()) { - thread_->Clear(request, MSG_STUN_SEND); - thread_->Send(RTC_FROM_HERE, request, MSG_STUN_SEND, NULL); + thread_->Clear(request.get(), MSG_STUN_SEND); + thread_->Send(RTC_FROM_HERE, request.get(), MSG_STUN_SEND, NULL); } } } bool StunRequestManager::HasRequestForTest(int msg_type) { RTC_DCHECK_RUN_ON(thread_); - for (const auto& kv : requests_) { - StunRequest* request = kv.second; + for (const auto& [unused, request] : requests_) { if (msg_type == kAllRequests || msg_type == request->type()) { return true; } @@ -91,28 +86,9 @@ bool StunRequestManager::HasRequestForTest(int msg_type) { return false; } -void StunRequestManager::Remove(StunRequest* request) { - RTC_DCHECK_RUN_ON(thread_); - RTC_DCHECK(request->manager() == this); - RequestMap::iterator iter = requests_.find(request->id()); - if (iter != requests_.end()) { - RTC_DCHECK(iter->second == request); - requests_.erase(iter); - thread_->Clear(request); - } -} - void StunRequestManager::Clear() { RTC_DCHECK_RUN_ON(thread_); - std::vector requests; - for (RequestMap::iterator i = requests_.begin(); i != requests_.end(); ++i) - requests.push_back(i->second); - - for (uint32_t i = 0; i < requests.size(); ++i) { - // StunRequest destructor calls Remove() which deletes requests - // from `requests_`. - delete requests[i]; - } + requests_.clear(); } bool StunRequestManager::CheckResponse(StunMessage* msg) { @@ -124,7 +100,7 @@ bool StunRequestManager::CheckResponse(StunMessage* msg) { return false; } - StunRequest* request = iter->second; + StunRequest* request = iter->second.get(); // Now that we know the request, we can see if the response is // integrity-protected or not. @@ -137,14 +113,15 @@ bool StunRequestManager::CheckResponse(StunMessage* msg) { msg->ValidateMessageIntegrity(request->msg()->password()); } + bool success = true; + if (!msg->GetNonComprehendedAttributes().empty()) { // If a response contains unknown comprehension-required attributes, it's // simply discarded and the transaction is considered failed. See RFC5389 // sections 7.3.3 and 7.3.4. RTC_LOG(LS_ERROR) << ": Discarding response due to unknown " "comprehension-required attribute."; - delete request; - return false; + success = false; } else if (msg->type() == GetStunSuccessResponseType(request->type())) { if (!msg->IntegrityOk() && !skip_integrity_checking) { return false; @@ -159,8 +136,8 @@ bool StunRequestManager::CheckResponse(StunMessage* msg) { return false; } - delete request; - return true; + requests_.erase(iter); + return success; } bool StunRequestManager::empty() const { @@ -199,6 +176,11 @@ bool StunRequestManager::CheckResponse(const char* data, size_t size) { return CheckResponse(response.get()); } +void StunRequestManager::OnRequestTimedOut(StunRequest* request) { + RTC_DCHECK_RUN_ON(thread_); + requests_.erase(request->id()); +} + StunRequest::StunRequest(StunRequestManager& manager) : manager_(manager), msg_(new StunMessage()), @@ -219,7 +201,6 @@ StunRequest::StunRequest(StunRequestManager& manager, } StunRequest::~StunRequest() { - manager_.Remove(this); manager_.network_thread()->Clear(this); } @@ -250,7 +231,7 @@ void StunRequest::OnMessage(rtc::Message* pmsg) { if (timeout_) { OnTimeout(); - delete this; + manager_.OnRequestTimedOut(this); return; } diff --git a/p2p/base/stun_request.h b/p2p/base/stun_request.h index 51276023a7..e75804d4b9 100644 --- a/p2p/base/stun_request.h +++ b/p2p/base/stun_request.h @@ -54,10 +54,6 @@ class StunRequestManager { // transmission. For testing only. bool HasRequestForTest(int msg_type); - // Removes a stun request that was added previously. This will happen - // automatically when a request succeeds, fails, or times out. - void Remove(StunRequest* request); - // Removes all stun requests that were added previously. void Clear(); @@ -66,6 +62,9 @@ class StunRequestManager { bool CheckResponse(StunMessage* msg); bool CheckResponse(const char* data, size_t size); + // Called from a StunRequest when a timeout occurs. + void OnRequestTimedOut(StunRequest* request); + bool empty() const; // TODO(tommi): Use TaskQueueBase* instead of rtc::Thread. @@ -75,7 +74,7 @@ class StunRequestManager { sigslot::signal3 SignalSendPacket; private: - typedef std::map RequestMap; + typedef std::map> RequestMap; rtc::Thread* const thread_; RequestMap requests_ RTC_GUARDED_BY(thread_);