Fix bugs in collecting STUN candidate stats and configuring STUN
candidate keepalive intervals.
StunStats for a STUN candidate cannot be updated after the initial report
in the stats collector. This is caused by the early return of cached
candidate reports for future queries after the initial report creation.
The STUN keepalive interval cannot be configured for UDPPort because of
incorrect type screening, where only StunPort was supported.
TBR=pthatcher@webrtc.org
Bug: webrtc:8951
Change-Id: I0c9c414f43e6327985be6e541e17b5d6f248a79d
Reviewed-on: https://webrtc-review.googlesource.com/58560
Commit-Queue: Qingsi Wang <qingsi@google.com>
Reviewed-by: Taylor Brandstetter <deadbeef@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#22278}diff --git a/p2p/base/port.h b/p2p/base/port.h
index ee5fa65..e45308d 100644
--- a/p2p/base/port.h
+++ b/p2p/base/port.h
@@ -236,6 +236,11 @@
const std::string& password);
~Port() override;
+ // Note that the port type does NOT uniquely identify different subclasses of
+ // Port. Use the 2-tuple of the port type AND the protocol (GetProtocol()) to
+ // uniquely identify subclasses. Whenever a new subclass of Port introduces a
+ // conflit in the value of the 2-tuple, make sure that the implementation that
+ // relies on this 2-tuple for RTTI is properly changed.
const std::string& Type() const override;
rtc::Network* Network() const override;
diff --git a/p2p/base/port_unittest.cc b/p2p/base/port_unittest.cc
index 438e4e6..8f5867f 100644
--- a/p2p/base/port_unittest.cc
+++ b/p2p/base/port_unittest.cc
@@ -499,7 +499,8 @@
UDPPort* CreateUdpPort(const SocketAddress& addr,
PacketSocketFactory* socket_factory) {
return UDPPort::Create(&main_, socket_factory, MakeNetwork(addr), 0, 0,
- username_, password_, std::string(), true);
+ username_, password_, std::string(), true,
+ rtc::nullopt);
}
TCPPort* CreateTcpPort(const SocketAddress& addr) {
return CreateTcpPort(addr, &socket_factory_);
@@ -514,7 +515,8 @@
ServerAddresses stun_servers;
stun_servers.insert(kStunAddr);
return StunPort::Create(&main_, factory, MakeNetwork(addr), 0, 0, username_,
- password_, stun_servers, std::string());
+ password_, stun_servers, std::string(),
+ rtc::nullopt);
}
Port* CreateRelayPort(const SocketAddress& addr, RelayType rtype,
ProtocolType int_proto, ProtocolType ext_proto) {
diff --git a/p2p/base/stunport.cc b/p2p/base/stunport.cc
index 9871e4f..81702a8 100644
--- a/p2p/base/stunport.cc
+++ b/p2p/base/stunport.cc
@@ -457,17 +457,16 @@
int rtt_ms,
const rtc::SocketAddress& stun_server_addr,
const rtc::SocketAddress& stun_reflected_addr) {
- if (bind_request_succeeded_servers_.find(stun_server_addr) !=
- bind_request_succeeded_servers_.end()) {
- return;
- }
- bind_request_succeeded_servers_.insert(stun_server_addr);
-
RTC_DCHECK(stats_.stun_binding_responses_received <
stats_.stun_binding_requests_sent);
stats_.stun_binding_responses_received++;
stats_.stun_binding_rtt_ms_total += rtt_ms;
stats_.stun_binding_rtt_ms_squared_total += rtt_ms * rtt_ms;
+ if (bind_request_succeeded_servers_.find(stun_server_addr) !=
+ bind_request_succeeded_servers_.end()) {
+ return;
+ }
+ bind_request_succeeded_servers_.insert(stun_server_addr);
// If socket is shared and |stun_reflected_addr| is equal to local socket
// address, or if the same address has been added by another STUN server,
// then discarding the stun address.
@@ -554,9 +553,11 @@
const std::string& username,
const std::string& password,
const ServerAddresses& servers,
- const std::string& origin) {
+ const std::string& origin,
+ rtc::Optional<int> stun_keepalive_interval) {
StunPort* port = new StunPort(thread, factory, network, min_port, max_port,
username, password, servers, origin);
+ port->set_stun_keepalive_delay(stun_keepalive_interval);
if (!port->Init()) {
delete port;
port = NULL;
diff --git a/p2p/base/stunport.h b/p2p/base/stunport.h
index 72afbd3..bdaa31b 100644
--- a/p2p/base/stunport.h
+++ b/p2p/base/stunport.h
@@ -42,9 +42,11 @@
const std::string& username,
const std::string& password,
const std::string& origin,
- bool emit_local_for_anyaddress) {
+ bool emit_local_for_anyaddress,
+ rtc::Optional<int> stun_keepalive_interval) {
UDPPort* port = new UDPPort(thread, factory, network, socket, username,
password, origin, emit_local_for_anyaddress);
+ port->set_stun_keepalive_delay(stun_keepalive_interval);
if (!port->Init()) {
delete port;
port = NULL;
@@ -60,10 +62,12 @@
const std::string& username,
const std::string& password,
const std::string& origin,
- bool emit_local_for_anyaddress) {
+ bool emit_local_for_anyaddress,
+ rtc::Optional<int> stun_keepalive_interval) {
UDPPort* port =
new UDPPort(thread, factory, network, min_port, max_port, username,
password, origin, emit_local_for_anyaddress);
+ port->set_stun_keepalive_delay(stun_keepalive_interval);
if (!port->Init()) {
delete port;
port = NULL;
@@ -262,7 +266,8 @@
const std::string& username,
const std::string& password,
const ServerAddresses& servers,
- const std::string& origin);
+ const std::string& origin,
+ rtc::Optional<int> stun_keepalive_interval);
void PrepareAddress() override;
diff --git a/p2p/base/stunport_unittest.cc b/p2p/base/stunport_unittest.cc
index e9acdb1..ef47db1 100644
--- a/p2p/base/stunport_unittest.cc
+++ b/p2p/base/stunport_unittest.cc
@@ -74,7 +74,7 @@
stun_port_.reset(cricket::StunPort::Create(
rtc::Thread::Current(), &socket_factory_, &network_, 0, 0,
rtc::CreateRandomString(16), rtc::CreateRandomString(22), stun_servers,
- std::string()));
+ std::string(), rtc::nullopt));
stun_port_->set_stun_keepalive_delay(stun_keepalive_delay_);
// If |stun_keepalive_lifetime_| is negative, let the stun port
// choose its lifetime from the network type.
@@ -92,10 +92,9 @@
ASSERT_TRUE(socket_ != NULL);
socket_->SignalReadPacket.connect(this, &StunPortTestBase::OnReadPacket);
stun_port_.reset(cricket::UDPPort::Create(
- rtc::Thread::Current(), &socket_factory_,
- &network_, socket_.get(),
- rtc::CreateRandomString(16), rtc::CreateRandomString(22),
- std::string(), false));
+ rtc::Thread::Current(), &socket_factory_, &network_, socket_.get(),
+ rtc::CreateRandomString(16), rtc::CreateRandomString(22), std::string(),
+ false, rtc::nullopt));
ASSERT_TRUE(stun_port_ != NULL);
ServerAddresses stun_servers;
stun_servers.insert(server_addr);
diff --git a/p2p/base/turnport_unittest.cc b/p2p/base/turnport_unittest.cc
index 0e9d1e2..40092c5 100644
--- a/p2p/base/turnport_unittest.cc
+++ b/p2p/base/turnport_unittest.cc
@@ -321,9 +321,9 @@
void CreateUdpPort() { CreateUdpPort(kLocalAddr2); }
void CreateUdpPort(const SocketAddress& address) {
- udp_port_.reset(UDPPort::Create(&main_, &socket_factory_,
- MakeNetwork(address), 0, 0, kIceUfrag2,
- kIcePwd2, std::string(), false));
+ udp_port_.reset(UDPPort::Create(
+ &main_, &socket_factory_, MakeNetwork(address), 0, 0, kIceUfrag2,
+ kIcePwd2, std::string(), false, rtc::nullopt));
// UDP port will be controlled.
udp_port_->SetIceRole(ICEROLE_CONTROLLED);
udp_port_->SignalPortComplete.connect(
diff --git a/p2p/client/basicportallocator.cc b/p2p/client/basicportallocator.cc
index 712d595..01acf42 100644
--- a/p2p/client/basicportallocator.cc
+++ b/p2p/client/basicportallocator.cc
@@ -433,7 +433,11 @@
const rtc::Optional<int>& stun_keepalive_interval) {
auto ports = ReadyPorts();
for (PortInterface* port : ports) {
- if (port->Type() == STUN_PORT_TYPE) {
+ // The port type and protocol can be used to identify different subclasses
+ // of Port in the current implementation. Note that a TCPPort has the type
+ // LOCAL_PORT_TYPE but uses the protocol PROTO_TCP.
+ if (port->Type() == STUN_PORT_TYPE ||
+ (port->Type() == LOCAL_PORT_TYPE && port->GetProtocol() == PROTO_UDP)) {
static_cast<UDPPort*>(port)->set_stun_keepalive_delay(
stun_keepalive_interval);
}
@@ -1297,13 +1301,15 @@
port = UDPPort::Create(
session_->network_thread(), session_->socket_factory(), network_,
udp_socket_.get(), session_->username(), session_->password(),
- session_->allocator()->origin(), emit_local_candidate_for_anyaddress);
+ session_->allocator()->origin(), emit_local_candidate_for_anyaddress,
+ session_->allocator()->stun_candidate_keepalive_interval());
} else {
port = UDPPort::Create(
session_->network_thread(), session_->socket_factory(), network_,
session_->allocator()->min_port(), session_->allocator()->max_port(),
session_->username(), session_->password(),
- session_->allocator()->origin(), emit_local_candidate_for_anyaddress);
+ session_->allocator()->origin(), emit_local_candidate_for_anyaddress,
+ session_->allocator()->stun_candidate_keepalive_interval());
}
if (port) {
@@ -1366,10 +1372,9 @@
session_->network_thread(), session_->socket_factory(), network_,
session_->allocator()->min_port(), session_->allocator()->max_port(),
session_->username(), session_->password(), config_->StunServers(),
- session_->allocator()->origin());
+ session_->allocator()->origin(),
+ session_->allocator()->stun_candidate_keepalive_interval());
if (port) {
- port->set_stun_keepalive_delay(
- session_->allocator()->stun_candidate_keepalive_interval());
session_->AddAllocatedPort(port, this, true);
// Since StunPort is not created using shared socket, |port| will not be
// added to the dequeue.
diff --git a/p2p/client/basicportallocator_unittest.cc b/p2p/client/basicportallocator_unittest.cc
index d5b5254..df6eb4c 100644
--- a/p2p/client/basicportallocator_unittest.cc
+++ b/p2p/client/basicportallocator_unittest.cc
@@ -14,6 +14,7 @@
#include "p2p/base/basicpacketsocketfactory.h"
#include "p2p/base/p2pconstants.h"
#include "p2p/base/p2ptransportchannel.h"
+#include "p2p/base/stunport.h"
#include "p2p/base/testrelayserver.h"
#include "p2p/base/teststunserver.h"
#include "p2p/base/testturnserver.h"
@@ -97,6 +98,25 @@
// Add some margin of error for slow bots.
static const int kStunTimeoutMs = cricket::STUN_TOTAL_TIMEOUT;
+namespace {
+
+void CheckStunKeepaliveIntervalOfAllReadyPorts(
+ const cricket::PortAllocatorSession* allocator_session,
+ int expected) {
+ auto ready_ports = allocator_session->ReadyPorts();
+ for (const auto* port : ready_ports) {
+ if (port->Type() == cricket::STUN_PORT_TYPE ||
+ (port->Type() == cricket::LOCAL_PORT_TYPE &&
+ port->GetProtocol() == cricket::PROTO_UDP)) {
+ EXPECT_EQ(
+ static_cast<const cricket::UDPPort*>(port)->stun_keepalive_delay(),
+ expected);
+ }
+ }
+}
+
+} // namespace
+
namespace cricket {
// Helper for dumping candidates
@@ -2100,4 +2120,74 @@
}
}
+TEST_F(BasicPortAllocatorTest, SetStunKeepaliveIntervalForPorts) {
+ const int pool_size = 1;
+ const int expected_stun_keepalive_interval = 123;
+ AddInterface(kClientAddr);
+ allocator_->SetConfiguration(allocator_->stun_servers(),
+ allocator_->turn_servers(), pool_size, false,
+ nullptr, expected_stun_keepalive_interval);
+ auto* pooled_session = allocator_->GetPooledSession();
+ ASSERT_NE(nullptr, pooled_session);
+ EXPECT_EQ_SIMULATED_WAIT(true, pooled_session->CandidatesAllocationDone(),
+ kDefaultAllocationTimeout, fake_clock);
+ CheckStunKeepaliveIntervalOfAllReadyPorts(pooled_session,
+ expected_stun_keepalive_interval);
+}
+
+TEST_F(BasicPortAllocatorTest,
+ ChangeStunKeepaliveIntervalForPortsAfterInitialConfig) {
+ const int pool_size = 1;
+ AddInterface(kClientAddr);
+ allocator_->SetConfiguration(allocator_->stun_servers(),
+ allocator_->turn_servers(), pool_size, false,
+ nullptr, 123 /* stun keepalive interval */);
+ auto* pooled_session = allocator_->GetPooledSession();
+ ASSERT_NE(nullptr, pooled_session);
+ EXPECT_EQ_SIMULATED_WAIT(true, pooled_session->CandidatesAllocationDone(),
+ kDefaultAllocationTimeout, fake_clock);
+ const int expected_stun_keepalive_interval = 321;
+ allocator_->SetConfiguration(allocator_->stun_servers(),
+ allocator_->turn_servers(), pool_size, false,
+ nullptr, expected_stun_keepalive_interval);
+ CheckStunKeepaliveIntervalOfAllReadyPorts(pooled_session,
+ expected_stun_keepalive_interval);
+}
+
+TEST_F(BasicPortAllocatorTest,
+ SetStunKeepaliveIntervalForPortsWithSharedSocket) {
+ const int pool_size = 1;
+ const int expected_stun_keepalive_interval = 123;
+ AddInterface(kClientAddr);
+ allocator_->set_flags(allocator().flags() |
+ PORTALLOCATOR_ENABLE_SHARED_SOCKET);
+ allocator_->SetConfiguration(allocator_->stun_servers(),
+ allocator_->turn_servers(), pool_size, false,
+ nullptr, expected_stun_keepalive_interval);
+ EXPECT_TRUE(CreateSession(ICE_CANDIDATE_COMPONENT_RTP));
+ session_->StartGettingPorts();
+ EXPECT_TRUE_SIMULATED_WAIT(candidate_allocation_done_,
+ kDefaultAllocationTimeout, fake_clock);
+ CheckStunKeepaliveIntervalOfAllReadyPorts(session_.get(),
+ expected_stun_keepalive_interval);
+}
+
+TEST_F(BasicPortAllocatorTest,
+ SetStunKeepaliveIntervalForPortsWithoutSharedSocket) {
+ const int pool_size = 1;
+ const int expected_stun_keepalive_interval = 123;
+ AddInterface(kClientAddr);
+ allocator_->set_flags(allocator().flags() &
+ ~(PORTALLOCATOR_ENABLE_SHARED_SOCKET));
+ allocator_->SetConfiguration(allocator_->stun_servers(),
+ allocator_->turn_servers(), pool_size, false,
+ nullptr, expected_stun_keepalive_interval);
+ EXPECT_TRUE(CreateSession(ICE_CANDIDATE_COMPONENT_RTP));
+ session_->StartGettingPorts();
+ EXPECT_TRUE_SIMULATED_WAIT(candidate_allocation_done_,
+ kDefaultAllocationTimeout, fake_clock);
+ CheckStunKeepaliveIntervalOfAllReadyPorts(session_.get(),
+ expected_stun_keepalive_interval);
+}
+
} // namespace cricket
diff --git a/pc/peerconnection_ice_unittest.cc b/pc/peerconnection_ice_unittest.cc
index eaccdaf..9586236 100644
--- a/pc/peerconnection_ice_unittest.cc
+++ b/pc/peerconnection_ice_unittest.cc
@@ -886,4 +886,48 @@
Values(SdpSemantics::kPlanB,
SdpSemantics::kUnifiedPlan));
+class PeerConnectionIceConfigTest : public testing::Test {
+ protected:
+ void SetUp() override {
+ pc_factory_ = CreatePeerConnectionFactory(
+ rtc::Thread::Current(), rtc::Thread::Current(), rtc::Thread::Current(),
+ FakeAudioCaptureModule::Create(), CreateBuiltinAudioEncoderFactory(),
+ CreateBuiltinAudioDecoderFactory(), nullptr, nullptr);
+ }
+ void CreatePeerConnection(const RTCConfiguration& config) {
+ std::unique_ptr<cricket::FakePortAllocator> port_allocator(
+ new cricket::FakePortAllocator(rtc::Thread::Current(), nullptr));
+ port_allocator_ = port_allocator.get();
+ rtc::scoped_refptr<PeerConnectionInterface> pc(
+ pc_factory_->CreatePeerConnection(
+ config, nullptr /* constraint */, std::move(port_allocator),
+ nullptr /* cert_generator */, &observer_));
+ EXPECT_TRUE(pc.get());
+ pc_ = std::move(pc.get());
+ }
+
+ rtc::scoped_refptr<PeerConnectionFactoryInterface> pc_factory_ = nullptr;
+ rtc::scoped_refptr<PeerConnectionInterface> pc_ = nullptr;
+ cricket::FakePortAllocator* port_allocator_ = nullptr;
+
+ MockPeerConnectionObserver observer_;
+};
+
+TEST_F(PeerConnectionIceConfigTest, SetStunCandidateKeepaliveInterval) {
+ RTCConfiguration config;
+ config.stun_candidate_keepalive_interval = 123;
+ config.ice_candidate_pool_size = 1;
+ CreatePeerConnection(config);
+ ASSERT_NE(port_allocator_, nullptr);
+ rtc::Optional<int> actual_stun_keepalive_interval =
+ port_allocator_->stun_candidate_keepalive_interval();
+ EXPECT_EQ(actual_stun_keepalive_interval.value_or(-1), 123);
+ config.stun_candidate_keepalive_interval = 321;
+ RTCError error;
+ pc_->SetConfiguration(config, &error);
+ actual_stun_keepalive_interval =
+ port_allocator_->stun_candidate_keepalive_interval();
+ EXPECT_EQ(actual_stun_keepalive_interval.value_or(-1), 321);
+}
+
} // namespace webrtc
diff --git a/pc/statscollector.cc b/pc/statscollector.cc
index 53d429c..3462b5c 100644
--- a/pc/statscollector.cc
+++ b/pc/statscollector.cc
@@ -719,18 +719,6 @@
if (local) {
report->AddString(StatsReport::kStatsValueNameCandidateNetworkType,
AdapterTypeToStatsType(candidate.network_type()));
- if (candidate_stats.stun_stats.has_value()) {
- const auto& stun_stats = candidate_stats.stun_stats.value();
- report->AddInt64(StatsReport::kStatsValueNameSentStunKeepaliveRequests,
- stun_stats.stun_binding_requests_sent);
- report->AddInt64(StatsReport::kStatsValueNameRecvStunKeepaliveResponses,
- stun_stats.stun_binding_responses_received);
- report->AddFloat(StatsReport::kStatsValueNameStunKeepaliveRttTotal,
- stun_stats.stun_binding_rtt_ms_total);
- report->AddFloat(
- StatsReport::kStatsValueNameStunKeepaliveRttSquaredTotal,
- stun_stats.stun_binding_rtt_ms_squared_total);
- }
}
report->AddString(StatsReport::kStatsValueNameCandidateIPAddress,
candidate.address().ipaddr().ToString());
@@ -744,6 +732,18 @@
candidate.protocol());
}
+ if (local && candidate_stats.stun_stats.has_value()) {
+ const auto& stun_stats = candidate_stats.stun_stats.value();
+ report->AddInt64(StatsReport::kStatsValueNameSentStunKeepaliveRequests,
+ stun_stats.stun_binding_requests_sent);
+ report->AddInt64(StatsReport::kStatsValueNameRecvStunKeepaliveResponses,
+ stun_stats.stun_binding_responses_received);
+ report->AddFloat(StatsReport::kStatsValueNameStunKeepaliveRttTotal,
+ stun_stats.stun_binding_rtt_ms_total);
+ report->AddFloat(StatsReport::kStatsValueNameStunKeepaliveRttSquaredTotal,
+ stun_stats.stun_binding_rtt_ms_squared_total);
+ }
+
return report;
}
