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}
diff --git a/webrtc/base/firewallsocketserver.cc b/webrtc/base/firewallsocketserver.cc
index c35a687..6339017 100644
--- a/webrtc/base/firewallsocketserver.cc
+++ b/webrtc/base/firewallsocketserver.cc
@@ -126,13 +126,13 @@
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);
}
- AddRule(allow, p, src, dst);
+ if (d == FD_OUT || d == FD_ANY) {
+ AddRule(allow, p, addr, any);
+ }
}
diff --git a/webrtc/p2p/base/p2ptransportchannel.cc b/webrtc/p2p/base/p2ptransportchannel.cc
index a1f8967..95696e1 100644
--- a/webrtc/p2p/base/p2ptransportchannel.cc
+++ b/webrtc/p2p/base/p2ptransportchannel.cc
@@ -1222,17 +1222,37 @@
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 a014701..11399d0 100644
--- a/webrtc/p2p/base/p2ptransportchannel.h
+++ b/webrtc/p2p/base/p2ptransportchannel.h
@@ -87,7 +87,7 @@
// Note: This is only for testing purpose.
// |ports_| should not be changed from outside.
- const std::vector<PortInterface *>& ports() { return ports_; }
+ const std::vector<PortInterface*>& 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 42ef919..f6beee0 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 @@
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 @@
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<cricket::Port*>(ch->ports()[0]);
+ if (!port) {
+ return nullptr;
+ }
+ return port->GetConnection(rtc::SocketAddress(ip, port_num));
+ }
+
+ private:
+ rtc::scoped_ptr<rtc::PhysicalSocketServer> pss_;
+ rtc::scoped_ptr<rtc::VirtualSocketServer> 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 ee3991d..25b5efd 100644
--- a/webrtc/p2p/base/port.cc
+++ b/webrtc/p2p/base/port.cc
@@ -1240,6 +1240,18 @@
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 @@
// 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 @@
<< ", 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 a6a54e8..a36c9f1 100644
--- a/webrtc/p2p/base/port.h
+++ b/webrtc/p2p/base/port.h
@@ -519,6 +519,7 @@
// 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.
