Flush creator after MaybeCoalescePacketOfHigherSpace. This would allow connection to coalesce HANDSHAKE + 1-RTT packets while there is an INITIAL in the coalescer.
Protected by FLAGS_quic_reloadable_flag_quic_flush_after_coalesce_higher_space_packets.
PiperOrigin-RevId: 436835803
diff --git a/quic/core/quic_connection.cc b/quic/core/quic_connection.cc
index 5dfff49..6234489 100644
--- a/quic/core/quic_connection.cc
+++ b/quic/core/quic_connection.cc
@@ -4786,10 +4786,30 @@
connection_->SendAck();
}
}
- connection_->packet_creator_.Flush();
- if (connection_->version().CanSendCoalescedPackets()) {
- connection_->MaybeCoalescePacketOfHigherSpace();
- connection_->FlushCoalescedPacket();
+
+ if (connection_->flush_after_coalesce_higher_space_packets_) {
+ // INITIAL or HANDSHAKE retransmission could cause peer to derive new
+ // keys, such that the buffered undecryptable packets may be processed.
+ // This endpoint would derive an inflated RTT sample when receiving ACKs
+ // of those undecryptable packets. To mitigate this, tries to coalesce as
+ // many higher space packets as possible (via for loop inside
+ // MaybeCoalescePacketOfHigherSpace) to fill the remaining space in the
+ // coalescer.
+ QUIC_RELOADABLE_FLAG_COUNT(
+ quic_flush_after_coalesce_higher_space_packets);
+ if (connection_->version().CanSendCoalescedPackets()) {
+ connection_->MaybeCoalescePacketOfHigherSpace();
+ }
+ connection_->packet_creator_.Flush();
+ if (connection_->version().CanSendCoalescedPackets()) {
+ connection_->FlushCoalescedPacket();
+ }
+ } else {
+ connection_->packet_creator_.Flush();
+ if (connection_->version().CanSendCoalescedPackets()) {
+ connection_->MaybeCoalescePacketOfHigherSpace();
+ connection_->FlushCoalescedPacket();
+ }
}
connection_->FlushPackets();
if (!handshake_packet_sent_ && connection_->handshake_packet_sent_) {
@@ -5831,16 +5851,15 @@
}
void QuicConnection::MaybeCoalescePacketOfHigherSpace() {
- if (!connected() || !packet_creator_.HasSoftMaxPacketLength() ||
- fill_coalesced_packet_) {
- // Make sure MaybeCoalescePacketOfHigherSpace is not re-entrant.
+ if (!connected() || !packet_creator_.HasSoftMaxPacketLength()) {
return;
}
- // INITIAL or HANDSHAKE retransmission could cause peer to derive new
- // keys, such that the buffered undecryptable packets may be processed.
- // This endpoint would derive an inflated RTT sample (which includes the PTO
- // timeout) when receiving ACKs of those undecryptable packets. To mitigate
- // this, tries to coalesce a packet of higher encryption level.
+ if (fill_coalesced_packet_) {
+ // Make sure MaybeCoalescePacketOfHigherSpace is not re-entrant.
+ QUIC_BUG_IF(quic_coalesce_packet_reentrant,
+ flush_after_coalesce_higher_space_packets_);
+ return;
+ }
for (EncryptionLevel retransmission_level :
{ENCRYPTION_INITIAL, ENCRYPTION_HANDSHAKE}) {
// Coalesce HANDSHAKE with INITIAL retransmission, and coalesce 1-RTT with
@@ -5854,6 +5873,9 @@
NOT_RETRANSMISSION &&
framer_.HasEncrypterOfEncryptionLevel(coalesced_level) &&
!coalesced_packet_.ContainsPacketOfEncryptionLevel(coalesced_level)) {
+ QUIC_DVLOG(1) << ENDPOINT
+ << "Trying to coalesce packet of encryption level: "
+ << EncryptionLevelToString(coalesced_level);
fill_coalesced_packet_ = true;
sent_packet_manager_.RetransmitDataOfSpaceIfAny(
QuicUtils::GetPacketNumberSpace(coalesced_level));
diff --git a/quic/core/quic_connection.h b/quic/core/quic_connection.h
index 0f7cac3..ec39ff1 100644
--- a/quic/core/quic_connection.h
+++ b/quic/core/quic_connection.h
@@ -2252,6 +2252,9 @@
// Enable this via reloadable flag once this feature is complete.
bool connection_migration_use_new_cid_ = false;
+ const bool flush_after_coalesce_higher_space_packets_ =
+ GetQuicReloadableFlag(quic_flush_after_coalesce_higher_space_packets);
+
// TODO(b/205023946) Debug-only fields, to be deprecated after the bug is
// fixed.
absl::optional<QuicWallTime> quic_bug_10511_43_timestamp_;
diff --git a/quic/core/quic_connection_test.cc b/quic/core/quic_connection_test.cc
index 2716e00..9fe34d3 100644
--- a/quic/core/quic_connection_test.cc
+++ b/quic/core/quic_connection_test.cc
@@ -15253,13 +15253,22 @@
EXPECT_EQ(clock_.Now() + kAlarmGranularity,
connection_.GetAckAlarm()->deadline());
// ACK is not throttled by amplification limit, and SHLO is bundled. Also
- // HANDSHAKE packet gets coalesced.
- EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, _, _, _)).Times(2);
+ // HANDSHAKE + 1RTT packets get coalesced.
+ if (GetQuicReloadableFlag(quic_flush_after_coalesce_higher_space_packets)) {
+ EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, _, _, _)).Times(3);
+ } else {
+ EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, _, _, _)).Times(2);
+ }
// ACK alarm fires.
clock_.AdvanceTime(kAlarmGranularity);
connection_.GetAckAlarm()->Fire();
- // Verify HANDSHAKE packet is coalesced with INITIAL ACK + SHLO.
- EXPECT_EQ(0x03030303u, writer_->final_bytes_of_last_packet());
+ if (GetQuicReloadableFlag(quic_flush_after_coalesce_higher_space_packets)) {
+ // Verify 1-RTT packet is coalesced.
+ EXPECT_EQ(0x04040404u, writer_->final_bytes_of_last_packet());
+ } else {
+ // Verify HANDSHAKE packet is coalesced with INITIAL ACK + SHLO.
+ EXPECT_EQ(0x03030303u, writer_->final_bytes_of_last_packet());
+ }
// Only the first packet in the coalesced packet has been processed,
// verify SHLO is bundled with INITIAL ACK.
EXPECT_EQ(1u, writer_->ack_frames().size());
@@ -15270,7 +15279,16 @@
writer_->framer()->ProcessPacket(*packet);
EXPECT_EQ(0u, writer_->ack_frames().size());
EXPECT_EQ(1u, writer_->crypto_frames().size());
- ASSERT_TRUE(writer_->coalesced_packet() == nullptr);
+ if (GetQuicReloadableFlag(quic_flush_after_coalesce_higher_space_packets)) {
+ // Process the coalesced 1-RTT packet.
+ ASSERT_TRUE(writer_->coalesced_packet() != nullptr);
+ packet = writer_->coalesced_packet()->Clone();
+ writer_->framer()->ProcessPacket(*packet);
+ EXPECT_EQ(0u, writer_->crypto_frames().size());
+ EXPECT_EQ(1u, writer_->stream_frames().size());
+ } else {
+ ASSERT_TRUE(writer_->coalesced_packet() == nullptr);
+ }
// Received INITIAL 3.
EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, _, _, _)).Times(AnyNumber());
@@ -15608,6 +15626,79 @@
}
}
+TEST_P(QuicConnectionTest, CoalesceOneRTTPacketWithInitialAndHandshakePackets) {
+ if (!version().HasIetfQuicFrames()) {
+ return;
+ }
+ set_perspective(Perspective::IS_SERVER);
+ EXPECT_CALL(visitor_, OnCryptoFrame(_)).Times(AnyNumber());
+ EXPECT_CALL(visitor_, OnStreamFrame(_)).Times(AnyNumber());
+ use_tagging_decrypter();
+
+ // Received INITIAL 1.
+ ProcessCryptoPacketAtLevel(1, ENCRYPTION_INITIAL);
+
+ peer_framer_.SetEncrypter(ENCRYPTION_ZERO_RTT,
+ std::make_unique<TaggingEncrypter>(0x02));
+
+ connection_.SetEncrypter(ENCRYPTION_INITIAL,
+ std::make_unique<TaggingEncrypter>(0x01));
+ connection_.SetEncrypter(ENCRYPTION_HANDSHAKE,
+ std::make_unique<TaggingEncrypter>(0x03));
+ SetDecrypter(ENCRYPTION_HANDSHAKE,
+ std::make_unique<StrictTaggingDecrypter>(0x03));
+ SetDecrypter(ENCRYPTION_ZERO_RTT,
+ std::make_unique<StrictTaggingDecrypter>(0x02));
+ connection_.SetEncrypter(ENCRYPTION_FORWARD_SECURE,
+ std::make_unique<TaggingEncrypter>(0x04));
+
+ // Received ENCRYPTION_ZERO_RTT 2.
+ ProcessDataPacketAtLevel(2, !kHasStopWaiting, ENCRYPTION_ZERO_RTT);
+
+ {
+ QuicConnection::ScopedPacketFlusher flusher(&connection_);
+ // Send INITIAL 1.
+ connection_.SetDefaultEncryptionLevel(ENCRYPTION_INITIAL);
+ connection_.SendCryptoDataWithString("foo", 0, ENCRYPTION_INITIAL);
+ // Send HANDSHAKE 2.
+ EXPECT_CALL(visitor_, OnHandshakePacketSent()).Times(1);
+ connection_.SetDefaultEncryptionLevel(ENCRYPTION_HANDSHAKE);
+ connection_.SendCryptoDataWithString(std::string(200, 'a'), 0,
+ ENCRYPTION_HANDSHAKE);
+ // Send 1-RTT data.
+ connection_.SetDefaultEncryptionLevel(ENCRYPTION_FORWARD_SECURE);
+ connection_.SendStreamDataWithString(0, std::string(2000, 'b'), 0, FIN);
+ }
+ // Verify coalesced packet [INITIAL 1 + HANDSHAKE 2 + part of 1-RTT data] +
+ // rest of 1-RTT data get sent.
+ EXPECT_EQ(2u, writer_->packets_write_attempts());
+
+ // Received ENCRYPTION_INITIAL 3.
+ ProcessDataPacketAtLevel(3, !kHasStopWaiting, ENCRYPTION_INITIAL);
+
+ // Verify a coalesced packet gets sent.
+ EXPECT_EQ(3u, writer_->packets_write_attempts());
+
+ // Only the first INITIAL packet has been processed yet.
+ EXPECT_EQ(1u, writer_->ack_frames().size());
+ EXPECT_EQ(1u, writer_->crypto_frames().size());
+
+ // Process HANDSHAKE packet.
+ ASSERT_TRUE(writer_->coalesced_packet() != nullptr);
+ auto packet = writer_->coalesced_packet()->Clone();
+ writer_->framer()->ProcessPacket(*packet);
+ EXPECT_EQ(1u, writer_->crypto_frames().size());
+ if (!GetQuicReloadableFlag(quic_flush_after_coalesce_higher_space_packets)) {
+ ASSERT_TRUE(writer_->coalesced_packet() == nullptr);
+ return;
+ }
+ // Process 1-RTT packet.
+ ASSERT_TRUE(writer_->coalesced_packet() != nullptr);
+ packet = writer_->coalesced_packet()->Clone();
+ writer_->framer()->ProcessPacket(*packet);
+ EXPECT_EQ(1u, writer_->stream_frames().size());
+}
+
} // namespace
} // namespace test
} // namespace quic
diff --git a/quic/core/quic_flags_list.h b/quic/core/quic_flags_list.h
index cb521fc..f053116 100644
--- a/quic/core/quic_flags_list.h
+++ b/quic/core/quic_flags_list.h
@@ -65,6 +65,8 @@
QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_count_bytes_on_alternative_path_seperately, true)
// If true, enable server retransmittable on wire PING.
QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_enable_server_on_wire_ping, true)
+// If true, flush creator after coalesce packet of higher space.
+QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_flush_after_coalesce_higher_space_packets, true)
// If true, flush pending frames as well as pending padding bytes on connection migration.
QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_flush_pending_frames_and_padding_bytes_on_migration, true)
// If true, ietf connection migration is no longer conditioned on connection option RVCM.
