[StunRequest] Remove Construct and Prepare methods.
Construction in StunRequest and derived classes now happens in
the ctor.
Bug: none
Change-Id: I803f6fd2b6d0ac1d9426304d1e1de10dec855eed
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/264942
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#37126}
diff --git a/p2p/base/stun_port.cc b/p2p/base/stun_port.cc
index e846194..e1a6820 100644
--- a/p2p/base/stun_port.cc
+++ b/p2p/base/stun_port.cc
@@ -40,17 +40,14 @@
StunBindingRequest(UDPPort* port,
const rtc::SocketAddress& addr,
int64_t start_time)
- : StunRequest(port->request_manager()),
+ : StunRequest(port->request_manager(),
+ std::make_unique<StunMessage>(STUN_BINDING_REQUEST)),
port_(port),
server_addr_(addr),
start_time_(start_time) {}
const rtc::SocketAddress& server_addr() const { return server_addr_; }
- void Prepare(StunMessage* message) override {
- message->SetType(STUN_BINDING_REQUEST);
- }
-
void OnResponse(StunMessage* response) override {
const StunAddressAttribute* addr_attr =
response->GetAddress(STUN_ATTR_MAPPED_ADDRESS);
diff --git a/p2p/base/stun_request.cc b/p2p/base/stun_request.cc
index 868db4c..2a1ad65 100644
--- a/p2p/base/stun_request.cc
+++ b/p2p/base/stun_request.cc
@@ -57,7 +57,6 @@
void StunRequestManager::SendDelayed(StunRequest* request, int delay) {
RTC_DCHECK_RUN_ON(thread_);
RTC_DCHECK_EQ(this, request->manager());
- request->Construct();
auto [iter, was_inserted] =
requests_.emplace(request->id(), absl::WrapUnique(request));
RTC_DCHECK(was_inserted);
@@ -212,18 +211,6 @@
manager_.network_thread()->Clear(this);
}
-void StunRequest::Construct() {
- // TODO(tommi): The implementation assumes that Construct() is only called
- // once (see `StunRequestManager::SendDelayed` below). However, these steps to
- // construct a request object are odd to have at this level (virtual method
- // called from the parent class, _after_ construction) and also triggered
- // from a different class... via a "Send" method.
- // Remove `Prepare`, `Construct` and make construction of the message objects
- // separate from the StunRequest and StunRequestManager classes.
- Prepare(msg_.get());
- RTC_DCHECK_NE(msg_->type(), 0);
-}
-
int StunRequest::type() {
RTC_DCHECK(msg_ != NULL);
return msg_->type();
diff --git a/p2p/base/stun_request.h b/p2p/base/stun_request.h
index 94fc98f..c6d1076 100644
--- a/p2p/base/stun_request.h
+++ b/p2p/base/stun_request.h
@@ -114,14 +114,7 @@
protected:
friend class StunRequestManager;
- // Causes our wrapped StunMessage to be Prepared.
- // Only called by StunRequestManager.
- // TODO(tommi): get rid of this (see cc file).
- void Construct();
-
- // Fills in a request object to be sent. Note that request's transaction ID
- // will already be set and cannot be changed.
- virtual void Prepare(StunMessage* message) {}
+ StunMessage* mutable_msg() { return msg_.get(); }
// Called when the message receives a response or times out.
virtual void OnResponse(StunMessage* response) {}
diff --git a/p2p/base/stun_request_unittest.cc b/p2p/base/stun_request_unittest.cc
index 99847f8..6831d9f 100644
--- a/p2p/base/stun_request_unittest.cc
+++ b/p2p/base/stun_request_unittest.cc
@@ -77,16 +77,9 @@
// Forwards results to the test class.
class StunRequestThunker : public StunRequest {
public:
- StunRequestThunker(StunRequestManager& manager,
- StunMessageType message_type,
- StunRequestTest* test)
- : StunRequest(manager, CreateStunMessage(message_type)), test_(test) {
- Construct(); // Triggers a call to `Prepare()` which sets the type.
- }
StunRequestThunker(StunRequestManager& manager, StunRequestTest* test)
- : StunRequest(manager), test_(test) {
- Construct(); // Triggers a call to `Prepare()` which sets the type.
- }
+ : StunRequest(manager, CreateStunMessage(STUN_BINDING_REQUEST)),
+ test_(test) {}
std::unique_ptr<StunMessage> CreateResponseMessage(StunMessageType type) {
return CreateStunMessage(type, msg());
@@ -99,16 +92,12 @@
}
virtual void OnTimeout() { test_->OnTimeout(); }
- virtual void Prepare(StunMessage* message) {
- message->SetType(STUN_BINDING_REQUEST);
- }
-
StunRequestTest* test_;
};
// Test handling of a normal binding response.
TEST_F(StunRequestTest, TestSuccess) {
- auto* request = new StunRequestThunker(manager_, STUN_BINDING_REQUEST, this);
+ auto* request = new StunRequestThunker(manager_, this);
std::unique_ptr<StunMessage> res =
request->CreateResponseMessage(STUN_BINDING_RESPONSE);
manager_.Send(request);
@@ -122,7 +111,7 @@
// Test handling of an error binding response.
TEST_F(StunRequestTest, TestError) {
- auto* request = new StunRequestThunker(manager_, STUN_BINDING_REQUEST, this);
+ auto* request = new StunRequestThunker(manager_, this);
std::unique_ptr<StunMessage> res =
request->CreateResponseMessage(STUN_BINDING_ERROR_RESPONSE);
manager_.Send(request);
@@ -136,7 +125,7 @@
// Test handling of a binding response with the wrong transaction id.
TEST_F(StunRequestTest, TestUnexpected) {
- auto* request = new StunRequestThunker(manager_, STUN_BINDING_REQUEST, this);
+ auto* request = new StunRequestThunker(manager_, this);
std::unique_ptr<StunMessage> res = CreateStunMessage(STUN_BINDING_RESPONSE);
manager_.Send(request);
@@ -151,7 +140,7 @@
// Test that requests are sent at the right times.
TEST_F(StunRequestTest, TestBackoff) {
rtc::ScopedFakeClock fake_clock;
- auto* request = new StunRequestThunker(manager_, STUN_BINDING_REQUEST, this);
+ auto* request = new StunRequestThunker(manager_, this);
std::unique_ptr<StunMessage> res =
request->CreateResponseMessage(STUN_BINDING_RESPONSE);
@@ -176,7 +165,7 @@
// Test that we timeout properly if no response is received.
TEST_F(StunRequestTest, TestTimeout) {
rtc::ScopedFakeClock fake_clock;
- auto* request = new StunRequestThunker(manager_, STUN_BINDING_REQUEST, this);
+ auto* request = new StunRequestThunker(manager_, this);
std::unique_ptr<StunMessage> res =
request->CreateResponseMessage(STUN_BINDING_RESPONSE);
@@ -213,7 +202,7 @@
// which is not recognized, the transaction should be considered a failure and
// the response should be ignored.
TEST_F(StunRequestTest, TestUnrecognizedComprehensionRequiredAttribute) {
- auto* request = new StunRequestThunker(manager_, STUN_BINDING_REQUEST, this);
+ auto* request = new StunRequestThunker(manager_, this);
std::unique_ptr<StunMessage> res =
request->CreateResponseMessage(STUN_BINDING_ERROR_RESPONSE);
diff --git a/p2p/base/turn_port.cc b/p2p/base/turn_port.cc
index 148fc6b..6cb3da4 100644
--- a/p2p/base/turn_port.cc
+++ b/p2p/base/turn_port.cc
@@ -73,7 +73,6 @@
class TurnAllocateRequest : public StunRequest {
public:
explicit TurnAllocateRequest(TurnPort* port);
- void Prepare(StunMessage* message) override;
void OnSent() override;
void OnResponse(StunMessage* response) override;
void OnErrorResponse(StunMessage* response) override;
@@ -90,17 +89,14 @@
class TurnRefreshRequest : public StunRequest {
public:
- explicit TurnRefreshRequest(TurnPort* port);
- void Prepare(StunMessage* message) override;
+ explicit TurnRefreshRequest(TurnPort* port, int lifetime = -1);
void OnSent() override;
void OnResponse(StunMessage* response) override;
void OnErrorResponse(StunMessage* response) override;
void OnTimeout() override;
- void set_lifetime(int lifetime) { lifetime_ = lifetime; }
private:
TurnPort* port_;
- int lifetime_;
};
class TurnCreatePermissionRequest : public StunRequest,
@@ -110,7 +106,6 @@
TurnEntry* entry,
const rtc::SocketAddress& ext_addr,
const std::string& remote_ufrag);
- void Prepare(StunMessage* message) override;
void OnSent() override;
void OnResponse(StunMessage* response) override;
void OnErrorResponse(StunMessage* response) override;
@@ -131,7 +126,6 @@
TurnEntry* entry,
int channel_id,
const rtc::SocketAddress& ext_addr);
- void Prepare(StunMessage* message) override;
void OnSent() override;
void OnResponse(StunMessage* response) override;
void OnErrorResponse(StunMessage* response) override;
@@ -933,8 +927,7 @@
request_manager_.Clear();
// Send refresh with lifetime 0.
- TurnRefreshRequest* req = new TurnRefreshRequest(this);
- req->set_lifetime(0);
+ TurnRefreshRequest* req = new TurnRefreshRequest(this, 0);
SendRequest(req, 0);
state_ = STATE_RECEIVEONLY;
@@ -1376,9 +1369,8 @@
TurnAllocateRequest::TurnAllocateRequest(TurnPort* port)
: StunRequest(port->request_manager(),
std::make_unique<TurnMessage>(TURN_ALLOCATE_REQUEST)),
- port_(port) {}
-
-void TurnAllocateRequest::Prepare(StunMessage* message) {
+ port_(port) {
+ StunMessage* message = mutable_msg();
// Create the request as indicated in RFC 5766, Section 6.1.
RTC_DCHECK_EQ(message->type(), TURN_ALLOCATE_REQUEST);
auto transport_attr =
@@ -1563,19 +1555,17 @@
TurnPort::MSG_TRY_ALTERNATE_SERVER);
}
-TurnRefreshRequest::TurnRefreshRequest(TurnPort* port)
+TurnRefreshRequest::TurnRefreshRequest(TurnPort* port, int lifetime /*= -1*/)
: StunRequest(port->request_manager(),
std::make_unique<TurnMessage>(TURN_REFRESH_REQUEST)),
- port_(port),
- lifetime_(-1) {}
-
-void TurnRefreshRequest::Prepare(StunMessage* message) {
+ port_(port) {
+ StunMessage* message = mutable_msg();
// Create the request as indicated in RFC 5766, Section 7.1.
// No attributes need to be included.
RTC_DCHECK_EQ(message->type(), TURN_REFRESH_REQUEST);
- if (lifetime_ > -1) {
+ if (lifetime > -1) {
message->AddAttribute(
- std::make_unique<StunUInt32Attribute>(STUN_ATTR_LIFETIME, lifetime_));
+ std::make_unique<StunUInt32Attribute>(STUN_ATTR_LIFETIME, lifetime));
}
port_->AddRequestAuthInfo(message);
@@ -1657,9 +1647,7 @@
remote_ufrag_(remote_ufrag) {
entry_->SignalDestroyed.connect(
this, &TurnCreatePermissionRequest::OnEntryDestroyed);
-}
-
-void TurnCreatePermissionRequest::Prepare(StunMessage* message) {
+ StunMessage* message = mutable_msg();
// Create the request as indicated in RFC5766, Section 9.1.
message->SetType(TURN_CREATE_PERMISSION_REQUEST);
message->AddAttribute(std::make_unique<StunXorAddressAttribute>(
@@ -1731,9 +1719,7 @@
ext_addr_(ext_addr) {
entry_->SignalDestroyed.connect(this,
&TurnChannelBindRequest::OnEntryDestroyed);
-}
-
-void TurnChannelBindRequest::Prepare(StunMessage* message) {
+ StunMessage* message = mutable_msg();
// Create the request as indicated in RFC5766, Section 11.1.
RTC_DCHECK_EQ(message->type(), TURN_CHANNEL_BIND_REQUEST);
message->AddAttribute(std::make_unique<StunUInt32Attribute>(
