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.