Add piggyback acknowledgement of the last ICE check received in
outgoing checks.
This change adds an experimental feature to allow an ICE agent to embed
the transaction ID of the latest connectivity check received from the
remote peer, as an auxiliary acknowledgement in additional to the check
response, in its own checks. This could facilitate the establishment of
ICE connectivity if the check process has a high RTT.
Bug: None
Change-Id: If3e6327720f13beeb14f103af3b5ffb4f9692998
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/142682
Reviewed-by: Honghai Zhang <honghaiz@webrtc.org>
Commit-Queue: Qingsi Wang <qingsi@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#28316}
diff --git a/p2p/base/connection.cc b/p2p/base/connection.cc
index 6fc93ed..2b04aa5 100644
--- a/p2p/base/connection.cc
+++ b/p2p/base/connection.cc
@@ -152,6 +152,9 @@
std::string username;
connection_->port()->CreateStunUsername(
connection_->remote_candidate().username(), &username);
+ // Note that the order of attributes does not impact the parsing on the
+ // receiver side. The attribute is retrieved then by iterating and matching
+ // over all parsed attributes. See StunMessage::GetAttribute.
request->AddAttribute(
absl::make_unique<StunByteStringAttribute>(STUN_ATTR_USERNAME, username));
@@ -167,6 +170,13 @@
request->AddAttribute(absl::make_unique<StunUInt32Attribute>(
STUN_ATTR_NETWORK_INFO, network_info));
+ if (webrtc::field_trial::IsEnabled("WebRTC-PiggybackCheckAcknowledgement") &&
+ connection_->last_ping_id_received()) {
+ request->AddAttribute(absl::make_unique<StunByteStringAttribute>(
+ STUN_ATTR_LAST_ICE_CHECK_RECEIVED,
+ connection_->last_ping_id_received().value()));
+ }
+
// Adding ICE_CONTROLLED or ICE_CONTROLLING attribute based on the role.
if (connection_->port()->GetIceRole() == ICEROLE_CONTROLLING) {
request->AddAttribute(absl::make_unique<StunUInt64Attribute>(
@@ -455,7 +465,7 @@
// request. In this case |last_ping_received_| will be updated but no
// response will be sent.
case STUN_BINDING_INDICATION:
- ReceivedPing();
+ ReceivedPing(msg->transaction_id());
break;
default:
@@ -467,7 +477,7 @@
void Connection::HandleBindingRequest(IceMessage* msg) {
// This connection should now be receiving.
- ReceivedPing();
+ ReceivedPing(msg->transaction_id());
if (webrtc::field_trial::IsEnabled("WebRTC-ExtraICEPing") &&
last_ping_response_received_ == 0) {
if (local_candidate().type() == RELAY_PORT_TYPE ||
@@ -550,6 +560,10 @@
SignalStateChange(this);
}
}
+
+ if (webrtc::field_trial::IsEnabled("WebRTC-PiggybackCheckAcknowledgement")) {
+ HandlePiggybackCheckAcknowledgementIfAny(msg);
+ }
}
void Connection::OnReadyToSend() {
@@ -678,24 +692,40 @@
num_pings_sent_++;
}
-void Connection::ReceivedPing() {
+void Connection::ReceivedPing(const absl::optional<std::string>& request_id) {
last_ping_received_ = rtc::TimeMillis();
+ last_ping_id_received_ = request_id;
UpdateReceiving(last_ping_received_);
}
-void Connection::ReceivedPingResponse(int rtt, const std::string& request_id) {
+void Connection::HandlePiggybackCheckAcknowledgementIfAny(StunMessage* msg) {
+ RTC_DCHECK(msg->type() == STUN_BINDING_REQUEST);
+ const StunByteStringAttribute* last_ice_check_received_attr =
+ msg->GetByteString(STUN_ATTR_LAST_ICE_CHECK_RECEIVED);
+ if (last_ice_check_received_attr) {
+ const std::string request_id = last_ice_check_received_attr->GetString();
+ auto iter = absl::c_find_if(
+ pings_since_last_response_,
+ [&request_id](const SentPing& ping) { return ping.id == request_id; });
+ if (iter != pings_since_last_response_.end()) {
+ const int64_t rtt = rtc::TimeMillis() - iter->sent_time;
+ ReceivedPingResponse(rtt, request_id, iter->nomination);
+ }
+ }
+}
+
+void Connection::ReceivedPingResponse(
+ int rtt,
+ const std::string& request_id,
+ const absl::optional<uint32_t>& nomination) {
RTC_DCHECK_GE(rtt, 0);
// 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.
- auto iter = absl::c_find_if(
- pings_since_last_response_,
- [request_id](const SentPing& ping) { return ping.id == request_id; });
- if (iter != pings_since_last_response_.end() &&
- iter->nomination > acked_nomination_) {
- acked_nomination_ = iter->nomination;
+ if (nomination && nomination.value() > acked_nomination_) {
+ acked_nomination_ = nomination.value();
}
total_round_trip_time_ms_ += rtt;
@@ -864,7 +894,15 @@
", rtt="
<< rtt << ", pings_since_last_response=" << pings;
}
- ReceivedPingResponse(rtt, request->id());
+ absl::optional<uint32_t> nomination;
+ const std::string request_id = request->id();
+ auto iter = absl::c_find_if(
+ pings_since_last_response_,
+ [&request_id](const SentPing& ping) { return ping.id == request_id; });
+ if (iter != pings_since_last_response_.end()) {
+ nomination.emplace(iter->nomination);
+ }
+ ReceivedPingResponse(rtt, request_id, nomination);
stats_.recv_ping_responses++;
LogCandidatePairEvent(
diff --git a/p2p/base/connection.h b/p2p/base/connection.h
index 601d34c..82b2c89 100644
--- a/p2p/base/connection.h
+++ b/p2p/base/connection.h
@@ -202,20 +202,30 @@
// Called when this connection should try checking writability again.
int64_t last_ping_sent() const { return last_ping_sent_; }
void Ping(int64_t now);
- void ReceivedPingResponse(int rtt, const std::string& request_id);
+ void ReceivedPingResponse(
+ int rtt,
+ const std::string& request_id,
+ const absl::optional<uint32_t>& nomination = absl::nullopt);
int64_t last_ping_response_received() const {
return last_ping_response_received_;
}
+ const absl::optional<std::string>& last_ping_id_received() const {
+ return last_ping_id_received_;
+ }
// Used to check if any STUN ping response has been received.
int rtt_samples() const { return rtt_samples_; }
// Called whenever a valid ping is received on this connection. This is
// public because the connection intercepts the first ping for us.
int64_t last_ping_received() const { return last_ping_received_; }
- void ReceivedPing();
+ void ReceivedPing(
+ const absl::optional<std::string>& request_id = absl::nullopt);
// Handles the binding request; sends a response if this is a valid request.
void HandleBindingRequest(IceMessage* msg);
-
+ // Handles the piggyback acknowledgement of the lastest connectivity check
+ // that the remote peer has received, if it is indicated in the incoming
+ // connectivity check from the peer.
+ void HandlePiggybackCheckAcknowledgementIfAny(StunMessage* msg);
int64_t last_data_received() const { return last_data_received_; }
// Debugging description of this connection
@@ -347,8 +357,8 @@
// The last nomination that has been acknowledged.
uint32_t acked_nomination_ = 0;
// Used by the controlled side to remember the nomination value received from
- // the controlling side. When the peer does not support ICE re-nomination,
- // its value will be 1 if the connection has been nominated.
+ // the controlling side. When the peer does not support ICE re-nomination, its
+ // value will be 1 if the connection has been nominated.
uint32_t remote_nomination_ = 0;
IceMode remote_ice_mode_;
@@ -366,6 +376,9 @@
int64_t last_ping_response_received_;
int64_t receiving_unchanged_since_ = 0;
std::vector<SentPing> pings_since_last_response_;
+ // Transaction ID of the last connectivity check received. Null if having not
+ // received a ping yet.
+ absl::optional<std::string> last_ping_id_received_;
absl::optional<int> unwritable_timeout_;
absl::optional<int> unwritable_min_checks_;
diff --git a/p2p/base/p2p_transport_channel_unittest.cc b/p2p/base/p2p_transport_channel_unittest.cc
index c5bcbdd..b97e66b 100644
--- a/p2p/base/p2p_transport_channel_unittest.cc
+++ b/p2p/base/p2p_transport_channel_unittest.cc
@@ -2256,6 +2256,55 @@
DestroyChannels();
}
+// Test that the writability can be established with the piggyback
+// acknowledgement in the connectivity check from the remote peer.
+TEST_F(P2PTransportChannelTest,
+ CanConnectWithPiggybackCheckAcknowledgementWhenCheckResponseBlocked) {
+ webrtc::test::ScopedFieldTrials field_trials(
+ "WebRTC-PiggybackCheckAcknowledgement/Enabled/");
+ rtc::ScopedFakeClock clock;
+ ConfigureEndpoints(OPEN, OPEN, kOnlyLocalPorts, kOnlyLocalPorts);
+ IceConfig ep1_config;
+ IceConfig ep2_config = CreateIceConfig(1000, GATHER_CONTINUALLY);
+ // Let ep2 be tolerable of the loss of connectivity checks, so that it keeps
+ // sending pings even after ep1 becomes unwritable as we configure the
+ // firewall below.
+ ep2_config.receiving_timeout = 30 * 1000;
+ ep2_config.ice_unwritable_timeout = 30 * 1000;
+ ep2_config.ice_unwritable_min_checks = 30;
+ ep2_config.ice_inactive_timeout = 60 * 1000;
+
+ CreateChannels(ep1_config, ep2_config);
+
+ // Wait until both sides become writable for the first time.
+ EXPECT_TRUE_SIMULATED_WAIT(
+ ep1_ch1() != nullptr && ep2_ch1() != nullptr && ep1_ch1()->receiving() &&
+ ep1_ch1()->writable() && ep2_ch1()->receiving() &&
+ ep2_ch1()->writable(),
+ kDefaultTimeout, clock);
+ // Block the ingress traffic to ep1 so that there is no check response from
+ // ep2.
+ ASSERT_NE(nullptr, LocalCandidate(ep1_ch1()));
+ fw()->AddRule(false, rtc::FP_ANY, rtc::FD_IN,
+ LocalCandidate(ep1_ch1())->address());
+ // Wait until ep1 becomes unwritable. At the same time ep2 should be still
+ // fine so that it will keep sending pings.
+ EXPECT_TRUE_SIMULATED_WAIT(ep1_ch1() != nullptr && !ep1_ch1()->writable(),
+ kDefaultTimeout, clock);
+ EXPECT_TRUE(ep2_ch1() != nullptr && ep2_ch1()->writable());
+ // Now let the pings from ep2 to flow but block any pings from ep1, so that
+ // ep1 can only become writable again after receiving an incoming ping from
+ // ep2 with piggyback acknowledgement of its previously sent pings. Note
+ // though that ep1 should have stopped sending pings after becoming unwritable
+ // in the current design.
+ fw()->ClearRules();
+ fw()->AddRule(false, rtc::FP_ANY, rtc::FD_OUT,
+ LocalCandidate(ep1_ch1())->address());
+ EXPECT_TRUE_SIMULATED_WAIT(ep1_ch1() != nullptr && ep1_ch1()->writable(),
+ kDefaultTimeout, clock);
+ DestroyChannels();
+}
+
// Test what happens when we have 2 users behind the same NAT. This can lead
// to interesting behavior because the STUN server will only give out the
// address of the outermost NAT.
@@ -3187,10 +3236,12 @@
++selected_candidate_pair_switches_;
}
- void ReceivePingOnConnection(Connection* conn,
- const std::string& remote_ufrag,
- int priority,
- uint32_t nomination = 0) {
+ void ReceivePingOnConnection(
+ Connection* conn,
+ const std::string& remote_ufrag,
+ int priority,
+ uint32_t nomination,
+ const absl::optional<std::string>& piggyback_ping_id) {
IceMessage msg;
msg.SetType(STUN_BINDING_REQUEST);
msg.AddAttribute(absl::make_unique<StunByteStringAttribute>(
@@ -3202,6 +3253,10 @@
msg.AddAttribute(absl::make_unique<StunUInt32Attribute>(
STUN_ATTR_NOMINATION, nomination));
}
+ if (piggyback_ping_id) {
+ msg.AddAttribute(absl::make_unique<StunByteStringAttribute>(
+ STUN_ATTR_LAST_ICE_CHECK_RECEIVED, piggyback_ping_id.value()));
+ }
msg.SetTransactionID(rtc::CreateRandomString(kStunTransactionIdLength));
msg.AddMessageIntegrity(conn->local_candidate().password());
msg.AddFingerprint();
@@ -3210,6 +3265,14 @@
conn->OnReadPacket(buf.Data(), buf.Length(), rtc::TimeMicros());
}
+ void ReceivePingOnConnection(Connection* conn,
+ const std::string& remote_ufrag,
+ int priority,
+ uint32_t nomination = 0) {
+ ReceivePingOnConnection(conn, remote_ufrag, priority, nomination,
+ absl::nullopt);
+ }
+
void OnReadyToSend(rtc::PacketTransportInternal* transport) {
channel_ready_to_send_ = true;
}
diff --git a/p2p/base/stun.cc b/p2p/base/stun.cc
index 3a44909..f40395b 100644
--- a/p2p/base/stun.cc
+++ b/p2p/base/stun.cc
@@ -465,6 +465,8 @@
return STUN_VALUE_BYTE_STRING;
case STUN_ATTR_RETRANSMIT_COUNT:
return STUN_VALUE_UINT32;
+ case STUN_ATTR_LAST_ICE_CHECK_RECEIVED:
+ return STUN_VALUE_BYTE_STRING;
default:
return STUN_VALUE_UNKNOWN;
}
diff --git a/p2p/base/stun.h b/p2p/base/stun.h
index 33410b5..5b9b953 100644
--- a/p2p/base/stun.h
+++ b/p2p/base/stun.h
@@ -594,16 +594,26 @@
StunMessage* CreateNew() const override;
};
-// RFC 5245 ICE STUN attributes.
enum IceAttributeType {
+ // RFC 5245 ICE STUN attributes.
STUN_ATTR_PRIORITY = 0x0024, // UInt32
STUN_ATTR_USE_CANDIDATE = 0x0025, // No content, Length = 0
STUN_ATTR_ICE_CONTROLLED = 0x8029, // UInt64
STUN_ATTR_ICE_CONTROLLING = 0x802A, // UInt64
- STUN_ATTR_NOMINATION = 0xC001, // UInt32
+ // The following attributes are in the comprehension-optional range
+ // (0xC000-0xFFFF) and are not registered with IANA. These STUN attributes are
+ // intended for ICE and should NOT be used in generic use cases of STUN
+ // messages.
+ //
+ // Note that the value 0xC001 has already been assigned by IANA to
+ // ENF-FLOW-DESCRIPTION
+ // (https://www.iana.org/assignments/stun-parameters/stun-parameters.xml).
+ STUN_ATTR_NOMINATION = 0xC001, // UInt32
// UInt32. The higher 16 bits are the network ID. The lower 16 bits are the
// network cost.
- STUN_ATTR_NETWORK_INFO = 0xC057
+ STUN_ATTR_NETWORK_INFO = 0xC057,
+ // Experimental: Transaction ID of the last connectivity check received.
+ STUN_ATTR_LAST_ICE_CHECK_RECEIVED = 0xC058,
};
// RFC 5245-defined errors.
