From 1fe120a6b9371819515f2f05beaf62ddcc9c9f30 Mon Sep 17 00:00:00 2001 From: Peter Thatcher Date: Wed, 10 Jun 2015 11:33:17 -0700 Subject: [PATCH] Add triggered checks. BUG=4590 R=guoweis@webrtc.org, juberti@webrtc.org Review URL: https://webrtc-codereview.appspot.com/51979004. Cr-Commit-Position: refs/heads/master@{#9409} --- webrtc/base/firewallsocketserver.cc | 12 +- webrtc/p2p/base/p2ptransportchannel.cc | 38 ++++-- webrtc/p2p/base/p2ptransportchannel.h | 2 +- .../p2p/base/p2ptransportchannel_unittest.cc | 111 +++++++++++++++++- webrtc/p2p/base/port.cc | 26 ++-- webrtc/p2p/base/port.h | 1 + 6 files changed, 162 insertions(+), 28 deletions(-) diff --git a/webrtc/base/firewallsocketserver.cc b/webrtc/base/firewallsocketserver.cc index c35a687fff..6339017e08 100644 --- a/webrtc/base/firewallsocketserver.cc +++ b/webrtc/base/firewallsocketserver.cc @@ -126,13 +126,13 @@ FirewallSocketServer::~FirewallSocketServer() { void FirewallSocketServer::AddRule(bool allow, FirewallProtocol p, FirewallDirection d, const SocketAddress& addr) { - SocketAddress src, dst; - if (d == FD_IN) { - dst = addr; - } else { - src = addr; + SocketAddress any; + if (d == FD_IN || d == FD_ANY) { + AddRule(allow, p, any, addr); + } + if (d == FD_OUT || d == FD_ANY) { + AddRule(allow, p, addr, any); } - AddRule(allow, p, src, dst); } diff --git a/webrtc/p2p/base/p2ptransportchannel.cc b/webrtc/p2p/base/p2ptransportchannel.cc index a1f8967bb5..95696e1e2c 100644 --- a/webrtc/p2p/base/p2ptransportchannel.cc +++ b/webrtc/p2p/base/p2ptransportchannel.cc @@ -1222,17 +1222,37 @@ Connection* P2PTransportChannel::FindNextPingableConnection() { return best_connection_; } - Connection* oldest_conn = NULL; - uint32 oldest_time = 0xFFFFFFFF; - for (uint32 i = 0; i < connections_.size(); ++i) { - if (IsPingable(connections_[i])) { - if (connections_[i]->last_ping_sent() < oldest_time) { - oldest_time = connections_[i]->last_ping_sent(); - oldest_conn = connections_[i]; - } + // First, find "triggered checks". We ping first those connections + // that have received a ping but have not sent a ping since receiving + // it (last_received_ping > last_sent_ping). But we shouldn't do + // triggered checks if the connection is already writable. + Connection* oldest_needing_triggered_check = nullptr; + Connection* oldest = nullptr; + for (Connection* conn : connections_) { + if (!IsPingable(conn)) { + continue; + } + bool needs_triggered_check = + (protocol_type_ == ICEPROTO_RFC5245 && + !conn->writable() && + conn->last_ping_received() > conn->last_ping_sent()); + if (needs_triggered_check && + (!oldest_needing_triggered_check || + (conn->last_ping_received() < + oldest_needing_triggered_check->last_ping_received()))) { + oldest_needing_triggered_check = conn; + } + if (!oldest || (conn->last_ping_sent() < oldest->last_ping_sent())) { + oldest = conn; } } - return oldest_conn; + + if (oldest_needing_triggered_check) { + LOG(LS_INFO) << "Selecting connection for triggered check: " << + oldest_needing_triggered_check->ToString(); + return oldest_needing_triggered_check; + } + return oldest; } // Apart from sending ping from |conn| this method also updates diff --git a/webrtc/p2p/base/p2ptransportchannel.h b/webrtc/p2p/base/p2ptransportchannel.h index a014701f18..11399d0681 100644 --- a/webrtc/p2p/base/p2ptransportchannel.h +++ b/webrtc/p2p/base/p2ptransportchannel.h @@ -87,7 +87,7 @@ class P2PTransportChannel : public TransportChannelImpl, // Note: This is only for testing purpose. // |ports_| should not be changed from outside. - const std::vector& ports() { return ports_; } + const std::vector& ports() { return ports_; } IceMode remote_ice_mode() const { return remote_ice_mode_; } diff --git a/webrtc/p2p/base/p2ptransportchannel_unittest.cc b/webrtc/p2p/base/p2ptransportchannel_unittest.cc index 42ef919927..f6beee07bf 100644 --- a/webrtc/p2p/base/p2ptransportchannel_unittest.cc +++ b/webrtc/p2p/base/p2ptransportchannel_unittest.cc @@ -13,6 +13,7 @@ #include "webrtc/p2p/base/teststunserver.h" #include "webrtc/p2p/base/testturnserver.h" #include "webrtc/p2p/client/basicportallocator.h" +#include "webrtc/p2p/client/fakeportallocator.h" #include "webrtc/base/dscp.h" #include "webrtc/base/fakenetwork.h" #include "webrtc/base/firewallsocketserver.h" @@ -1017,7 +1018,7 @@ const P2PTransportChannelTest::Result* P2PTransportChannelTest::kMatrixSharedSocketAsIce [NUM_CONFIGS][NUM_CONFIGS] = { // OPEN CONE ADDR PORT SYMM 2CON SCON !UDP !TCP HTTP PRXH PRXS -/*OP*/ {LULU, LUSU, LUSU, LUSU, LUPU, LUSU, LUPU, PTLT, LTPT, LSRS, NULL, PTLT}, +/*OP*/ {LULU, LUSU, LUSU, LUSU, LUPU, LUSU, LUPU, PTLT, LTPT, LSRS, NULL, LTPT}, /*CO*/ {LULU, LUSU, LUSU, LUSU, LUPU, LUSU, LUPU, NULL, NULL, LSRS, NULL, LTRT}, /*AD*/ {LULU, LUSU, LUSU, LUSU, LUPU, LUSU, LUPU, NULL, NULL, LSRS, NULL, LTRT}, /*PO*/ {LULU, LUSU, LUSU, LUSU, LURU, LUSU, LURU, NULL, NULL, LSRS, NULL, LTRT}, @@ -1698,3 +1699,111 @@ TEST_F(P2PTransportChannelMultihomedTest, TestDrain) { DestroyChannels(); } + +class P2PTransportChannelPingOrderTest : public testing::Test, + public sigslot::has_slots<> { + public: + P2PTransportChannelPingOrderTest() : + pss_(new rtc::PhysicalSocketServer), + vss_(new rtc::VirtualSocketServer(pss_.get())), + ss_scope_(vss_.get()) { + } + + protected: + void PrepareChannel(cricket::P2PTransportChannel* ch) { + ch->SignalRequestSignaling.connect( + this, &P2PTransportChannelPingOrderTest::OnChannelRequestSignaling); + ch->SetIceProtocolType(cricket::ICEPROTO_RFC5245); + ch->SetIceRole(cricket::ICEROLE_CONTROLLING); + ch->SetIceCredentials(kIceUfrag[0], kIcePwd[0]); + ch->SetRemoteIceCredentials(kIceUfrag[1], kIcePwd[1]); + } + + void OnChannelRequestSignaling(cricket::TransportChannelImpl* channel) { + channel->OnSignalingReady(); + } + + cricket::Candidate CreateCandidate(const std::string& ip, + int port, + int priority) { + cricket::Candidate c; + c.set_address(rtc::SocketAddress(ip, port)); + c.set_component(1); + c.set_protocol(cricket::UDP_PROTOCOL_NAME); + c.set_priority(priority); + return c; + } + + cricket::Connection* WaitForConnectionTo(cricket::P2PTransportChannel* ch, + const std::string& ip, + int port_num) { + EXPECT_TRUE_WAIT(GetConnectionTo(ch, ip, port_num) != nullptr, 3000); + return GetConnectionTo(ch, ip, port_num); + } + + cricket::Connection* GetConnectionTo(cricket::P2PTransportChannel* ch, + const std::string& ip, + int port_num) { + if (ch->ports().empty()) { + return nullptr; + } + cricket::Port* port = static_cast(ch->ports()[0]); + if (!port) { + return nullptr; + } + return port->GetConnection(rtc::SocketAddress(ip, port_num)); + } + + private: + rtc::scoped_ptr pss_; + rtc::scoped_ptr vss_; + rtc::SocketServerScope ss_scope_; +}; + +TEST_F(P2PTransportChannelPingOrderTest, TestTriggeredChecks) { + cricket::FakePortAllocator pa(rtc::Thread::Current(), nullptr); + cricket::P2PTransportChannel ch("trigger checks", 1, nullptr, &pa); + PrepareChannel(&ch); + ch.Connect(); + ch.OnCandidate(CreateCandidate("1.1.1.1", 1, 1)); + ch.OnCandidate(CreateCandidate("2.2.2.2", 2, 2)); + + cricket::Connection* conn1 = WaitForConnectionTo(&ch, "1.1.1.1", 1); + cricket::Connection* conn2 = WaitForConnectionTo(&ch, "2.2.2.2", 2); + ASSERT_TRUE(conn1 != nullptr); + ASSERT_TRUE(conn2 != nullptr); + + // Before a triggered check, the first connection to ping is the + // highest priority one. + EXPECT_EQ(conn2, ch.FindNextPingableConnection()); + + // Receiving a ping causes a triggered check which should make conn1 + // be pinged first instead of conn2, even though conn2 has a higher + // priority. + conn1->ReceivedPing(); + EXPECT_EQ(conn1, ch.FindNextPingableConnection()); +} + +TEST_F(P2PTransportChannelPingOrderTest, TestNoTriggeredChecksWhenWritable) { + cricket::FakePortAllocator pa(rtc::Thread::Current(), nullptr); + cricket::P2PTransportChannel ch("trigger checks", 1, nullptr, &pa); + PrepareChannel(&ch); + ch.Connect(); + ch.OnCandidate(CreateCandidate("1.1.1.1", 1, 1)); + ch.OnCandidate(CreateCandidate("2.2.2.2", 2, 2)); + + cricket::Connection* conn1 = WaitForConnectionTo(&ch, "1.1.1.1", 1); + cricket::Connection* conn2 = WaitForConnectionTo(&ch, "2.2.2.2", 2); + ASSERT_TRUE(conn1 != nullptr); + ASSERT_TRUE(conn2 != nullptr); + + EXPECT_EQ(conn2, ch.FindNextPingableConnection()); + conn1->ReceivedPingResponse(); + ASSERT_TRUE(conn1->writable()); + conn1->ReceivedPing(); + + // Ping received, but the connection is already writable, so no + // "triggered check" and conn2 is pinged before conn1 because it has + // a higher priority. + EXPECT_EQ(conn2, ch.FindNextPingableConnection()); +} diff --git a/webrtc/p2p/base/port.cc b/webrtc/p2p/base/port.cc index ee3991d63f..25b5efd056 100644 --- a/webrtc/p2p/base/port.cc +++ b/webrtc/p2p/base/port.cc @@ -1240,6 +1240,18 @@ void Connection::ReceivedPing() { set_read_state(STATE_READABLE); } +void Connection::ReceivedPingResponse() { + // We've already validated that this is a STUN binding response with + // the correct local and remote username for this connection. + // So if we're not already, become writable. We may be bringing a pruned + // connection back to life, but if we don't really want it, we can always + // prune it again. + set_write_state(STATE_WRITABLE); + set_state(STATE_SUCCEEDED); + pings_since_last_response_.clear(); + last_ping_response_received_ = rtc::Time(); +} + std::string Connection::ToDebugId() const { std::stringstream ss; ss << std::hex << this; @@ -1304,19 +1316,13 @@ void Connection::OnConnectionRequestResponse(ConnectionRequest* request, // connection. rtc::LoggingSeverity sev = !writable() ? rtc::LS_INFO : rtc::LS_VERBOSE; - // We've already validated that this is a STUN binding response with - // the correct local and remote username for this connection. - // So if we're not already, become writable. We may be bringing a pruned - // connection back to life, but if we don't really want it, we can always - // prune it again. uint32 rtt = request->Elapsed(); - set_write_state(STATE_WRITABLE); - set_state(STATE_SUCCEEDED); + ReceivedPingResponse(); if (remote_ice_mode_ == ICEMODE_LITE) { // A ice-lite end point never initiates ping requests. This will allow - // us to move to STATE_READABLE. - ReceivedPing(); + // us to move to STATE_READABLE without an incoming ping request. + set_read_state(STATE_READABLE); } if (LOG_CHECK_LEVEL_V(sev)) { @@ -1332,8 +1338,6 @@ void Connection::OnConnectionRequestResponse(ConnectionRequest* request, << ", pings_since_last_response=" << pings; } - pings_since_last_response_.clear(); - last_ping_response_received_ = rtc::Time(); rtt_ = (RTT_RATIO * rtt_ + rtt) / (RTT_RATIO + 1); // Peer reflexive candidate is only for RFC 5245 ICE. diff --git a/webrtc/p2p/base/port.h b/webrtc/p2p/base/port.h index a6a54e8d26..a36c9f13bd 100644 --- a/webrtc/p2p/base/port.h +++ b/webrtc/p2p/base/port.h @@ -519,6 +519,7 @@ class Connection : public rtc::MessageHandler, // Called when this connection should try checking writability again. uint32 last_ping_sent() const { return last_ping_sent_; } void Ping(uint32 now); + void ReceivedPingResponse(); // Called whenever a valid ping is received on this connection. This is // public because the connection intercepts the first ping for us.