gfe-relnote: Deprecate gfe2_reloadable_flag_quic_fix_rto_retransmission3. PiperOrigin-RevId: 275103541 Change-Id: I316da0f4640701d44078a2413619069b220d834b
diff --git a/quic/core/quic_connection.cc b/quic/core/quic_connection.cc index 0b4c05f..079e14d 100644 --- a/quic/core/quic_connection.cc +++ b/quic/core/quic_connection.cc
@@ -2712,7 +2712,7 @@ WriteIfNotBlocked(); } - if (sent_packet_manager_.fix_rto_retransmission()) { + if (session_decides_what_to_write()) { if (packet_generator_.packet_number() == previous_created_packet_number && (retransmission_mode == QuicSentPacketManager::TLP_MODE || retransmission_mode == QuicSentPacketManager::RTO_MODE ||
diff --git a/quic/core/quic_connection_test.cc b/quic/core/quic_connection_test.cc index 3d923a2..20f63f7 100644 --- a/quic/core/quic_connection_test.cc +++ b/quic/core/quic_connection_test.cc
@@ -3495,8 +3495,7 @@ // Ensure that the data is still in flight, but the retransmission alarm is no // longer set. EXPECT_GT(manager_->GetBytesInFlight(), 0u); - if (QuicConnectionPeer::GetSentPacketManager(&connection_) - ->fix_rto_retransmission()) { + if (connection_.session_decides_what_to_write()) { EXPECT_TRUE(connection_.GetRetransmissionAlarm()->IsSet()); } else { EXPECT_FALSE(connection_.GetRetransmissionAlarm()->IsSet()); @@ -3779,8 +3778,7 @@ writer_->SetWritable(); connection_.OnCanWrite(); - if (QuicConnectionPeer::GetSentPacketManager(&connection_) - ->fix_rto_retransmission()) { + if (connection_.session_decides_what_to_write()) { EXPECT_TRUE(connection_.GetRetransmissionAlarm()->IsSet()); } else { EXPECT_FALSE(connection_.GetRetransmissionAlarm()->IsSet()); @@ -4243,31 +4241,14 @@ // Simulate the retransmission alarm firing. clock_.AdvanceTime(DefaultRetransmissionTime()); // RTO fires, but there is no packet to be RTOed. - if (GetQuicReloadableFlag(quic_fix_rto_retransmission3)) { - EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, _, _, _)).Times(1); - } else { - EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, _, _, _)).Times(0); - } + EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, _, _, _)).Times(1); connection_.GetRetransmissionAlarm()->Fire(); - if (GetQuicReloadableFlag(quic_fix_rto_retransmission3)) { - EXPECT_EQ(1u, writer_->rst_stream_frames().size()); - } + EXPECT_EQ(1u, writer_->rst_stream_frames().size()); EXPECT_CALL(visitor_, OnStreamFrame(_)).Times(40); EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, _, _, _)).Times(20); - if (GetQuicReloadableFlag(quic_fix_rto_retransmission3)) { - EXPECT_CALL(visitor_, WillingAndAbleToWrite()) - .WillRepeatedly(Return(false)); - } else { - EXPECT_CALL(visitor_, WillingAndAbleToWrite()).WillRepeatedly(Return(true)); - } - if (GetQuicReloadableFlag(quic_fix_rto_retransmission3)) { - EXPECT_CALL(visitor_, OnAckNeedsRetransmittableFrame()).Times(1); - } else { - // Since there is a buffered RST_STREAM, no retransmittable frame is bundled - // with ACKs. - EXPECT_CALL(visitor_, OnAckNeedsRetransmittableFrame()).Times(0); - } + EXPECT_CALL(visitor_, WillingAndAbleToWrite()).WillRepeatedly(Return(false)); + EXPECT_CALL(visitor_, OnAckNeedsRetransmittableFrame()).Times(1); // Receives packets 1 - 40. for (size_t i = 1; i <= 40; ++i) { ProcessDataPacket(i); @@ -9185,8 +9166,7 @@ // Regresstion test for b/138962304. TEST_P(QuicConnectionTest, RtoAndWriteBlocked) { - if (!QuicConnectionPeer::GetSentPacketManager(&connection_) - ->fix_rto_retransmission()) { + if (!connection_.session_decides_what_to_write()) { return; } EXPECT_FALSE(connection_.GetRetransmissionAlarm()->IsSet()); @@ -9215,8 +9195,7 @@ // Regresstion test for b/138962304. TEST_P(QuicConnectionTest, TlpAndWriteBlocked) { - if (!QuicConnectionPeer::GetSentPacketManager(&connection_) - ->fix_rto_retransmission()) { + if (!connection_.session_decides_what_to_write()) { return; } EXPECT_FALSE(connection_.GetRetransmissionAlarm()->IsSet()); @@ -9249,8 +9228,7 @@ // Regresstion test for b/139375344. TEST_P(QuicConnectionTest, RtoForcesSendingPing) { - if (!QuicConnectionPeer::GetSentPacketManager(&connection_) - ->fix_rto_retransmission() || + if (!connection_.session_decides_what_to_write() || connection_.PtoEnabled()) { return; } @@ -9289,12 +9267,10 @@ } TEST_P(QuicConnectionTest, ProbeTimeout) { - if (!connection_.session_decides_what_to_write() || - !GetQuicReloadableFlag(quic_fix_rto_retransmission3)) { + if (!connection_.session_decides_what_to_write()) { return; } SetQuicReloadableFlag(quic_enable_pto, true); - SetQuicReloadableFlag(quic_fix_rto_retransmission3, true); QuicConfig config; QuicTagVector connection_options; connection_options.push_back(k2PTO); @@ -9322,8 +9298,7 @@ } TEST_P(QuicConnectionTest, CloseConnectionAfter6ClientPTOs) { - if (!connection_.session_decides_what_to_write() || - !GetQuicReloadableFlag(quic_fix_rto_retransmission3)) { + if (!connection_.session_decides_what_to_write()) { return; } SetQuicReloadableFlag(quic_enable_pto, true); @@ -9364,8 +9339,7 @@ } TEST_P(QuicConnectionTest, CloseConnectionAfter7ClientPTOs) { - if (!connection_.session_decides_what_to_write() || - !GetQuicReloadableFlag(quic_fix_rto_retransmission3)) { + if (!connection_.session_decides_what_to_write()) { return; } SetQuicReloadableFlag(quic_enable_pto, true); @@ -9405,8 +9379,7 @@ } TEST_P(QuicConnectionTest, CloseConnectionAfter8ClientPTOs) { - if (!connection_.session_decides_what_to_write() || - !GetQuicReloadableFlag(quic_fix_rto_retransmission3)) { + if (!connection_.session_decides_what_to_write()) { return; } SetQuicReloadableFlag(quic_enable_pto, true); @@ -9558,8 +9531,7 @@ // Regression test for b/137401387 and b/138962304. TEST_P(QuicConnectionTest, RtoPacketAsTwo) { - if (!QuicConnectionPeer::GetSentPacketManager(&connection_) - ->fix_rto_retransmission() || + if (!connection_.session_decides_what_to_write() || connection_.PtoEnabled()) { return; } @@ -9603,8 +9575,7 @@ } TEST_P(QuicConnectionTest, PtoSkipsPacketNumber) { - if (!connection_.session_decides_what_to_write() || - !GetQuicReloadableFlag(quic_fix_rto_retransmission3)) { + if (!connection_.session_decides_what_to_write()) { return; } SetQuicReloadableFlag(quic_enable_pto, true);
diff --git a/quic/core/quic_sent_packet_manager.cc b/quic/core/quic_sent_packet_manager.cc index a747f0b..d6bf700 100644 --- a/quic/core/quic_sent_packet_manager.cc +++ b/quic/core/quic_sent_packet_manager.cc
@@ -108,7 +108,6 @@ pto_enabled_(false), max_probe_packets_per_pto_(2), consecutive_pto_count_(0), - fix_rto_retransmission_(false), handshake_mode_disabled_(false), detect_spurious_losses_(GetQuicReloadableFlag(quic_detect_spurious_loss)), neuter_handshake_packets_once_( @@ -170,7 +169,8 @@ } } - if (GetQuicReloadableFlag(quic_enable_pto) && fix_rto_retransmission_) { + if (GetQuicReloadableFlag(quic_enable_pto) && + session_decides_what_to_write()) { if (config.HasClientSentConnectionOption(k2PTO, perspective)) { pto_enabled_ = true; QUIC_RELOADABLE_FLAG_COUNT_N(quic_enable_pto, 2, 4); @@ -881,7 +881,8 @@ if (session_decides_what_to_write()) { has_retransmissions = it->state != OUTSTANDING; } - if (!fix_rto_retransmission_ && it->in_flight && !has_retransmissions && + if (!session_decides_what_to_write() && it->in_flight && + !has_retransmissions && !unacked_packets_.HasRetransmittableFrames(*it)) { // Log only for non-retransmittable data. // Retransmittable data is marked as lost during loss detection, and will @@ -903,7 +904,7 @@ for (QuicPacketNumber retransmission : retransmissions) { MarkForRetransmission(retransmission, RTO_RETRANSMISSION); } - if (fix_rto_retransmission_ && retransmissions.empty()) { + if (retransmissions.empty()) { QUIC_BUG_IF(pending_timer_transmission_count_ != 0); // No packets to be RTO retransmitted, raise up a credit to allow // connection to send. @@ -950,7 +951,6 @@ void QuicSentPacketManager::EnableIetfPtoAndLossDetection() { DCHECK(session_decides_what_to_write()); - fix_rto_retransmission_ = true; pto_enabled_ = true; handshake_mode_disabled_ = true; uber_loss_algorithm_.SetLossDetectionType(kIetfLossDetection); @@ -1057,7 +1057,7 @@ // Do not set the timer if there is any credit left. return QuicTime::Zero(); } - if (!fix_rto_retransmission_ && + if (!session_decides_what_to_write() && !unacked_packets_.HasUnackedRetransmittableFrames()) { return QuicTime::Zero(); } @@ -1426,11 +1426,6 @@ void QuicSentPacketManager::SetSessionDecideWhatToWrite( bool session_decides_what_to_write) { - if (GetQuicReloadableFlag(quic_fix_rto_retransmission3) && - session_decides_what_to_write) { - fix_rto_retransmission_ = true; - QUIC_RELOADABLE_FLAG_COUNT(quic_fix_rto_retransmission3); - } unacked_packets_.SetSessionDecideWhatToWrite(session_decides_what_to_write); }
diff --git a/quic/core/quic_sent_packet_manager.h b/quic/core/quic_sent_packet_manager.h index 9e903fd..e0adb01 100644 --- a/quic/core/quic_sent_packet_manager.h +++ b/quic/core/quic_sent_packet_manager.h
@@ -409,8 +409,6 @@ return unacked_packets_.supports_multiple_packet_number_spaces(); } - bool fix_rto_retransmission() const { return fix_rto_retransmission_; } - bool pto_enabled() const { return pto_enabled_; } bool handshake_mode_disabled() const { return handshake_mode_disabled_; } @@ -652,10 +650,6 @@ // Number of times the PTO timer has fired in a row without receiving an ack. size_t consecutive_pto_count_; - // Latched value of quic_fix_rto_retransmission3 and - // session_decides_what_to_write. - bool fix_rto_retransmission_; - // True if HANDSHAKE mode has been disabled. bool handshake_mode_disabled_;
diff --git a/quic/core/quic_sent_packet_manager_test.cc b/quic/core/quic_sent_packet_manager_test.cc index 4ee9fd3..389f5cb 100644 --- a/quic/core/quic_sent_packet_manager_test.cc +++ b/quic/core/quic_sent_packet_manager_test.cc
@@ -358,7 +358,6 @@ } void EnablePto(QuicTag tag) { - SetQuicReloadableFlag(quic_fix_rto_retransmission3, true); manager_.SetSessionDecideWhatToWrite(true); SetQuicReloadableFlag(quic_enable_pto, true); QuicConfig config; @@ -3070,12 +3069,8 @@ EXPECT_CALL(notifier_, RetransmitFrames(_, _)).Times(0); manager_.OnRetransmissionTimeout(); EXPECT_EQ(2u, stats_.rto_count); - if (GetQuicReloadableFlag(quic_fix_rto_retransmission3)) { - // Verify a credit is raised up. - EXPECT_EQ(1u, manager_.pending_timer_transmission_count()); - } else { - EXPECT_EQ(0u, manager_.pending_timer_transmission_count()); - } + // Verify a credit is raised up. + EXPECT_EQ(1u, manager_.pending_timer_transmission_count()); } TEST_P(QuicSentPacketManagerTest, ComputingProbeTimeout) {