Simply SetRetransmissionAlarm by consolidating more of the changes from cl/381566145 and cl/314387174 into SetRetransmissionAlarm to ensure the logic is applied consistently. (This change is based on Ian's cl/381657413) Protected by FLAGS_quic_reloadable_flag_quic_simplify_set_retransmission_alarm. PiperOrigin-RevId: 439976832
diff --git a/quic/core/quic_connection.cc b/quic/core/quic_connection.cc index 0716c52..510e096 100644 --- a/quic/core/quic_connection.cc +++ b/quic/core/quic_connection.cc
@@ -1942,6 +1942,7 @@ bool QuicConnection::ShouldSetRetransmissionAlarmOnPacketSent( bool in_flight, EncryptionLevel level) const { + QUICHE_DCHECK(!sent_packet_manager_.simplify_set_retransmission_alarm()); if (!retransmission_alarm_->IsSet()) { return true; } @@ -3660,8 +3661,13 @@ return true; } } - if (ShouldSetRetransmissionAlarmOnPacketSent(in_flight, - packet->encryption_level)) { + if (sent_packet_manager_.simplify_set_retransmission_alarm()) { + QUIC_RELOADABLE_FLAG_COUNT_N(quic_simplify_set_retransmission_alarm, 1, 2); + if (in_flight || !retransmission_alarm_->IsSet()) { + SetRetransmissionAlarm(); + } + } else if (ShouldSetRetransmissionAlarmOnPacketSent( + in_flight, packet->encryption_level)) { SetRetransmissionAlarm(); } SetPingAlarm(); @@ -4334,8 +4340,11 @@ undecryptable_packets_.emplace_back(packet, decryption_level, last_received_packet_info_); if (perspective_ == Perspective::IS_CLIENT) { - if (!retransmission_alarm_->IsSet() || - GetRetransmissionDeadline() < retransmission_alarm_->deadline()) { + if (sent_packet_manager_.simplify_set_retransmission_alarm()) { + SetRetransmissionAlarm(); + } else if (!retransmission_alarm_->IsSet() || + GetRetransmissionDeadline() < + retransmission_alarm_->deadline()) { // Re-arm PTO only if we can make it sooner to speed up recovery. SetRetransmissionAlarm(); } @@ -4403,8 +4412,12 @@ undecryptable_packets_.clear(); } if (perspective_ == Perspective::IS_CLIENT) { - if (!retransmission_alarm_->IsSet() || undecryptable_packets_.empty() || - GetRetransmissionDeadline() < retransmission_alarm_->deadline()) { + if (sent_packet_manager_.simplify_set_retransmission_alarm()) { + SetRetransmissionAlarm(); + } else if (!retransmission_alarm_->IsSet() || + undecryptable_packets_.empty() || + GetRetransmissionDeadline() < + retransmission_alarm_->deadline()) { // 1) If there is still undecryptable packet, only re-arm PTO to make it // sooner to speed up recovery. // 2) If all undecryptable packets get processed, re-arm (which may @@ -4760,6 +4773,27 @@ retransmission_alarm_->Cancel(); return; } + PacketNumberSpace packet_number_space; + if (sent_packet_manager_.simplify_set_retransmission_alarm() && + SupportsMultiplePacketNumberSpaces() && !IsHandshakeConfirmed() && + !sent_packet_manager_ + .GetEarliestPacketSentTimeForPto(&packet_number_space) + .IsInitialized()) { + // Before handshake gets confirmed, GetEarliestPacketSentTimeForPto + // returning 0 indicates no packets are in flight or only application data + // is in flight. + QUIC_RELOADABLE_FLAG_COUNT_N(quic_simplify_set_retransmission_alarm, 2, 2); + if (perspective_ == Perspective::IS_SERVER) { + // No need to arm PTO on server side. + retransmission_alarm_->Cancel(); + return; + } + if (retransmission_alarm_->IsSet() && + GetRetransmissionDeadline() > retransmission_alarm_->deadline()) { + // Do not postpone armed PTO on the client side. + return; + } + } retransmission_alarm_->Update(GetRetransmissionDeadline(), kAlarmGranularity); }
diff --git a/quic/core/quic_connection_test.cc b/quic/core/quic_connection_test.cc index 6cab381..6f7b4e2 100644 --- a/quic/core/quic_connection_test.cc +++ b/quic/core/quic_connection_test.cc
@@ -13473,7 +13473,6 @@ TEST_P(QuicConnectionTest, MigratePath) { EXPECT_CALL(visitor_, GetHandshakeState()) - .Times(testing::AtMost(2)) .WillRepeatedly(Return(HANDSHAKE_CONFIRMED)); EXPECT_CALL(visitor_, OnPathDegrading()); connection_.OnPathDegradingDetected(); @@ -15778,6 +15777,58 @@ "Already sent connection close"); } +// Regression test for b/157895910. +TEST_P(QuicConnectionTest, EarliestSentTimeNotInitializedWhenPtoFires) { + if (!connection_.SupportsMultiplePacketNumberSpaces()) { + 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); + 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)); + connection_.SetEncrypter(ENCRYPTION_FORWARD_SECURE, + std::make_unique<TaggingEncrypter>(0x04)); + { + 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 half RTT data. + connection_.SetDefaultEncryptionLevel(ENCRYPTION_FORWARD_SECURE); + connection_.SendStreamDataWithString(0, std::string(2000, 'b'), 0, FIN); + } + + // Received ACKs for both INITIAL and HANDSHAKE packets. + EXPECT_CALL(*send_algorithm_, OnCongestionEvent(_, _, _, _, _)) + .Times(AnyNumber()); + QuicFrames frames1; + QuicAckFrame ack_frame1 = InitAckFrame(1); + frames1.push_back(QuicFrame(&ack_frame1)); + + QuicFrames frames2; + QuicAckFrame ack_frame2 = + InitAckFrame({{QuicPacketNumber(2), QuicPacketNumber(3)}}); + frames2.push_back(QuicFrame(&ack_frame2)); + ProcessCoalescedPacket( + {{2, frames1, ENCRYPTION_INITIAL}, {3, frames2, ENCRYPTION_HANDSHAKE}}); + // Verify PTO is not armed given the only outstanding data is half RTT data. + EXPECT_FALSE(connection_.GetRetransmissionAlarm()->IsSet()); +} + } // namespace } // namespace test } // namespace quic
diff --git a/quic/core/quic_flags_list.h b/quic/core/quic_flags_list.h index ac0e7d9..66d5dc9 100644 --- a/quic/core/quic_flags_list.h +++ b/quic/core/quic_flags_list.h
@@ -37,6 +37,8 @@ QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_allow_client_enabled_bbr_v2, false) // If true, close read side but not write side in QuicSpdyStream::OnStreamReset(). QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_fix_on_stream_reset, true) +// If true, consolidate more logic into SetRetransmissionAlarm to ensure the logic is applied consistently. +QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_simplify_set_retransmission_alarm, true) // If true, default on PTO which unifies TLP + RTO loss recovery. QUIC_FLAG(FLAGS_quic_restart_flag_quic_default_on_pto2, true) // If true, default-enable 5RTO blachole detection.
diff --git a/quic/core/quic_sent_packet_manager.cc b/quic/core/quic_sent_packet_manager.cc index f7e91b9..6fabf5b 100644 --- a/quic/core/quic_sent_packet_manager.cc +++ b/quic/core/quic_sent_packet_manager.cc
@@ -1292,7 +1292,8 @@ return QuicTime::Zero(); } PacketNumberSpace packet_number_space; - if (supports_multiple_packet_number_spaces() && + if (!simplify_set_retransmission_alarm_ && + supports_multiple_packet_number_spaces() && unacked_packets_.perspective() == Perspective::IS_SERVER && !GetEarliestPacketSentTimeForPto(&packet_number_space).IsInitialized()) { // Do not set the timer on the server side if the only in flight packets are
diff --git a/quic/core/quic_sent_packet_manager.h b/quic/core/quic_sent_packet_manager.h index 22067ff..f31cc1c 100644 --- a/quic/core/quic_sent_packet_manager.h +++ b/quic/core/quic_sent_packet_manager.h
@@ -495,6 +495,10 @@ bool use_lower_min_irtt() const { return use_lower_min_irtt_; } + bool simplify_set_retransmission_alarm() const { + return simplify_set_retransmission_alarm_; + } + private: friend class test::QuicConnectionPeer; friend class test::QuicSentPacketManagerPeer; @@ -770,6 +774,9 @@ // Latched value of --quic_use_lower_min_for_trusted_irtt. bool use_lower_min_irtt_ = GetQuicReloadableFlag(quic_use_lower_min_for_trusted_irtt); + + const bool simplify_set_retransmission_alarm_ = + GetQuicReloadableFlag(quic_simplify_set_retransmission_alarm); }; } // namespace quic
diff --git a/quic/core/quic_sent_packet_manager_test.cc b/quic/core/quic_sent_packet_manager_test.cc index bf141e4..6a33fda 100644 --- a/quic/core/quic_sent_packet_manager_test.cc +++ b/quic/core/quic_sent_packet_manager_test.cc
@@ -4167,6 +4167,9 @@ // Regression test for b/157895910. TEST_F(QuicSentPacketManagerTest, EarliestSentTimeNotInitializedWhenPtoFires) { + if (GetQuicReloadableFlag(quic_simplify_set_retransmission_alarm)) { + return; + } manager_.EnableMultiplePacketNumberSpacesSupport(); EXPECT_CALL(*send_algorithm_, PacingRate(_)) .WillRepeatedly(Return(QuicBandwidth::Zero()));