While sending PATH_CHALLENGE, make sure QuicConnection flushes probing packet on alternative path before bundling ACK.
Protected by quic_reloadable_flag_quic_not_bundle_ack_on_alternative_path.
PiperOrigin-RevId: 442816394
diff --git a/quiche/quic/core/quic_connection.cc b/quiche/quic/core/quic_connection.cc
index c5edb1d..44a7830 100644
--- a/quiche/quic/core/quic_connection.cc
+++ b/quiche/quic/core/quic_connection.cc
@@ -34,6 +34,7 @@
#include "quiche/quic/core/quic_legacy_version_encapsulator.h"
#include "quiche/quic/core/quic_packet_creator.h"
#include "quiche/quic/core/quic_packet_writer.h"
+#include "quiche/quic/core/quic_packets.h"
#include "quiche/quic/core/quic_path_validator.h"
#include "quiche/quic/core/quic_types.h"
#include "quiche/quic/core/quic_utils.h"
@@ -210,6 +211,20 @@
return kCubicBytes;
}
+bool ContainsNonProbingFrame(const SerializedPacket& packet) {
+ for (const QuicFrame& frame : packet.nonretransmittable_frames) {
+ if (!QuicUtils::IsProbingFrame(frame.type)) {
+ return true;
+ }
+ }
+ for (const QuicFrame& frame : packet.retransmittable_frames) {
+ if (!QuicUtils::IsProbingFrame(frame.type)) {
+ return true;
+ }
+ }
+ return false;
+}
+
} // namespace
#define ENDPOINT \
@@ -3360,7 +3375,18 @@
WriteResult result(WRITE_STATUS_OK, encrypted_length);
QuicSocketAddress send_to_address = packet->peer_address;
// Self address is always the default self address on this code path.
- bool send_on_current_path = send_to_address == peer_address();
+ const bool send_on_current_path = send_to_address == peer_address();
+ if (!send_on_current_path && only_send_probing_frames_on_alternative_path_) {
+ QUIC_RELOADABLE_FLAG_COUNT_N(quic_not_bundle_ack_on_alternative_path, 2, 2);
+ QUIC_BUG_IF(quic_send_non_probing_frames_on_alternative_path,
+ ContainsNonProbingFrame(*packet))
+ << "Packet " << packet->packet_number
+ << " with non-probing frames was sent on alternative path: "
+ "nonretransmittable_frames: "
+ << QuicFramesToString(packet->nonretransmittable_frames)
+ << " retransmittable_frames: "
+ << QuicFramesToString(packet->retransmittable_frames);
+ }
switch (fate) {
case DISCARD:
++stats_.packets_discarded;
@@ -6417,28 +6443,60 @@
return connected_;
}
if (connection_migration_use_new_cid_) {
- {
- QuicConnectionId client_cid, server_cid;
- FindOnPathConnectionIds(self_address, effective_peer_address, &client_cid,
- &server_cid);
+ if (!only_send_probing_frames_on_alternative_path_) {
+ {
+ QuicConnectionId client_cid, server_cid;
+ FindOnPathConnectionIds(self_address, effective_peer_address,
+ &client_cid, &server_cid);
+ QuicPacketCreator::ScopedPeerAddressContext context(
+ &packet_creator_, peer_address, client_cid, server_cid,
+ connection_migration_use_new_cid_);
+ if (writer == writer_) {
+ ScopedPacketFlusher flusher(this);
+ // It's on current path, add the PATH_CHALLENGE the same way as other
+ // frames. This may cause connection to be closed.
+ packet_creator_.AddPathChallengeFrame(data_buffer);
+ } else {
+ std::unique_ptr<SerializedPacket> probing_packet =
+ packet_creator_.SerializePathChallengeConnectivityProbingPacket(
+ data_buffer);
+ QUICHE_DCHECK_EQ(IsRetransmittable(*probing_packet),
+ NO_RETRANSMITTABLE_DATA);
+ QUICHE_DCHECK_EQ(self_address, alternative_path_.self_address);
+ WritePacketUsingWriter(std::move(probing_packet), writer,
+ self_address, peer_address,
+ /*measure_rtt=*/false);
+ }
+ }
+ return connected_;
+ }
+ QUIC_RELOADABLE_FLAG_COUNT_N(quic_not_bundle_ack_on_alternative_path, 1, 2);
+ QuicConnectionId client_cid, server_cid;
+ FindOnPathConnectionIds(self_address, effective_peer_address, &client_cid,
+ &server_cid);
+ if (writer == writer_) {
+ ScopedPacketFlusher flusher(this);
+ {
+ QuicPacketCreator::ScopedPeerAddressContext context(
+ &packet_creator_, peer_address, client_cid, server_cid,
+ connection_migration_use_new_cid_);
+ // It's using the default writer, add the PATH_CHALLENGE the same way as
+ // other frames. This may cause connection to be closed.
+ packet_creator_.AddPathChallengeFrame(data_buffer);
+ }
+ } else {
+ // Switch to the right CID and source/peer addresses.
QuicPacketCreator::ScopedPeerAddressContext context(
&packet_creator_, peer_address, client_cid, server_cid,
connection_migration_use_new_cid_);
- if (writer == writer_) {
- ScopedPacketFlusher flusher(this);
- // It's on current path, add the PATH_CHALLENGE the same way as other
- // frames. This may cause connection to be closed.
- packet_creator_.AddPathChallengeFrame(data_buffer);
- } else {
- std::unique_ptr<SerializedPacket> probing_packet =
- packet_creator_.SerializePathChallengeConnectivityProbingPacket(
- data_buffer);
- QUICHE_DCHECK_EQ(IsRetransmittable(*probing_packet),
- NO_RETRANSMITTABLE_DATA);
- QUICHE_DCHECK_EQ(self_address, alternative_path_.self_address);
- WritePacketUsingWriter(std::move(probing_packet), writer, self_address,
- peer_address, /*measure_rtt=*/false);
- }
+ std::unique_ptr<SerializedPacket> probing_packet =
+ packet_creator_.SerializePathChallengeConnectivityProbingPacket(
+ data_buffer);
+ QUICHE_DCHECK_EQ(IsRetransmittable(*probing_packet),
+ NO_RETRANSMITTABLE_DATA);
+ QUICHE_DCHECK_EQ(self_address, alternative_path_.self_address);
+ WritePacketUsingWriter(std::move(probing_packet), writer, self_address,
+ peer_address, /*measure_rtt=*/false);
}
return connected_;
}
diff --git a/quiche/quic/core/quic_connection.h b/quiche/quic/core/quic_connection.h
index 72b3fdd..6aca00b 100644
--- a/quiche/quic/core/quic_connection.h
+++ b/quiche/quic/core/quic_connection.h
@@ -2251,6 +2251,9 @@
// fixed.
absl::optional<QuicWallTime> quic_bug_10511_43_timestamp_;
std::string quic_bug_10511_43_error_detail_;
+
+ bool only_send_probing_frames_on_alternative_path_ =
+ GetQuicReloadableFlag(quic_not_bundle_ack_on_alternative_path);
};
} // namespace quic
diff --git a/quiche/quic/core/quic_connection_test.cc b/quiche/quic/core/quic_connection_test.cc
index 522c78e..ac4ad7c 100644
--- a/quiche/quic/core/quic_connection_test.cc
+++ b/quiche/quic/core/quic_connection_test.cc
@@ -14037,6 +14037,123 @@
EXPECT_EQ(1u, connection_.GetStats().num_validated_peer_migration);
}
+// Regression test of b/228645208.
+TEST_P(QuicConnectionTest, NoNonProbingFrameOnAlternativePath) {
+ if (!connection_.connection_migration_use_new_cid()) {
+ return;
+ }
+
+ PathProbeTestInit(Perspective::IS_SERVER);
+ SetClientConnectionId(TestConnectionId(1));
+ connection_.CreateConnectionIdManager();
+
+ QuicConnectionId server_cid0 = connection_.connection_id();
+ QuicConnectionId client_cid0 = connection_.client_connection_id();
+ QuicConnectionId client_cid1 = TestConnectionId(2);
+ QuicConnectionId server_cid1;
+ // Sends new server CID to client.
+ EXPECT_CALL(visitor_, OnServerConnectionIdIssued(_))
+ .WillOnce(
+ Invoke([&](const QuicConnectionId& cid) { server_cid1 = cid; }));
+ EXPECT_CALL(visitor_, SendNewConnectionId(_));
+ connection_.MaybeSendConnectionIdToClient();
+ // Receives new client CID from client.
+ QuicNewConnectionIdFrame new_cid_frame;
+ new_cid_frame.connection_id = client_cid1;
+ new_cid_frame.sequence_number = 1u;
+ new_cid_frame.retire_prior_to = 0u;
+ connection_.OnNewConnectionIdFrame(new_cid_frame);
+ auto* packet_creator = QuicConnectionPeer::GetPacketCreator(&connection_);
+ ASSERT_EQ(packet_creator->GetDestinationConnectionId(), client_cid0);
+ ASSERT_EQ(packet_creator->GetSourceConnectionId(), server_cid0);
+
+ peer_creator_.SetServerConnectionId(server_cid1);
+ const QuicSocketAddress kNewPeerAddress =
+ QuicSocketAddress(QuicIpAddress::Loopback4(), /*port=*/23456);
+ QuicPathFrameBuffer path_challenge_payload{0, 1, 2, 3, 4, 5, 6, 7};
+ QuicFrames frames1;
+ frames1.push_back(
+ QuicFrame(QuicPathChallengeFrame(0, path_challenge_payload)));
+ QuicPathFrameBuffer payload;
+ EXPECT_CALL(*send_algorithm_,
+ OnPacketSent(_, _, _, _, NO_RETRANSMITTABLE_DATA))
+ .Times(AtLeast(1))
+ .WillOnce(Invoke([&]() {
+ EXPECT_EQ(kNewPeerAddress, writer_->last_write_peer_address());
+ EXPECT_EQ(kPeerAddress, connection_.peer_address());
+ EXPECT_EQ(kPeerAddress, connection_.effective_peer_address());
+ EXPECT_FALSE(writer_->path_response_frames().empty());
+ EXPECT_FALSE(writer_->path_challenge_frames().empty());
+ payload = writer_->path_challenge_frames().front().data_buffer;
+ }));
+ ProcessFramesPacketWithAddresses(frames1, kSelfAddress, kNewPeerAddress,
+ ENCRYPTION_FORWARD_SECURE);
+ EXPECT_EQ(kPeerAddress, connection_.peer_address());
+ EXPECT_EQ(kPeerAddress, connection_.effective_peer_address());
+ EXPECT_TRUE(connection_.HasPendingPathValidation());
+ const auto* default_path = QuicConnectionPeer::GetDefaultPath(&connection_);
+ const auto* alternative_path =
+ QuicConnectionPeer::GetAlternativePath(&connection_);
+ EXPECT_EQ(default_path->client_connection_id, client_cid0);
+ EXPECT_EQ(default_path->server_connection_id, server_cid0);
+ EXPECT_EQ(alternative_path->client_connection_id, client_cid1);
+ EXPECT_EQ(alternative_path->server_connection_id, server_cid1);
+ EXPECT_EQ(packet_creator->GetDestinationConnectionId(), client_cid0);
+ EXPECT_EQ(packet_creator->GetSourceConnectionId(), server_cid0);
+
+ // Process non-probing packets on the default path.
+ peer_creator_.SetServerConnectionId(server_cid0);
+ EXPECT_CALL(visitor_, OnStreamFrame(_)).WillRepeatedly(Invoke([=]() {
+ EXPECT_EQ(kPeerAddress, connection_.peer_address());
+ }));
+ // Receives packets 3 - 39 to send 19 ACK-only packets, which will force the
+ // connection to reach |kMaxConsecutiveNonRetransmittablePackets| while
+ // sending the next ACK.
+ for (size_t i = 3; i <= 39; ++i) {
+ ProcessDataPacket(i);
+ }
+ EXPECT_EQ(kPeerAddress, connection_.peer_address());
+ EXPECT_EQ(kPeerAddress, connection_.effective_peer_address());
+
+ EXPECT_TRUE(connection_.HasPendingAcks());
+ QuicTime ack_time = connection_.GetAckAlarm()->deadline();
+ QuicTime path_validation_retry_time =
+ connection_.GetRetryTimeout(kNewPeerAddress, writer_.get());
+ // Advance time to simultaneously fire path validation retry and ACK alarms.
+ clock_.AdvanceTime(std::max(ack_time, path_validation_retry_time) -
+ clock_.ApproximateNow());
+
+ // The 20th ACK should bundle with a WINDOW_UPDATE frame.
+ EXPECT_CALL(visitor_, OnAckNeedsRetransmittableFrame())
+ .WillOnce(Invoke([this]() {
+ connection_.SendControlFrame(QuicFrame(QuicWindowUpdateFrame(1, 0, 0)));
+ }));
+ if (GetQuicReloadableFlag(quic_not_bundle_ack_on_alternative_path)) {
+ EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, _, _, _))
+ .WillOnce(Invoke([&]() {
+ EXPECT_EQ(kNewPeerAddress, writer_->last_write_peer_address());
+ EXPECT_FALSE(writer_->path_challenge_frames().empty());
+ // Retry path validation shouldn't bundle ACK.
+ EXPECT_TRUE(writer_->ack_frames().empty());
+ }))
+ .WillOnce(Invoke([&]() {
+ EXPECT_EQ(kPeerAddress, writer_->last_write_peer_address());
+ EXPECT_FALSE(writer_->ack_frames().empty());
+ EXPECT_FALSE(writer_->window_update_frames().empty());
+ }));
+ static_cast<TestAlarmFactory::TestAlarm*>(
+ QuicPathValidatorPeer::retry_timer(
+ QuicConnectionPeer::path_validator(&connection_)))
+ ->Fire();
+ } else {
+ EXPECT_QUIC_BUG(static_cast<TestAlarmFactory::TestAlarm*>(
+ QuicPathValidatorPeer::retry_timer(
+ QuicConnectionPeer::path_validator(&connection_)))
+ ->Fire(),
+ "quic_bug_12645_2");
+ }
+}
+
TEST_P(QuicConnectionTest,
ProbedOnAnotherPathAfterPeerIpAddressChangeAtServer) {
PathProbeTestInit(Perspective::IS_SERVER);
diff --git a/quiche/quic/core/quic_flags_list.h b/quiche/quic/core/quic_flags_list.h
index 794e4f7..092ce91 100644
--- a/quiche/quic/core/quic_flags_list.h
+++ b/quiche/quic/core/quic_flags_list.h
@@ -57,6 +57,8 @@
QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_disable_server_blackhole_detection, false)
// If true, discard INITIAL packet if the key has been dropped.
QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_discard_initial_packet_with_key_dropped, true)
+// If true, do not bundle ACK while sending PATH_CHALLENGE on alternative path.
+QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_not_bundle_ack_on_alternative_path, true)
// If true, do not count bytes sent/received on the alternative path into the bytes sent/received on the default path.
QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_count_bytes_on_alternative_path_seperately, true)
// If true, enable server retransmittable on wire PING.
