diff --git a/webrtc/p2p/base/p2ptransportchannel.cc b/webrtc/p2p/base/p2ptransportchannel.cc index cce20f4abb..1b7cb58756 100644 --- a/webrtc/p2p/base/p2ptransportchannel.cc +++ b/webrtc/p2p/base/p2ptransportchannel.cc @@ -286,23 +286,26 @@ void P2PTransportChannel::SetIceTiebreaker(uint64 tiebreaker) { 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 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 active_connections; + for (Connection* connection : connections_) { + if (connection->active()) { + active_connections.push_back(connection); + } + } + if (active_connections.empty()) { + return TransportChannelState::STATE_FAILED; + } + + std::set 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 @@ TransportChannelState P2PTransportChannel::GetState() const { 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 @@ bool P2PTransportChannel::GetStats(ConnectionInfos *infos) { infos->clear(); std::vector::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 @@ void P2PTransportChannel::PruneConnections() { 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 @@ void P2PTransportChannel::HandleAllTimedOut() { 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 @@ void P2PTransportChannel::OnCheckAndPing() { 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 @@ bool P2PTransportChannel::IsPingable(Connection* conn) { // 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 3f7b696002..b3e7bc1a84 100644 --- a/webrtc/p2p/base/p2ptransportchannel.h +++ b/webrtc/p2p/base/p2ptransportchannel.h @@ -164,7 +164,7 @@ class P2PTransportChannel : public TransportChannelImpl, // 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 4add040afe..56635dce44 100644 --- a/webrtc/p2p/base/p2ptransportchannel_unittest.cc +++ b/webrtc/p2p/base/p2ptransportchannel_unittest.cc @@ -1112,17 +1112,24 @@ TEST_F(P2PTransportChannelTest, GetStats) { 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 @@ TEST_F(P2PTransportChannelMultihomedTest, TestFailoverControllingSide) { 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 @@ TEST_F(P2PTransportChannelPingTest, ConnectionResurrection) { 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 @@ TEST_F(P2PTransportChannelPingTest, ConnectionResurrection) { 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 @@ TEST_F(P2PTransportChannelPingTest, TestDontPruneWhenWeak) { 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 842a26e826..a5c7770f55 100644 --- a/webrtc/p2p/base/port.cc +++ b/webrtc/p2p/base/port.cc @@ -801,7 +801,8 @@ Connection::Connection(Port* port, 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 @@ void Connection::set_write_state(WriteState value) { LOG_J(LS_VERBOSE, this) << "set_write_state from: " << old_value << " to " << value; SignalStateChange(this); - CheckTimeout(); } } @@ -858,7 +858,6 @@ void Connection::set_receiving(bool value) { LOG_J(LS_VERBOSE, this) << "set_receiving to " << value; receiving_ = value; SignalStateChange(this); - CheckTimeout(); } } @@ -999,7 +998,7 @@ void Connection::OnReadyToSend() { } void Connection::Prune() { - if (!pruned_) { + if (!pruned_ || active()) { LOG_J(LS_VERBOSE, this) << "Connection pruned"; pruned_ = true; requests_.Clear(); @@ -1089,6 +1088,9 @@ void Connection::UpdateState(uint32 now) { 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 @@ void Connection::ReceivedPingResponse() { 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 @@ void Connection::OnConnectionRequestErrorResponse(ConnectionRequest* request, 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 @@ void Connection::OnConnectionRequestSent(ConnectionRequest* request) { << ", 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 47903be05b..4616963aa0 100644 --- a/webrtc/p2p/base/port.h +++ b/webrtc/p2p/base/port.h @@ -50,9 +50,9 @@ extern const char TCPTYPE_ACTIVE_STR[]; 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 @@ class Connection : public rtc::MessageHandler, // 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 @@ class Connection : public rtc::MessageHandler, 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 @@ class Connection : public rtc::MessageHandler, State state_; // Time duration to switch from receiving to not receiving. uint32 receiving_timeout_; + uint32 time_created_ms_; friend class Port; friend class ConnectionRequest;