From 49864b7f0d18256311bedc09d94aa96aa72f43be Mon Sep 17 00:00:00 2001 From: Jonas Oreland Date: Fri, 5 Jun 2020 14:55:35 +0200 Subject: [PATCH] Add forget_learned_state to IceControllerInterface This patch enables an IceController to use Connection::ForgetLearnedState by returning it in a SwitchResult, that will cause P2PTransportChannel to call the method. BUG: webrtc:11463 Change-Id: I098bbbd2fb2961822b165770189ac0c2225d1cb0 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/176511 Commit-Queue: Jonas Oreland Reviewed-by: Harald Alvestrand Cr-Commit-Position: refs/heads/master@{#31458} --- p2p/base/ice_controller_interface.h | 19 ++++-- p2p/base/p2p_transport_channel.cc | 23 ++++--- p2p/base/p2p_transport_channel.h | 12 +++- p2p/base/p2p_transport_channel_unittest.cc | 75 ++++++++++++++++++++++ 4 files changed, 116 insertions(+), 13 deletions(-) diff --git a/p2p/base/ice_controller_interface.h b/p2p/base/ice_controller_interface.h index cc4cf4d0d7..d5dc29e782 100644 --- a/p2p/base/ice_controller_interface.h +++ b/p2p/base/ice_controller_interface.h @@ -51,12 +51,20 @@ struct IceControllerEvent { // - which connection to ping // - which connection to use // - which connection to prune +// - which connection to forget learned state on // -// P2PTransportChannel creates a |Connection| and adds a const pointer -// to the IceController using |AddConnection|, i.e the IceController -// should not call any non-const methods on a Connection. +// The P2PTransportChannel owns (creates and destroys) Connections, +// but P2PTransportChannel gives const pointers to the the IceController using +// |AddConnection|, i.e the IceController should not call any non-const methods +// on a Connection but signal back in the interface if any mutable function +// shall be called. // -// The IceController shall keeps track of all connections added +// Current these are limited to: +// Connection::Ping - returned in PingResult +// Connection::Prune - retuned in PruneConnections +// Connection::ForgetLearnedState - return in SwitchResult +// +// The IceController shall keep track of all connections added // (and not destroyed) and give them back using the connections()-function- // // When a Connection gets destroyed @@ -71,6 +79,9 @@ class IceControllerInterface { // An optional recheck event for when a Switch() should be attempted again. absl::optional recheck_event; + + // A vector with connection to run ForgetLearnedState on. + std::vector connections_to_forget_state_on; }; // This represents the result of a call to SelectConnectionToPing. diff --git a/p2p/base/p2p_transport_channel.cc b/p2p/base/p2p_transport_channel.cc index b96857a2ac..6f0df04150 100644 --- a/p2p/base/p2p_transport_channel.cc +++ b/p2p/base/p2p_transport_channel.cc @@ -275,8 +275,7 @@ bool P2PTransportChannel::MaybeSwitchSelectedConnection( if (result.connection.has_value()) { RTC_LOG(LS_INFO) << "Switching selected connection due to: " << reason.ToString(); - SwitchSelectedConnection(const_cast(*result.connection), - reason); + SwitchSelectedConnection(FromIceController(*result.connection), reason); } if (result.recheck_event.has_value()) { @@ -291,6 +290,10 @@ bool P2PTransportChannel::MaybeSwitchSelectedConnection( result.recheck_event->recheck_delay_ms); } + for (const auto* con : result.connections_to_forget_state_on) { + FromIceController(con)->ForgetLearnedState(); + } + return result.connection.has_value(); } @@ -1403,7 +1406,7 @@ bool P2PTransportChannel::CreateConnection(PortInterface* port, return false; } -bool P2PTransportChannel::FindConnection(Connection* connection) const { +bool P2PTransportChannel::FindConnection(const Connection* connection) const { RTC_DCHECK_RUN_ON(network_thread_); return absl::c_linear_search(connections(), connection); } @@ -1709,7 +1712,7 @@ void P2PTransportChannel::PruneConnections() { std::vector connections_to_prune = ice_controller_->PruneConnections(); for (const Connection* conn : connections_to_prune) { - const_cast(conn)->Prune(); + FromIceController(conn)->Prune(); } } @@ -1912,11 +1915,10 @@ void P2PTransportChannel::CheckAndPing() { UpdateConnectionStates(); auto result = ice_controller_->SelectConnectionToPing(last_ping_sent_ms_); - Connection* conn = - const_cast(result.connection.value_or(nullptr)); int delay = result.recheck_delay_ms; - if (conn) { + if (result.connection.value_or(nullptr)) { + Connection* conn = FromIceController(*result.connection); PingConnection(conn); MarkConnectionPinged(conn); } @@ -1929,7 +1931,12 @@ void P2PTransportChannel::CheckAndPing() { // This method is only for unit testing. Connection* P2PTransportChannel::FindNextPingableConnection() { RTC_DCHECK_RUN_ON(network_thread_); - return const_cast(ice_controller_->FindNextPingableConnection()); + auto* conn = ice_controller_->FindNextPingableConnection(); + if (conn) { + return FromIceController(conn); + } else { + return nullptr; + } } // A connection is considered a backup connection if the channel state diff --git a/p2p/base/p2p_transport_channel.h b/p2p/base/p2p_transport_channel.h index 3d6c86f031..4f891beb1e 100644 --- a/p2p/base/p2p_transport_channel.h +++ b/p2p/base/p2p_transport_channel.h @@ -245,7 +245,7 @@ class RTC_EXPORT P2PTransportChannel : public IceTransportInternal { bool CreateConnection(PortInterface* port, const Candidate& remote_candidate, PortInterface* origin_port); - bool FindConnection(Connection* connection) const; + bool FindConnection(const Connection* connection) const; uint32_t GetRemoteCandidateGeneration(const Candidate& candidate); bool IsDuplicateRemoteCandidate(const Candidate& candidate); @@ -348,6 +348,16 @@ class RTC_EXPORT P2PTransportChannel : public IceTransportInternal { // 2. Peer-reflexive remote candidates. Candidate SanitizeRemoteCandidate(const Candidate& c) const; + // Cast a Connection returned from IceController and verify that it exists. + // (P2P owns all Connections, and only gives const pointers to IceController, + // see IceControllerInterface). + Connection* FromIceController(const Connection* conn) { + // Verify that IceController does not return a connection + // that we have destroyed. + RTC_DCHECK(FindConnection(conn)); + return const_cast(conn); + } + std::string transport_name_ RTC_GUARDED_BY(network_thread_); int component_ RTC_GUARDED_BY(network_thread_); PortAllocator* allocator_ RTC_GUARDED_BY(network_thread_); diff --git a/p2p/base/p2p_transport_channel_unittest.cc b/p2p/base/p2p_transport_channel_unittest.cc index 059b13fcc2..eee91465be 100644 --- a/p2p/base/p2p_transport_channel_unittest.cc +++ b/p2p/base/p2p_transport_channel_unittest.cc @@ -5728,6 +5728,81 @@ TEST(P2PTransportChannel, InjectIceController) { /* event_log = */ nullptr, &factory); } +class ForgetLearnedStateController : public cricket::BasicIceController { + public: + explicit ForgetLearnedStateController( + const cricket::IceControllerFactoryArgs& args) + : cricket::BasicIceController(args) {} + + SwitchResult SortAndSwitchConnection(IceControllerEvent reason) override { + auto result = cricket::BasicIceController::SortAndSwitchConnection(reason); + if (forget_connnection_) { + result.connections_to_forget_state_on.push_back(forget_connnection_); + forget_connnection_ = nullptr; + } + result.recheck_event = + IceControllerEvent(IceControllerEvent::ICE_CONTROLLER_RECHECK); + result.recheck_event->recheck_delay_ms = 100; + return result; + } + + void ForgetThisConnectionNextTimeSortAndSwitchConnectionIsCalled( + Connection* con) { + forget_connnection_ = con; + } + + private: + Connection* forget_connnection_ = nullptr; +}; + +class ForgetLearnedStateControllerFactory + : public cricket::IceControllerFactoryInterface { + public: + std::unique_ptr Create( + const cricket::IceControllerFactoryArgs& args) override { + auto controller = std::make_unique(args); + // Keep a pointer to allow modifying calls. + // Must not be used after the p2ptransportchannel has been destructed. + controller_ = controller.get(); + return controller; + } + virtual ~ForgetLearnedStateControllerFactory() = default; + + ForgetLearnedStateController* controller_; +}; + +TEST_F(P2PTransportChannelPingTest, TestForgetLearnedState) { + ForgetLearnedStateControllerFactory factory; + FakePortAllocator pa(rtc::Thread::Current(), nullptr); + P2PTransportChannel ch("ping sufficiently", 1, &pa, nullptr, nullptr, + &factory); + PrepareChannel(&ch); + ch.MaybeStartGathering(); + ch.AddRemoteCandidate(CreateUdpCandidate(LOCAL_PORT_TYPE, "1.1.1.1", 1, 1)); + ch.AddRemoteCandidate(CreateUdpCandidate(LOCAL_PORT_TYPE, "2.2.2.2", 2, 2)); + + Connection* conn1 = WaitForConnectionTo(&ch, "1.1.1.1", 1); + Connection* conn2 = WaitForConnectionTo(&ch, "2.2.2.2", 2); + ASSERT_TRUE(conn1 != nullptr); + ASSERT_TRUE(conn2 != nullptr); + + // Wait for conn1 to be selected. + conn1->ReceivedPingResponse(LOW_RTT, "id"); + EXPECT_EQ_WAIT(conn1, ch.selected_connection(), kMediumTimeout); + + conn2->ReceivedPingResponse(LOW_RTT, "id"); + EXPECT_TRUE(conn2->writable()); + + // Now let the ice controller signal to P2PTransportChannel that it + // should Forget conn2. + factory.controller_ + ->ForgetThisConnectionNextTimeSortAndSwitchConnectionIsCalled(conn2); + + // We don't have a mock Connection, so verify this by checking that it + // is no longer writable. + EXPECT_EQ_WAIT(false, conn2->writable(), kMediumTimeout); +} + TEST_F(P2PTransportChannelTest, DisableDnsLookupsWithTransportPolicyRelay) { ConfigureEndpoints(OPEN, OPEN, kDefaultPortAllocatorFlags, kDefaultPortAllocatorFlags);