Delete a connection only if it has timed out on writing and not receiving for 10 seconds.
BUG=webrtc:5034,webrtc:4937
R=pthatcher@webrtc.org
Review URL: https://codereview.webrtc.org/1371623003 .
Cr-Commit-Position: refs/heads/master@{#10119}
diff --git a/webrtc/p2p/base/p2ptransportchannel.cc b/webrtc/p2p/base/p2ptransportchannel.cc
index cce20f4..1b7cb58 100644
--- a/webrtc/p2p/base/p2ptransportchannel.cc
+++ b/webrtc/p2p/base/p2ptransportchannel.cc
@@ -286,23 +286,26 @@
tiebreaker_ = tiebreaker;
}
-// Currently a channel is considered ICE completed once there is no
-// more than one connection per Network. This works for a single NIC
-// with both IPv4 and IPv6 enabled. However, this condition won't
-// happen when there are multiple NICs and all of them have
-// connectivity.
-// TODO(guoweis): Change Completion to be driven by a channel level
-// timer.
+// A channel is considered ICE completed once there is at most one active
+// connection per network and at least one active connection.
TransportChannelState P2PTransportChannel::GetState() const {
- std::set<rtc::Network*> networks;
-
- if (connections_.empty()) {
- return had_connection_ ? TransportChannelState::STATE_FAILED
- : TransportChannelState::STATE_INIT;
+ if (!had_connection_) {
+ return TransportChannelState::STATE_INIT;
}
- for (uint32 i = 0; i < connections_.size(); ++i) {
- rtc::Network* network = connections_[i]->port()->Network();
+ std::vector<Connection*> active_connections;
+ for (Connection* connection : connections_) {
+ if (connection->active()) {
+ active_connections.push_back(connection);
+ }
+ }
+ if (active_connections.empty()) {
+ return TransportChannelState::STATE_FAILED;
+ }
+
+ std::set<rtc::Network*> networks;
+ for (Connection* connection : active_connections) {
+ rtc::Network* network = connection->port()->Network();
if (networks.find(network) == networks.end()) {
networks.insert(network);
} else {
@@ -312,8 +315,8 @@
return TransportChannelState::STATE_CONNECTING;
}
}
- LOG_J(LS_VERBOSE, this) << "Ice is completed for this channel.";
+ LOG_J(LS_VERBOSE, this) << "Ice is completed for this channel.";
return TransportChannelState::STATE_COMPLETED;
}
@@ -872,8 +875,7 @@
infos->clear();
std::vector<Connection *>::const_iterator it;
- for (it = connections_.begin(); it != connections_.end(); ++it) {
- Connection* connection = *it;
+ for (Connection* connection : connections_) {
ConnectionInfo info;
info.best_connection = (best_connection_ == connection);
info.receiving = connection->receiving();
@@ -1016,7 +1018,7 @@
Connection* premier = GetBestConnectionOnNetwork(network);
// Do not prune connections if the current best connection is weak on this
// network. Otherwise, it may delete connections prematurely.
- if (!premier || premier->Weak()) {
+ if (!premier || premier->weak()) {
continue;
}
@@ -1105,8 +1107,8 @@
HandleNotWritable();
}
-bool P2PTransportChannel::Weak() const {
- return !best_connection_ || best_connection_->Weak();
+bool P2PTransportChannel::weak() const {
+ return !best_connection_ || best_connection_->weak();
}
// If we have a best connection, return it, otherwise return top one in the
@@ -1154,7 +1156,7 @@
UpdateConnectionStates();
// When the best connection is either not receiving or not writable,
// switch to weak ping delay.
- int ping_delay = Weak() ? WEAK_PING_DELAY : STRONG_PING_DELAY;
+ int ping_delay = weak() ? WEAK_PING_DELAY : STRONG_PING_DELAY;
if (rtc::Time() >= last_ping_sent_ms_ + ping_delay) {
Connection* conn = FindNextPingableConnection();
if (conn) {
@@ -1186,7 +1188,7 @@
// If the channel is weak, ping all candidates. Otherwise, we only
// want to ping connections that have not timed out on writing.
- return Weak() || conn->write_state() != Connection::STATE_WRITE_TIMEOUT;
+ return weak() || conn->write_state() != Connection::STATE_WRITE_TIMEOUT;
}
// Returns the next pingable connection to ping. This will be the oldest
diff --git a/webrtc/p2p/base/p2ptransportchannel.h b/webrtc/p2p/base/p2ptransportchannel.h
index 3f7b696..b3e7bc1 100644
--- a/webrtc/p2p/base/p2ptransportchannel.h
+++ b/webrtc/p2p/base/p2ptransportchannel.h
@@ -164,7 +164,7 @@
// A transport channel is weak if the current best connection is either
// not receiving or not writable, or if there is no best connection at all.
- bool Weak() const;
+ bool weak() const;
void UpdateConnectionStates();
void RequestSort();
void SortConnections();
diff --git a/webrtc/p2p/base/p2ptransportchannel_unittest.cc b/webrtc/p2p/base/p2ptransportchannel_unittest.cc
index 4add040..56635dc 100644
--- a/webrtc/p2p/base/p2ptransportchannel_unittest.cc
+++ b/webrtc/p2p/base/p2ptransportchannel_unittest.cc
@@ -1112,17 +1112,24 @@
TestSendRecv(1);
cricket::ConnectionInfos infos;
ASSERT_TRUE(ep1_ch1()->GetStats(&infos));
- ASSERT_EQ(1U, infos.size());
- EXPECT_TRUE(infos[0].new_connection);
- EXPECT_TRUE(infos[0].best_connection);
- EXPECT_TRUE(infos[0].receiving);
- EXPECT_TRUE(infos[0].writable);
- EXPECT_FALSE(infos[0].timeout);
- EXPECT_EQ(10U, infos[0].sent_total_packets);
- EXPECT_EQ(0U, infos[0].sent_discarded_packets);
- EXPECT_EQ(10 * 36U, infos[0].sent_total_bytes);
- EXPECT_EQ(10 * 36U, infos[0].recv_total_bytes);
- EXPECT_GT(infos[0].rtt, 0U);
+ ASSERT_TRUE(infos.size() >= 1);
+ cricket::ConnectionInfo* best_conn_info = nullptr;
+ for (cricket::ConnectionInfo& info : infos) {
+ if (info.best_connection) {
+ best_conn_info = &info;
+ break;
+ }
+ }
+ ASSERT_TRUE(best_conn_info != nullptr);
+ EXPECT_TRUE(best_conn_info->new_connection);
+ EXPECT_TRUE(best_conn_info->receiving);
+ EXPECT_TRUE(best_conn_info->writable);
+ EXPECT_FALSE(best_conn_info->timeout);
+ EXPECT_EQ(10U, best_conn_info->sent_total_packets);
+ EXPECT_EQ(0U, best_conn_info->sent_discarded_packets);
+ EXPECT_EQ(10 * 36U, best_conn_info->sent_total_bytes);
+ EXPECT_EQ(10 * 36U, best_conn_info->recv_total_bytes);
+ EXPECT_GT(best_conn_info->rtt, 0U);
DestroyChannels();
}
@@ -1620,6 +1627,20 @@
DestroyChannels();
}
+TEST_F(P2PTransportChannelMultihomedTest, TestGetState) {
+ AddAddress(0, kAlternateAddrs[0]);
+ AddAddress(0, kPublicAddrs[0]);
+ AddAddress(1, kPublicAddrs[1]);
+ // Create channels and let them go writable, as usual.
+ CreateChannels(1);
+
+ // Both transport channels will reach STATE_COMPLETED quickly.
+ EXPECT_EQ_WAIT(cricket::TransportChannelState::STATE_COMPLETED,
+ ep1_ch1()->GetState(), 1000);
+ EXPECT_EQ_WAIT(cricket::TransportChannelState::STATE_COMPLETED,
+ ep2_ch1()->GetState(), 1000);
+}
+
/*
TODO(pthatcher): Once have a way to handle network interfaces changes
@@ -1796,9 +1817,11 @@
conn2->ReceivedPing();
conn2->ReceivedPingResponse();
- // Wait for conn1 to be destroyed.
- EXPECT_TRUE_WAIT(GetConnectionTo(&ch, "1.1.1.1", 1) == nullptr, 3000);
- cricket::Port* port = GetPort(&ch);
+ // Wait for conn1 to be pruned.
+ EXPECT_TRUE_WAIT(conn1->pruned(), 3000);
+ // Destroy the connection to test SignalUnknownAddress.
+ conn1->Destroy();
+ EXPECT_TRUE_WAIT(GetConnectionTo(&ch, "1.1.1.1", 1) == nullptr, 1000);
// Create a minimal STUN message with prflx priority.
cricket::IceMessage request;
@@ -1810,6 +1833,7 @@
cricket::STUN_ATTR_PRIORITY, prflx_priority));
EXPECT_NE(prflx_priority, remote_priority);
+ cricket::Port* port = GetPort(&ch);
// conn1 should be resurrected with original priority.
port->SignalUnknownAddress(port, rtc::SocketAddress("1.1.1.1", 1),
cricket::PROTO_UDP, &request, kIceUfrag[1], false);
@@ -2068,3 +2092,70 @@
WAIT(conn3->pruned(), 1000);
EXPECT_FALSE(conn3->pruned());
}
+
+// Test that GetState returns the state correctly.
+TEST_F(P2PTransportChannelPingTest, TestGetState) {
+ cricket::FakePortAllocator pa(rtc::Thread::Current(), nullptr);
+ cricket::P2PTransportChannel ch("test channel", 1, nullptr, &pa);
+ PrepareChannel(&ch);
+ ch.Connect();
+ ch.MaybeStartGathering();
+ EXPECT_EQ(cricket::TransportChannelState::STATE_INIT, ch.GetState());
+ ch.AddRemoteCandidate(CreateCandidate("1.1.1.1", 1, 100));
+ ch.AddRemoteCandidate(CreateCandidate("2.2.2.2", 2, 1));
+ 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);
+ // Now there are two connections, so the transport channel is connecting.
+ EXPECT_EQ(cricket::TransportChannelState::STATE_CONNECTING, ch.GetState());
+ // |conn1| becomes writable and receiving; it then should prune |conn2|.
+ conn1->ReceivedPingResponse();
+ EXPECT_TRUE_WAIT(conn2->pruned(), 1000);
+ EXPECT_EQ(cricket::TransportChannelState::STATE_COMPLETED, ch.GetState());
+ conn1->Prune(); // All connections are pruned.
+ EXPECT_EQ(cricket::TransportChannelState::STATE_FAILED, ch.GetState());
+}
+
+// Test that when a low-priority connection is pruned, it is not deleted
+// right away, and it can become active and be pruned again.
+TEST_F(P2PTransportChannelPingTest, TestConnectionPrunedAgain) {
+ cricket::FakePortAllocator pa(rtc::Thread::Current(), nullptr);
+ cricket::P2PTransportChannel ch("test channel", 1, nullptr, &pa);
+ PrepareChannel(&ch);
+ ch.SetIceConfig(CreateIceConfig(1000, false));
+ ch.Connect();
+ ch.MaybeStartGathering();
+ ch.AddRemoteCandidate(CreateCandidate("1.1.1.1", 1, 100));
+ cricket::Connection* conn1 = WaitForConnectionTo(&ch, "1.1.1.1", 1);
+ ASSERT_TRUE(conn1 != nullptr);
+ EXPECT_EQ(conn1, ch.best_connection());
+ conn1->ReceivedPingResponse(); // Becomes writable and receiving
+
+ // Add a low-priority connection |conn2|, which will be pruned, but it will
+ // not be deleted right away. Once the current best connection becomes not
+ // receiving, |conn2| will start to ping and upon receiving the ping response,
+ // it will become the best connection.
+ ch.AddRemoteCandidate(CreateCandidate("2.2.2.2", 2, 1));
+ cricket::Connection* conn2 = WaitForConnectionTo(&ch, "2.2.2.2", 2);
+ ASSERT_TRUE(conn2 != nullptr);
+ EXPECT_TRUE_WAIT(!conn2->active(), 1000);
+ // |conn2| should not send a ping yet.
+ EXPECT_EQ(cricket::Connection::STATE_WAITING, conn2->state());
+ EXPECT_EQ(cricket::TransportChannelState::STATE_COMPLETED, ch.GetState());
+ // Wait for |conn1| becoming not receiving.
+ EXPECT_TRUE_WAIT(!conn1->receiving(), 3000);
+ // Make sure conn2 is not deleted.
+ conn2 = WaitForConnectionTo(&ch, "2.2.2.2", 2);
+ ASSERT_TRUE(conn2 != nullptr);
+ EXPECT_EQ_WAIT(cricket::Connection::STATE_INPROGRESS, conn2->state(), 1000);
+ conn2->ReceivedPingResponse();
+ EXPECT_EQ_WAIT(conn2, ch.best_connection(), 1000);
+ EXPECT_EQ(cricket::TransportChannelState::STATE_CONNECTING, ch.GetState());
+
+ // When |conn1| comes back again, |conn2| will be pruned again.
+ conn1->ReceivedPingResponse();
+ EXPECT_EQ_WAIT(conn1, ch.best_connection(), 1000);
+ EXPECT_TRUE_WAIT(!conn2->active(), 1000);
+ EXPECT_EQ(cricket::TransportChannelState::STATE_COMPLETED, ch.GetState());
+}
diff --git a/webrtc/p2p/base/port.cc b/webrtc/p2p/base/port.cc
index 842a26e..a5c7770 100644
--- a/webrtc/p2p/base/port.cc
+++ b/webrtc/p2p/base/port.cc
@@ -801,7 +801,8 @@
sent_packets_total_(0),
reported_(false),
state_(STATE_WAITING),
- receiving_timeout_(WEAK_CONNECTION_RECEIVE_TIMEOUT) {
+ receiving_timeout_(WEAK_CONNECTION_RECEIVE_TIMEOUT),
+ time_created_ms_(rtc::Time()) {
// All of our connections start in WAITING state.
// TODO(mallinath) - Start connections from STATE_FROZEN.
// Wire up to send stun packets
@@ -849,7 +850,6 @@
LOG_J(LS_VERBOSE, this) << "set_write_state from: " << old_value << " to "
<< value;
SignalStateChange(this);
- CheckTimeout();
}
}
@@ -858,7 +858,6 @@
LOG_J(LS_VERBOSE, this) << "set_receiving to " << value;
receiving_ = value;
SignalStateChange(this);
- CheckTimeout();
}
}
@@ -999,7 +998,7 @@
}
void Connection::Prune() {
- if (!pruned_) {
+ if (!pruned_ || active()) {
LOG_J(LS_VERBOSE, this) << "Connection pruned";
pruned_ = true;
requests_.Clear();
@@ -1089,6 +1088,9 @@
uint32 last_recv_time = last_received();
bool receiving = now <= last_recv_time + receiving_timeout_;
set_receiving(receiving);
+ if (dead(now)) {
+ Destroy();
+ }
}
void Connection::Ping(uint32 now) {
@@ -1119,6 +1121,28 @@
last_ping_response_received_ = rtc::Time();
}
+bool Connection::dead(uint32 now) const {
+ if (now < (time_created_ms_ + MIN_CONNECTION_LIFETIME)) {
+ // A connection that hasn't passed its minimum lifetime is still alive.
+ // We do this to prevent connections from being pruned too quickly
+ // during a network change event when two networks would be up
+ // simultaneously but only for a brief period.
+ return false;
+ }
+
+ if (receiving_) {
+ // A connection that is receiving is alive.
+ return false;
+ }
+
+ // A connection is alive until it is inactive.
+ return !active();
+
+ // TODO(honghaiz): Move from using the write state to using the receiving
+ // state with something like the following:
+ // return (now > (last_received() + DEAD_CONNECTION_RECEIVE_TIMEOUT));
+}
+
std::string Connection::ToDebugId() const {
std::stringstream ss;
ss << std::hex << this;
@@ -1230,7 +1254,7 @@
LOG_J(LS_ERROR, this) << "Received STUN error response, code="
<< error_code << "; killing connection";
set_state(STATE_FAILED);
- set_write_state(STATE_WRITE_TIMEOUT);
+ Destroy();
}
}
@@ -1251,13 +1275,6 @@
<< ", use_candidate=" << use_candidate;
}
-void Connection::CheckTimeout() {
- // If write has timed out and it is not receiving, remove the connection.
- if (!receiving_ && write_state_ == STATE_WRITE_TIMEOUT) {
- Destroy();
- }
-}
-
void Connection::HandleRoleConflictFromPeer() {
port_->SignalRoleConflict(port_);
}
diff --git a/webrtc/p2p/base/port.h b/webrtc/p2p/base/port.h
index 47903be..4616963 100644
--- a/webrtc/p2p/base/port.h
+++ b/webrtc/p2p/base/port.h
@@ -50,9 +50,9 @@
extern const char TCPTYPE_PASSIVE_STR[];
extern const char TCPTYPE_SIMOPEN_STR[];
-// If a connection does not receive anything for this long, it is considered
-// dead.
-const uint32 DEAD_CONNECTION_RECEIVE_TIMEOUT = 30 * 1000; // 30 seconds.
+// The minimum time we will wait before destroying a connection after creating
+// it.
+const uint32 MIN_CONNECTION_LIFETIME = 10 * 1000; // 10 seconds.
// The timeout duration when a connection does not receive anything.
const uint32 WEAK_CONNECTION_RECEIVE_TIMEOUT = 2500; // 2.5 seconds
@@ -435,8 +435,13 @@
// Determines whether the connection has finished connecting. This can only
// be false for TCP connections.
bool connected() const { return connected_; }
-
- bool Weak() const { return !(writable() && receiving() && connected()); }
+ bool weak() const { return !(writable() && receiving() && connected()); }
+ bool active() const {
+ // TODO(honghaiz): Move from using |write_state_| to using |pruned_|.
+ return write_state_ != STATE_WRITE_TIMEOUT;
+ }
+ // A connection is dead if it can be safely deleted.
+ bool dead(uint32 now) const;
// Estimate of the round-trip time over this connection.
uint32 rtt() const { return rtt_; }
@@ -572,9 +577,6 @@
void set_state(State state);
void set_connected(bool value);
- // Checks if this connection is useless, and hence, should be destroyed.
- void CheckTimeout();
-
void OnMessage(rtc::Message *pmsg);
Port* port_;
@@ -615,6 +617,7 @@
State state_;
// Time duration to switch from receiving to not receiving.
uint32 receiving_timeout_;
+ uint32 time_created_ms_;
friend class Port;
friend class ConnectionRequest;
