From 22e623ad6837e545d60cdb557f845a4cccac1568 Mon Sep 17 00:00:00 2001 From: Qingsi Wang Date: Tue, 13 Mar 2018 10:53:57 -0700 Subject: [PATCH] Add configurable threshold for writability state update. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add configurable parameters in RTCConfiguration with the default value given by the constants CONNECTION_WRITE_CONNECT_TIME and CONNECTION_WRITE_CONNECT_FAILURES in the ICE implementation. These two parameters define the time period for which a candidate pair must wait for ping response and the minimum number of connectivity checks that the pair must send without response before its state becomes unreliable from writable as defined in the current ICE implementation. Bug: webrtc:8988 Change-Id: I484599b7d776489a87741ffea8926df766095da9 Reviewed-on: https://webrtc-review.googlesource.com/60704 Commit-Queue: Qingsi Wang Reviewed-by: Taylor Brandstetter Reviewed-by: Sami Kalliomäki Cr-Commit-Position: refs/heads/master@{#22411} --- api/peerconnectioninterface.h | 10 +++ p2p/base/icetransportinternal.h | 11 +++ p2p/base/p2ptransportchannel.cc | 20 +++++ p2p/base/port.cc | 17 +++-- p2p/base/port.h | 15 ++++ p2p/base/port_unittest.cc | 73 ++++++++++++++++++- p2p/base/tcpport.cc | 4 + pc/peerconnection.cc | 7 ++ .../api/org/webrtc/PeerConnection.java | 18 +++++ sdk/android/src/jni/pc/peerconnection.cc | 8 ++ 10 files changed, 173 insertions(+), 10 deletions(-) diff --git a/api/peerconnectioninterface.h b/api/peerconnectioninterface.h index 7364aa0ced..4cb3d65815 100644 --- a/api/peerconnectioninterface.h +++ b/api/peerconnectioninterface.h @@ -515,6 +515,16 @@ class PeerConnectionInterface : public rtc::RefCountInterface { rtc::Optional ice_check_interval_weak_connectivity; rtc::Optional ice_check_min_interval; + // The min time period for which a candidate pair must wait for response to + // connectivity checks before it becomes unwritable. This parameter + // overrides the default value in the ICE implementation if set. + rtc::Optional ice_unwritable_timeout; + + // The min number of connectivity checks that a candidate pair must sent + // without receiving response before it becomes unwritable. This parameter + // overrides the default value in the ICE implementation if set. + rtc::Optional ice_unwritable_min_checks; + // The interval in milliseconds at which STUN candidates will resend STUN // binding requests to keep NAT bindings open. rtc::Optional stun_candidate_keepalive_interval; diff --git a/p2p/base/icetransportinternal.h b/p2p/base/icetransportinternal.h index a111f44eac..f5cafd09c7 100644 --- a/p2p/base/icetransportinternal.h +++ b/p2p/base/icetransportinternal.h @@ -128,6 +128,17 @@ struct IceConfig { // candidate pairs with strong or weak connectivity, if either of the above // interval is shorter than the min interval. rtc::Optional ice_check_min_interval; + // The min time period for which a candidate pair must wait for response to + // connectivity checks before it becomes unwritable. This parameter + // overrides the default value given by |CONNECTION_WRITE_CONNECT_TIMEOUT| + // in port.h if set, when determining the writability of a candidate pair. + rtc::Optional ice_unwritable_timeout; + + // The min number of connectivity checks that a candidate pair must sent + // without receiving response before it becomes unwritable. This parameter + // overrides the default value given by |CONNECTION_WRITE_CONNECT_FAILURES| in + // port.h if set, when determining the writability of a candidate pair. + rtc::Optional ice_unwritable_min_checks; // The interval in milliseconds at which STUN candidates will resend STUN // binding requests to keep NAT bindings open. rtc::Optional stun_keepalive_interval; diff --git a/p2p/base/p2ptransportchannel.cc b/p2p/base/p2ptransportchannel.cc index 5384801674..1398cb1813 100644 --- a/p2p/base/p2ptransportchannel.cc +++ b/p2p/base/p2ptransportchannel.cc @@ -215,6 +215,8 @@ void P2PTransportChannel::AddConnection(Connection* connection) { unpinged_connections_.insert(connection); connection->set_remote_ice_mode(remote_ice_mode_); connection->set_receiving_timeout(config_.receiving_timeout); + connection->set_unwritable_timeout(config_.ice_unwritable_timeout); + connection->set_unwritable_min_checks(config_.ice_unwritable_min_checks); connection->SignalReadPacket.connect( this, &P2PTransportChannel::OnReadPacket); connection->SignalReadyToSend.connect( @@ -566,6 +568,24 @@ void P2PTransportChannel::SetIceConfig(const IceConfig& config) { << config_.ice_check_min_interval.value_or(-1); } + if (config_.ice_unwritable_timeout != config.ice_unwritable_timeout) { + config_.ice_unwritable_timeout = config.ice_unwritable_timeout; + for (Connection* conn : connections_) { + conn->set_unwritable_timeout(config_.ice_unwritable_timeout); + } + RTC_LOG(LS_INFO) << "Set unwritable timeout to " + << config_.ice_unwritable_timeout.value_or(-1); + } + + if (config_.ice_unwritable_min_checks != config.ice_unwritable_min_checks) { + config_.ice_unwritable_min_checks = config.ice_unwritable_min_checks; + for (Connection* conn : connections_) { + conn->set_unwritable_min_checks(config_.ice_unwritable_min_checks); + } + RTC_LOG(LS_INFO) << "Set unwritable min checks to " + << config_.ice_unwritable_min_checks.value_or(-1); + } + if (config_.network_preference != config.network_preference) { config_.network_preference = config.network_preference; RequestSortAndStateUpdate(); diff --git a/p2p/base/port.cc b/p2p/base/port.cc index b142865b75..dfc3f12949 100644 --- a/p2p/base/port.cc +++ b/p2p/base/port.cc @@ -1126,6 +1126,14 @@ void Connection::set_use_candidate_attr(bool enable) { use_candidate_attr_ = enable; } +int Connection::unwritable_timeout() const { + return unwritable_timeout_.value_or(CONNECTION_WRITE_CONNECT_TIMEOUT); +} + +int Connection::unwritable_min_checks() const { + return unwritable_min_checks_.value_or(CONNECTION_WRITE_CONNECT_FAILURES); +} + void Connection::OnSendStunPacket(const void* data, size_t size, StunRequest* req) { rtc::PacketOptions options(port_->DefaultDscpValue()); @@ -1351,14 +1359,11 @@ void Connection::UpdateState(int64_t now) { // allow for changes in network conditions. if ((write_state_ == STATE_WRITABLE) && - TooManyFailures(pings_since_last_response_, - CONNECTION_WRITE_CONNECT_FAILURES, - rtt, + TooManyFailures(pings_since_last_response_, unwritable_min_checks(), rtt, now) && - TooLongWithoutResponse(pings_since_last_response_, - CONNECTION_WRITE_CONNECT_TIMEOUT, + TooLongWithoutResponse(pings_since_last_response_, unwritable_timeout(), now)) { - uint32_t max_pings = CONNECTION_WRITE_CONNECT_FAILURES; + uint32_t max_pings = unwritable_min_checks(); LOG_J(LS_INFO, this) << "Unwritable after " << max_pings << " ping failures and " << now - pings_since_last_response_[0].sent_time diff --git a/p2p/base/port.h b/p2p/base/port.h index e45308def5..d9f7bf9e8f 100644 --- a/p2p/base/port.h +++ b/p2p/base/port.h @@ -575,6 +575,18 @@ class Connection : public CandidatePairInterface, // Estimate of the round-trip time over this connection. int rtt() const { return rtt_; } + int unwritable_timeout() const; + void set_unwritable_timeout(const rtc::Optional& value_ms) { + // TODO(qingsi): Validate configuration parameters in + // PeerConnection::ValidateConfiguration. + RTC_CHECK_LT(value_ms.value_or(-1), CONNECTION_WRITE_TIMEOUT); + unwritable_timeout_ = value_ms; + } + int unwritable_min_checks() const; + void set_unwritable_min_checks(const rtc::Optional& value) { + unwritable_min_checks_ = value; + } + // Gets the |ConnectionInfo| stats, where |best_connection| has not been // populated (default value false). ConnectionInfo stats(); @@ -817,6 +829,9 @@ class Connection : public CandidatePairInterface, PacketLossEstimator packet_loss_estimator_; + rtc::Optional unwritable_timeout_; + rtc::Optional unwritable_min_checks_; + bool reported_; IceCandidatePairState state_; // Time duration to switch from receiving to not receiving. diff --git a/p2p/base/port_unittest.cc b/p2p/base/port_unittest.cc index 8f5867f1c7..bbf2fef69a 100644 --- a/p2p/base/port_unittest.cc +++ b/p2p/base/port_unittest.cc @@ -52,6 +52,7 @@ namespace { constexpr int kDefaultTimeout = 3000; constexpr int kShortTimeout = 1000; +constexpr int kMaxExpectedSimulatedRtt = 200; const SocketAddress kLocalAddr1("192.168.1.2", 0); const SocketAddress kLocalAddr2("192.168.1.3", 0); const SocketAddress kNatAddr1("77.77.77.77", rtc::NAT_SERVER_UDP_PORT); @@ -2559,6 +2560,12 @@ TEST_F(PortTest, TestConnectionPriority) { #endif } +// Note that UpdateState takes into account the estimated RTT, and the +// correctness of using |kMaxExpectedSimulatedRtt| as an upper bound of RTT in +// the following tests depends on the link rate and the delay distriubtion +// configured in VirtualSocketServer::AddPacketToNetwork. The tests below use +// the default setup where the RTT is deterministically one, which generates an +// estimate given by |MINIMUM_RTT| = 100. TEST_F(PortTest, TestWritableState) { rtc::ScopedFakeClock clock; UDPPort* port1 = CreateUdpPort(kLocalAddr1); @@ -2604,7 +2611,8 @@ TEST_F(PortTest, TestWritableState) { for (uint32_t i = 1; i <= CONNECTION_WRITE_CONNECT_FAILURES; ++i) { ch1.Ping(i); } - int unreliable_timeout_delay = CONNECTION_WRITE_CONNECT_TIMEOUT + 500; + int unreliable_timeout_delay = + CONNECTION_WRITE_CONNECT_TIMEOUT + kMaxExpectedSimulatedRtt; ch1.conn()->UpdateState(unreliable_timeout_delay); EXPECT_EQ(Connection::STATE_WRITE_UNRELIABLE, ch1.conn()->write_state()); @@ -2615,14 +2623,13 @@ TEST_F(PortTest, TestWritableState) { // responses. EXPECT_EQ_SIMULATED_WAIT(Connection::STATE_WRITABLE, ch1.conn()->write_state(), kDefaultTimeout, clock); - // Wait long enough for a full timeout (past however long we've already // waited). for (uint32_t i = 1; i <= CONNECTION_WRITE_CONNECT_FAILURES; ++i) { ch1.Ping(unreliable_timeout_delay + i); } ch1.conn()->UpdateState(unreliable_timeout_delay + CONNECTION_WRITE_TIMEOUT + - 500u); + kMaxExpectedSimulatedRtt); EXPECT_EQ(Connection::STATE_WRITE_TIMEOUT, ch1.conn()->write_state()); // Even if the connection has timed out, the Connection shouldn't block @@ -2633,6 +2640,64 @@ TEST_F(PortTest, TestWritableState) { ch2.Stop(); } +// Test writability states using the configured threshold value to replace +// the default value given by |CONNECTION_WRITE_CONNECT_TIMEOUT| and +// |CONNECTION_WRITE_CONNECT_FAILURES|. +TEST_F(PortTest, TestWritableStateWithConfiguredThreshold) { + rtc::ScopedFakeClock clock; + UDPPort* port1 = CreateUdpPort(kLocalAddr1); + port1->SetIceRole(cricket::ICEROLE_CONTROLLING); + UDPPort* port2 = CreateUdpPort(kLocalAddr2); + port2->SetIceRole(cricket::ICEROLE_CONTROLLED); + + // Set up channels. + TestChannel ch1(port1); + TestChannel ch2(port2); + + // Acquire addresses. + ch1.Start(); + ch2.Start(); + ASSERT_EQ_SIMULATED_WAIT(1, ch1.complete_count(), kDefaultTimeout, clock); + ASSERT_EQ_SIMULATED_WAIT(1, ch2.complete_count(), kDefaultTimeout, clock); + + // Send a ping from src to dst. + ch1.CreateConnection(GetCandidate(port2)); + ASSERT_TRUE(ch1.conn() != NULL); + ch1.Ping(); + SIMULATED_WAIT(!ch2.remote_address().IsNil(), kShortTimeout, clock); + + // Accept the connection to return the binding response, transition to + // writable, and allow data to be sent. + ch2.AcceptConnection(GetCandidate(port1)); + EXPECT_EQ_SIMULATED_WAIT(Connection::STATE_WRITABLE, + ch1.conn()->write_state(), kDefaultTimeout, clock); + + ch1.conn()->set_unwritable_timeout(1000); + ch1.conn()->set_unwritable_min_checks(3); + // Send two checks. + ch1.Ping(1); + ch1.Ping(2); + // We have not reached the timeout nor have we sent the minimum number of + // checks to change the state to Unreliable. + ch1.conn()->UpdateState(999); + EXPECT_EQ(Connection::STATE_WRITABLE, ch1.conn()->write_state()); + // We have not sent the minimum number of checks without responses. + ch1.conn()->UpdateState(1000 + kMaxExpectedSimulatedRtt); + EXPECT_EQ(Connection::STATE_WRITABLE, ch1.conn()->write_state()); + // Last ping after which the candidate pair should become Unreliable after + // timeout. + ch1.Ping(3); + // We have not reached the timeout. + ch1.conn()->UpdateState(999); + EXPECT_EQ(Connection::STATE_WRITABLE, ch1.conn()->write_state()); + // We should be in the state Unreliable now. + ch1.conn()->UpdateState(1000 + kMaxExpectedSimulatedRtt); + EXPECT_EQ(Connection::STATE_WRITE_UNRELIABLE, ch1.conn()->write_state()); + + ch1.Stop(); + ch2.Stop(); +} + TEST_F(PortTest, TestTimeoutForNeverWritable) { UDPPort* port1 = CreateUdpPort(kLocalAddr1); port1->SetIceRole(cricket::ICEROLE_CONTROLLING); @@ -2655,7 +2720,7 @@ TEST_F(PortTest, TestTimeoutForNeverWritable) { for (uint32_t i = 1; i <= CONNECTION_WRITE_CONNECT_FAILURES; ++i) { ch1.Ping(i); } - ch1.conn()->UpdateState(CONNECTION_WRITE_TIMEOUT + 500u); + ch1.conn()->UpdateState(CONNECTION_WRITE_TIMEOUT + kMaxExpectedSimulatedRtt); EXPECT_EQ(Connection::STATE_WRITE_TIMEOUT, ch1.conn()->write_state()); } diff --git a/p2p/base/tcpport.cc b/p2p/base/tcpport.cc index 4c1a8e4235..b780a1e527 100644 --- a/p2p/base/tcpport.cc +++ b/p2p/base/tcpport.cc @@ -323,6 +323,10 @@ void TCPPort::OnAddressReady(rtc::AsyncPacketSocket* socket, 0, "", true); } +// TODO(qingsi): |CONNECTION_WRITE_CONNECT_TIMEOUT| is overriden by +// |ice_unwritable_timeout| in IceConfig when determining the writability state. +// Replace this constant with the config parameter assuming the default value if +// we decide it is also applicable here. TCPConnection::TCPConnection(TCPPort* port, const Candidate& candidate, rtc::AsyncPacketSocket* socket) diff --git a/pc/peerconnection.cc b/pc/peerconnection.cc index fadb47bcca..7f1ddef78a 100644 --- a/pc/peerconnection.cc +++ b/pc/peerconnection.cc @@ -666,6 +666,8 @@ bool PeerConnectionInterface::RTCConfiguration::operator==( rtc::Optional ice_check_interval_strong_connectivity; rtc::Optional ice_check_interval_weak_connectivity; rtc::Optional ice_check_min_interval; + rtc::Optional ice_unwritable_timeout; + rtc::Optional ice_unwritable_min_checks; rtc::Optional stun_candidate_keepalive_interval; rtc::Optional ice_regather_interval_range; webrtc::TurnCustomizer* turn_customizer; @@ -710,6 +712,8 @@ bool PeerConnectionInterface::RTCConfiguration::operator==( ice_check_interval_weak_connectivity == o.ice_check_interval_weak_connectivity && ice_check_min_interval == o.ice_check_min_interval && + ice_unwritable_timeout == o.ice_unwritable_timeout && + ice_unwritable_min_checks == o.ice_unwritable_min_checks && stun_candidate_keepalive_interval == o.stun_candidate_keepalive_interval && ice_regather_interval_range == o.ice_regather_interval_range && @@ -2722,6 +2726,9 @@ bool PeerConnection::SetConfiguration(const RTCConfiguration& configuration, configuration.ice_check_interval_strong_connectivity; modified_config.ice_check_interval_weak_connectivity = configuration.ice_check_interval_weak_connectivity; + modified_config.ice_unwritable_timeout = configuration.ice_unwritable_timeout; + modified_config.ice_unwritable_min_checks = + configuration.ice_unwritable_min_checks; modified_config.stun_candidate_keepalive_interval = configuration.stun_candidate_keepalive_interval; modified_config.turn_customizer = configuration.turn_customizer; diff --git a/sdk/android/api/org/webrtc/PeerConnection.java b/sdk/android/api/org/webrtc/PeerConnection.java index 65aa734983..cf0eed50b9 100644 --- a/sdk/android/api/org/webrtc/PeerConnection.java +++ b/sdk/android/api/org/webrtc/PeerConnection.java @@ -409,6 +409,12 @@ public class PeerConnection { public Integer iceCheckIntervalStrongConnectivityMs; public Integer iceCheckIntervalWeakConnectivityMs; public Integer iceCheckMinInterval; + // The time period in milliseconds for which a candidate pair must wait for response to + // connectivitiy checks before it becomes unwritable. + public Integer iceUnwritableTimeMs; + // The minimum number of connectivity checks that a candidate pair must sent without receiving + // response before it becomes unwritable. + public Integer iceUnwritableMinChecks; // The interval in milliseconds at which STUN candidates will resend STUN binding requests // to keep NAT bindings open. // The default value in the implementation is used if this field is null. @@ -462,6 +468,8 @@ public class PeerConnection { iceCheckIntervalStrongConnectivityMs = null; iceCheckIntervalWeakConnectivityMs = null; iceCheckMinInterval = null; + iceUnwritableTimeMs = null; + iceUnwritableMinChecks = null; stunCandidateKeepaliveIntervalMs = null; disableIPv6OnWifi = false; maxIPv6Networks = 5; @@ -568,6 +576,16 @@ public class PeerConnection { return iceCheckMinInterval; } + @CalledByNative("RTCConfiguration") + Integer getIceUnwritableTimeout() { + return iceUnwritableTimeMs; + } + + @CalledByNative("RTCConfiguration") + Integer getIceUnwritableMinChecks() { + return iceUnwritableMinChecks; + } + @CalledByNative("RTCConfiguration") Integer getStunCandidateKeepaliveInterval() { return stunCandidateKeepaliveIntervalMs; diff --git a/sdk/android/src/jni/pc/peerconnection.cc b/sdk/android/src/jni/pc/peerconnection.cc index c18af7f17f..16e40d17df 100644 --- a/sdk/android/src/jni/pc/peerconnection.cc +++ b/sdk/android/src/jni/pc/peerconnection.cc @@ -187,6 +187,14 @@ void JavaToNativeRTCConfiguration( Java_RTCConfiguration_getIceCheckMinInterval(jni, j_rtc_config); rtc_config->ice_check_min_interval = JavaToNativeOptionalInt(jni, j_ice_check_min_interval); + ScopedJavaLocalRef j_ice_unwritable_timeout = + Java_RTCConfiguration_getIceUnwritableTimeout(jni, j_rtc_config); + rtc_config->ice_unwritable_timeout = + JavaToNativeOptionalInt(jni, j_ice_unwritable_timeout); + ScopedJavaLocalRef j_ice_unwritable_min_checks = + Java_RTCConfiguration_getIceUnwritableMinChecks(jni, j_rtc_config); + rtc_config->ice_unwritable_min_checks = + JavaToNativeOptionalInt(jni, j_ice_unwritable_min_checks); ScopedJavaLocalRef j_stun_candidate_keepalive_interval = Java_RTCConfiguration_getStunCandidateKeepaliveInterval(jni, j_rtc_config);