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; }