From 4ff5443e4e6191f66ca5b8a9181220b50996d2ca Mon Sep 17 00:00:00 2001 From: Qingsi Wang Date: Thu, 1 Mar 2018 18:25:20 -0800 Subject: [PATCH] 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 Reviewed-by: Taylor Brandstetter Cr-Commit-Position: refs/heads/master@{#22278} --- p2p/base/port.h | 5 ++ p2p/base/port_unittest.cc | 6 +- p2p/base/stunport.cc | 15 ++-- p2p/base/stunport.h | 11 ++- p2p/base/stunport_unittest.cc | 9 +-- p2p/base/turnport_unittest.cc | 6 +- p2p/client/basicportallocator.cc | 17 +++-- p2p/client/basicportallocator_unittest.cc | 90 +++++++++++++++++++++++ pc/peerconnection_ice_unittest.cc | 44 +++++++++++ pc/statscollector.cc | 24 +++--- 10 files changed, 189 insertions(+), 38 deletions(-) diff --git a/p2p/base/port.h b/p2p/base/port.h index ee5fa65b69..e45308def5 100644 --- a/p2p/base/port.h +++ b/p2p/base/port.h @@ -236,6 +236,11 @@ class Port : public PortInterface, public rtc::MessageHandler, 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 438e4e6008..8f5867f1c7 100644 --- a/p2p/base/port_unittest.cc +++ b/p2p/base/port_unittest.cc @@ -499,7 +499,8 @@ class PortTest : public testing::Test, public sigslot::has_slots<> { 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 @@ class PortTest : public testing::Test, public sigslot::has_slots<> { 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 9871e4fb61..81702a80aa 100644 --- a/p2p/base/stunport.cc +++ b/p2p/base/stunport.cc @@ -457,17 +457,16 @@ void UDPPort::OnStunBindingRequestSucceeded( 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 @@ StunPort* StunPort::Create(rtc::Thread* thread, const std::string& username, const std::string& password, const ServerAddresses& servers, - const std::string& origin) { + const std::string& origin, + rtc::Optional 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 72afbd3735..bdaa31b7f5 100644 --- a/p2p/base/stunport.h +++ b/p2p/base/stunport.h @@ -42,9 +42,11 @@ class UDPPort : public Port { const std::string& username, const std::string& password, const std::string& origin, - bool emit_local_for_anyaddress) { + bool emit_local_for_anyaddress, + rtc::Optional 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 @@ class UDPPort : public Port { const std::string& username, const std::string& password, const std::string& origin, - bool emit_local_for_anyaddress) { + bool emit_local_for_anyaddress, + rtc::Optional 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 @@ class StunPort : public UDPPort { const std::string& username, const std::string& password, const ServerAddresses& servers, - const std::string& origin); + const std::string& origin, + rtc::Optional stun_keepalive_interval); void PrepareAddress() override; diff --git a/p2p/base/stunport_unittest.cc b/p2p/base/stunport_unittest.cc index e9acdb11a2..ef47db1bcf 100644 --- a/p2p/base/stunport_unittest.cc +++ b/p2p/base/stunport_unittest.cc @@ -74,7 +74,7 @@ class StunPortTestBase : public testing::Test, public sigslot::has_slots<> { 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 @@ class StunPortTestBase : public testing::Test, public sigslot::has_slots<> { 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 0e9d1e2112..40092c5571 100644 --- a/p2p/base/turnport_unittest.cc +++ b/p2p/base/turnport_unittest.cc @@ -321,9 +321,9 @@ class TurnPortTest : public testing::Test, 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 712d5959a4..01acf42cdd 100644 --- a/p2p/client/basicportallocator.cc +++ b/p2p/client/basicportallocator.cc @@ -433,7 +433,11 @@ void BasicPortAllocatorSession::SetStunKeepaliveIntervalForReadyPorts( const rtc::Optional& 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(port)->set_stun_keepalive_delay( stun_keepalive_interval); } @@ -1297,13 +1301,15 @@ void AllocationSequence::CreateUDPPorts() { 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 @@ void AllocationSequence::CreateStunPorts() { 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 d5b5254779..df6eb4cb15 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 @@ static const char kTurnPassword[] = "test"; // 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(port)->stun_keepalive_delay(), + expected); + } + } +} + +} // namespace + namespace cricket { // Helper for dumping candidates @@ -2100,4 +2120,74 @@ TEST_F(BasicPortAllocatorTest, TestSetCandidateFilterAfterCandidatesGathered) { } } +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 eaccdaf54f..9586236743 100644 --- a/pc/peerconnection_ice_unittest.cc +++ b/pc/peerconnection_ice_unittest.cc @@ -886,4 +886,48 @@ INSTANTIATE_TEST_CASE_P(PeerConnectionIceTest, 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 port_allocator( + new cricket::FakePortAllocator(rtc::Thread::Current(), nullptr)); + port_allocator_ = port_allocator.get(); + rtc::scoped_refptr 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 pc_factory_ = nullptr; + rtc::scoped_refptr 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 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 53d429c1ff..3462b5cb8b 100644 --- a/pc/statscollector.cc +++ b/pc/statscollector.cc @@ -719,18 +719,6 @@ StatsReport* StatsCollector::AddCandidateReport( 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 @@ StatsReport* StatsCollector::AddCandidateReport( 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; }