In QUIC, do not check amplification limit if there is pending timer credit. This would guarantee CRYPTO frame be retransmitted because of 1) PTO fires 2) bundled with outgoing ACKs. Protected by FLAGS_quic_reloadable_flag_quic_donot_check_amplification_limit_with_pending_timer_credit. PiperOrigin-RevId: 408392907
diff --git a/quic/core/quic_connection.cc b/quic/core/quic_connection.cc index 7c4f004..87b1c86 100644 --- a/quic/core/quic_connection.cc +++ b/quic/core/quic_connection.cc
@@ -3300,7 +3300,12 @@ return packet_creator_.HasSoftMaxPacketLength(); } - if (LimitedByAmplificationFactor()) { + const bool donot_check_amplification_limit_with_pending_timer_credit = + GetQuicReloadableFlag( + quic_donot_check_amplification_limit_with_pending_timer_credit); + + if (!donot_check_amplification_limit_with_pending_timer_credit && + LimitedByAmplificationFactor()) { // Server is constrained by the amplification restriction. QUIC_CODE_COUNT(quic_throttled_by_amplification_limit); QUIC_DVLOG(1) << ENDPOINT @@ -3314,10 +3319,31 @@ } if (sent_packet_manager_.pending_timer_transmission_count() > 0) { - // Force sending the retransmissions for HANDSHAKE, TLP, RTO, PROBING cases. + // Allow sending if there are pending tokens, which occurs when: + // 1) firing PTO, + // 2) bundling CRYPTO data with ACKs, + // 3) coalescing CRYPTO data of higher space. return true; } + if (donot_check_amplification_limit_with_pending_timer_credit) { + QUIC_RELOADABLE_FLAG_COUNT( + quic_donot_check_amplification_limit_with_pending_timer_credit); + if (LimitedByAmplificationFactor()) { + // Server is constrained by the amplification restriction. + QUIC_CODE_COUNT(quic_throttled_by_amplification_limit); + QUIC_DVLOG(1) + << ENDPOINT + << "Constrained by amplification restriction to peer address " + << default_path_.peer_address << " bytes received " + << default_path_.bytes_received_before_address_validation + << ", bytes sent" + << default_path_.bytes_sent_before_address_validation; + ++stats_.num_amplification_throttling; + return false; + } + } + if (HandleWriteBlocked()) { return false; }
diff --git a/quic/core/quic_connection_test.cc b/quic/core/quic_connection_test.cc index a6ed2a1..8879a32 100644 --- a/quic/core/quic_connection_test.cc +++ b/quic/core/quic_connection_test.cc
@@ -763,7 +763,6 @@ std::unique_ptr<QuicDecrypter> decrypter) { if (connection_.version().KnowsWhichDecrypterToUse()) { connection_.InstallDecrypter(level, std::move(decrypter)); - connection_.RemoveDecrypter(ENCRYPTION_INITIAL); } else { connection_.SetDecrypter(level, std::move(decrypter)); } @@ -898,7 +897,6 @@ connection_.InstallDecrypter( QuicPacketCreatorPeer::GetEncryptionLevel(&peer_creator_), std::make_unique<StrictTaggingDecrypter>(0x01)); - connection_.RemoveDecrypter(ENCRYPTION_INITIAL); } else { connection_.SetDecrypter( QuicPacketCreatorPeer::GetEncryptionLevel(&peer_creator_), @@ -15560,6 +15558,94 @@ } } +// Regression test for b/201643321. +TEST_P(QuicConnectionTest, FailedToRetransmitShlo) { + 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); + EXPECT_TRUE(connection_.HasPendingAcks()); + + 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>(0x02)); + SetDecrypter(ENCRYPTION_ZERO_RTT, + std::make_unique<StrictTaggingDecrypter>(0x02)); + connection_.SetEncrypter(ENCRYPTION_FORWARD_SECURE, + std::make_unique<TaggingEncrypter>(0x04)); + // Received ENCRYPTION_ZERO_RTT 1. + ProcessDataPacketAtLevel(1, !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("foo", 0, ENCRYPTION_HANDSHAKE); + connection_.SetDefaultEncryptionLevel(ENCRYPTION_FORWARD_SECURE); + // Send half RTT data to exhaust amplification credit. + connection_.SendStreamDataWithString(0, std::string(100 * 1024, 'a'), 0, + NO_FIN); + } + // Received INITIAL 2. + ProcessCryptoPacketAtLevel(2, ENCRYPTION_INITIAL); + ASSERT_TRUE(connection_.HasPendingAcks()); + // Verify ACK delay is 1ms. + EXPECT_EQ(clock_.Now() + kAlarmGranularity, + connection_.GetAckAlarm()->deadline()); + if (!GetQuicReloadableFlag( + quic_donot_check_amplification_limit_with_pending_timer_credit)) { + // ACK is not sent because of amplification limit throttled. + EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, _, _, _)).Times(0); + } else { + // ACK is not throttled by amplification limit, and SHLO is bundled. Also + // HANDSHAKE packet gets coalesced. + EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, _, _, _)).Times(2); + } + // ACK alarm fires. + clock_.AdvanceTime(kAlarmGranularity); + connection_.GetAckAlarm()->Fire(); + if (GetQuicReloadableFlag( + quic_donot_check_amplification_limit_with_pending_timer_credit)) { + // 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()); + EXPECT_EQ(1u, writer_->crypto_frames().size()); + // Process the coalesced HANDSHAKE packet. + ASSERT_TRUE(writer_->coalesced_packet() != nullptr); + auto packet = writer_->coalesced_packet()->Clone(); + writer_->framer()->ProcessPacket(*packet); + EXPECT_EQ(0u, writer_->ack_frames().size()); + EXPECT_EQ(1u, writer_->crypto_frames().size()); + ASSERT_TRUE(writer_->coalesced_packet() == nullptr); + } + + // Received INITIAL 3. + EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, _, _, _)).Times(AnyNumber()); + ProcessCryptoPacketAtLevel(3, ENCRYPTION_INITIAL); + if (!GetQuicReloadableFlag( + quic_donot_check_amplification_limit_with_pending_timer_credit)) { + EXPECT_FALSE(connection_.HasPendingAcks()); + } else { + EXPECT_TRUE(connection_.HasPendingAcks()); + } +} + } // namespace } // namespace test } // namespace quic
diff --git a/quic/core/quic_flags_list.h b/quic/core/quic_flags_list.h index 5105d90..d625fd0 100644 --- a/quic/core/quic_flags_list.h +++ b/quic/core/quic_flags_list.h
@@ -15,6 +15,8 @@ QUIC_FLAG(FLAGS_quic_restart_flag_quic_testonly_default_false, false) // A testonly restart flag that will always default to true. QUIC_FLAG(FLAGS_quic_restart_flag_quic_testonly_default_true, true) +// Donot check amplification limit if there is available pending_timer_transmission_count. +QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_donot_check_amplification_limit_with_pending_timer_credit, false) // If bytes in flight has dipped below 1.25*MaxBW in the last round, do not exit PROBE_UP due to excess queue buildup. QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_bbr2_no_probe_up_exit_if_no_queue, true) // If true and connection option B201 is used, check if cwnd limited before aggregation epoch, instead of ack event, in PROBE_UP phase.