Don't delete an ICE connection until it has been pruned or timed out on writing in the case where it
hasn't received anything yet. Deleting an ICE connection before it is pruned or timed out
when it hasn't received anything yet leads to ICE connections being deleted
before they have a chance to send a ping and receive a response.
BUG=
Review URL: https://codereview.webrtc.org/1544003002
Cr-Commit-Position: refs/heads/master@{#11151}
diff --git a/webrtc/p2p/base/port.cc b/webrtc/p2p/base/port.cc
index ddf38dc..f6a3009 100644
--- a/webrtc/p2p/base/port.cc
+++ b/webrtc/p2p/base/port.cc
@@ -1124,17 +1124,28 @@
}
bool Connection::dead(uint32_t 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.
+ if (last_received() > 0) {
+ // If it has ever received anything, we keep it alive until it hasn't
+ // received anything for DEAD_CONNECTION_RECEIVE_TIMEOUT. This covers the
+ // normal case of a successfully used connection that stops working. This
+ // also allows a remote peer to continue pinging over a locally inactive
+ // (pruned) connection.
+ return (now > (last_received() + DEAD_CONNECTION_RECEIVE_TIMEOUT));
+ }
+
+ if (active()) {
+ // If it has never received anything, keep it alive as long as it is
+ // actively pinging and not pruned. Otherwise, the connection might be
+ // deleted before it has a chance to ping. This is the normal case for a
+ // new connection that is pinging but hasn't received anything yet.
return false;
}
- // It is dead if it has not received anything for
- // DEAD_CONNECTION_RECEIVE_TIMEOUT milliseconds.
- return (now > (last_received() + DEAD_CONNECTION_RECEIVE_TIMEOUT));
+ // If it has never received anything and is not actively pinging (pruned), we
+ // keep it around for at least MIN_CONNECTION_LIFETIME 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 now > (time_created_ms_ + MIN_CONNECTION_LIFETIME);
}
std::string Connection::ToDebugId() const {
diff --git a/webrtc/p2p/base/port_unittest.cc b/webrtc/p2p/base/port_unittest.cc
index 4bff58a..34806d9 100644
--- a/webrtc/p2p/base/port_unittest.cc
+++ b/webrtc/p2p/base/port_unittest.cc
@@ -1255,19 +1255,26 @@
ASSERT_EQ_WAIT(1, ch1.complete_count(), kTimeout);
ASSERT_EQ_WAIT(1, ch2.complete_count(), kTimeout);
+ // Test case that the connection has never received anything.
uint32_t before_created = rtc::Time();
ch1.CreateConnection(GetCandidate(port2));
uint32_t after_created = rtc::Time();
Connection* conn = ch1.conn();
ASSERT(conn != nullptr);
- // If the connection has never received anything, it will be dead after
- // MIN_CONNECTION_LIFETIME
- conn->UpdateState(before_created + MIN_CONNECTION_LIFETIME - 1);
- rtc::Thread::Current()->ProcessMessages(100);
+ // It is not dead if it is after MIN_CONNECTION_LIFETIME but not pruned.
+ conn->UpdateState(after_created + MIN_CONNECTION_LIFETIME + 1);
+ rtc::Thread::Current()->ProcessMessages(0);
EXPECT_TRUE(ch1.conn() != nullptr);
+ // It is not dead if it is before MIN_CONNECTION_LIFETIME and pruned.
+ conn->UpdateState(before_created + MIN_CONNECTION_LIFETIME - 1);
+ conn->Prune();
+ rtc::Thread::Current()->ProcessMessages(0);
+ EXPECT_TRUE(ch1.conn() != nullptr);
+ // It will be dead after MIN_CONNECTION_LIFETIME and pruned.
conn->UpdateState(after_created + MIN_CONNECTION_LIFETIME + 1);
EXPECT_TRUE_WAIT(ch1.conn() == nullptr, kTimeout);
+ // Test case that the connection has received something.
// Create a connection again and receive a ping.
ch1.CreateConnection(GetCandidate(port2));
conn = ch1.conn();
