Automated g4 rollback of changelist 269791493. *** Reason for rollback *** Causes b/142408319 when combine with gfe2_reloadable_flag_quic_treat_queued_packets_as_sent *** Original change description *** gfe-relnote: In QUIC, making sure there are always enough credits for non-loss retransmission. Protected by gfe2_reloadable_flag_quic_grant_enough_credits. *** PiperOrigin-RevId: 274006897 Change-Id: I2616771107eb81111a0b5afa0c5af465cf984ffa
diff --git a/quic/core/quic_connection_test.cc b/quic/core/quic_connection_test.cc index bbcbf95..68ba446 100644 --- a/quic/core/quic_connection_test.cc +++ b/quic/core/quic_connection_test.cc
@@ -3668,7 +3668,8 @@ } TEST_P(QuicConnectionTest, QueueAfterTwoRTOs) { - if (connection_.PtoEnabled()) { + if (connection_.PtoEnabled() || + !connection_.session_decides_what_to_write()) { return; } connection_.SetMaxTailLossProbes(0); @@ -3681,19 +3682,10 @@ // Block the writer and ensure they're queued. BlockOnNextWrite(); clock_.AdvanceTime(DefaultRetransmissionTime()); - // Only one packet should be retransmitted. - if (connection_.session_decides_what_to_write()) { - if (GetQuicReloadableFlag(quic_treat_queued_packets_as_sent)) { - EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, _, _, _)).Times(2); - } else { - EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, _, _, _)).Times(0); - } + if (GetQuicReloadableFlag(quic_treat_queued_packets_as_sent)) { + EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, _, _, _)).Times(2); } else { - if (GetQuicReloadableFlag(quic_treat_queued_packets_as_sent)) { - EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, _, _, _)).Times(1); - } else { - EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, _, _, _)).Times(0); - } + EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, _, _, _)).Times(0); } connection_.GetRetransmissionAlarm()->Fire(); EXPECT_TRUE(connection_.HasQueuedData()); @@ -3702,21 +3694,11 @@ writer_->SetWritable(); clock_.AdvanceTime(QuicTime::Delta::FromMicroseconds( 2 * DefaultRetransmissionTime().ToMicroseconds())); - // Retransmit already retransmitted packets event though the packet number - // greater than the largest observed. - if (connection_.session_decides_what_to_write()) { - if (GetQuicReloadableFlag(quic_treat_queued_packets_as_sent)) { - EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, _, _, _)).Times(0); - } else { - // 2 RTOs + 1 TLP. - EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, _, _, _)).Times(3); - } + if (GetQuicReloadableFlag(quic_treat_queued_packets_as_sent)) { + EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, _, _, _)).Times(2); } else { - if (GetQuicReloadableFlag(quic_treat_queued_packets_as_sent)) { - EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, _, _, _)).Times(1); - } else { - EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, _, _, _)).Times(2); - } + // 2 RTOs + 1 TLP, which is buggy. + EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, _, _, _)).Times(3); } connection_.GetRetransmissionAlarm()->Fire(); connection_.OnCanWrite(); @@ -9338,8 +9320,7 @@ TEST_P(QuicConnectionTest, RtoPacketAsTwo) { if (!QuicConnectionPeer::GetSentPacketManager(&connection_) ->fix_rto_retransmission() || - connection_.PtoEnabled() || - GetQuicReloadableFlag(quic_grant_enough_credits)) { + connection_.PtoEnabled()) { return; } connection_.SetMaxTailLossProbes(1); @@ -9412,62 +9393,6 @@ EXPECT_TRUE(connection_.GetRetransmissionAlarm()->IsSet()); } -TEST_P(QuicConnectionTest, RetransmitPacketAsTwo) { - if (!connection_.session_decides_what_to_write() || - !GetQuicReloadableFlag(quic_grant_enough_credits)) { - return; - } - connection_.SetMaxTailLossProbes(1); - connection_.SetDefaultEncryptionLevel(ENCRYPTION_FORWARD_SECURE); - std::string stream_data(3000, 's'); - // Send packets 1 - 66 and exhaust cwnd. - for (size_t i = 0; i < 22; ++i) { - // 3 packets for each stream, the first 2 are guaranteed to be full packets. - SendStreamDataToPeer(i + 2, stream_data, 0, FIN, nullptr); - } - CongestionBlockWrites(); - - if (connection_.PtoEnabled()) { - // Fires PTO, packets 1 and 2 are retransmitted as 4 packets. - EXPECT_CALL(*send_algorithm_, - OnPacketSent(_, _, QuicPacketNumber(67), _, _)); - EXPECT_CALL(*send_algorithm_, - OnPacketSent(_, _, QuicPacketNumber(68), _, _)); - EXPECT_CALL(*send_algorithm_, - OnPacketSent(_, _, QuicPacketNumber(69), _, _)); - EXPECT_CALL(*send_algorithm_, - OnPacketSent(_, _, QuicPacketNumber(70), _, _)); - } else { - // Fires TLP. Verify 2 packets gets sent. - EXPECT_CALL(*send_algorithm_, - OnPacketSent(_, _, QuicPacketNumber(67), _, _)); - EXPECT_CALL(*send_algorithm_, - OnPacketSent(_, _, QuicPacketNumber(68), _, _)); - } - connection_.GetRetransmissionAlarm()->Fire(); - - if (connection_.PtoEnabled()) { - // Fires PTO, packets 3 and 4 are retransmitted as 3 packets. - EXPECT_CALL(*send_algorithm_, - OnPacketSent(_, _, QuicPacketNumber(71), _, _)); - EXPECT_CALL(*send_algorithm_, - OnPacketSent(_, _, QuicPacketNumber(72), _, _)); - EXPECT_CALL(*send_algorithm_, - OnPacketSent(_, _, QuicPacketNumber(73), _, _)); - } else { - // Fires RTO. Verify packets 2 and 3 are retransmitted as 3 packets. - EXPECT_CALL(*send_algorithm_, - OnPacketSent(_, _, QuicPacketNumber(69), _, _)); - EXPECT_CALL(*send_algorithm_, - OnPacketSent(_, _, QuicPacketNumber(70), _, _)); - EXPECT_CALL(*send_algorithm_, - OnPacketSent(_, _, QuicPacketNumber(71), _, _)); - } - connection_.GetRetransmissionAlarm()->Fire(); - EXPECT_EQ( - 0u, connection_.sent_packet_manager().pending_timer_transmission_count()); -} - } // namespace } // namespace test } // namespace quic
diff --git a/quic/core/quic_sent_packet_manager.cc b/quic/core/quic_sent_packet_manager.cc index a32a253..a747f0b 100644 --- a/quic/core/quic_sent_packet_manager.cc +++ b/quic/core/quic_sent_packet_manager.cc
@@ -67,31 +67,6 @@ (unacked_packets_.perspective() == Perspective::IS_SERVER ? "Server: " \ : "Client: ") -QuicSentPacketManager::ScopedCreditGrantor::ScopedCreditGrantor( - QuicSentPacketManager* manager) - : manager_(manager), credits_granted_(false) { - if (!GetQuicReloadableFlag(quic_grant_enough_credits)) { - return; - } - QUIC_RELOADABLE_FLAG_COUNT(quic_grant_enough_credits); - if (manager_->pending_timer_transmission_count() > 1) { - // There are enough credits to retransmit one packet. - return; - } - // Grant 2 credits because a single packet can be transmitted as 2 (if packet - // number length changes). - manager_->set_pending_timer_transmission_count(2); - credits_granted_ = true; -} - -QuicSentPacketManager::ScopedCreditGrantor::~ScopedCreditGrantor() { - if (!credits_granted_) { - // Do not clear credits if there is no credit granted. - return; - } - manager_->set_pending_timer_transmission_count(0); -} - QuicSentPacketManager::QuicSentPacketManager( Perspective perspective, const QuicClock* clock, @@ -555,7 +530,6 @@ // applications may want to use higher priority stream data for bandwidth // probing, and some applications want to consider RTO is an indication of // loss, etc. - ScopedCreditGrantor grantor(this); unacked_packets_.RetransmitFrames(*transmission_info, transmission_type); return; }
diff --git a/quic/core/quic_sent_packet_manager.h b/quic/core/quic_sent_packet_manager.h index cc6f4b9..9e903fd 100644 --- a/quic/core/quic_sent_packet_manager.h +++ b/quic/core/quic_sent_packet_manager.h
@@ -108,19 +108,6 @@ PTO_MODE, }; - // Grantor makes sure there are enough credits to retransmit a packet. When - // grantor is out of scope, remaining credits will be cleared. - class QUIC_EXPORT_PRIVATE ScopedCreditGrantor { - public: - explicit ScopedCreditGrantor(QuicSentPacketManager* manager); - ~ScopedCreditGrantor(); - - private: - QuicSentPacketManager* manager_; - // Indicates whether any credit has been granted. - bool credits_granted_; - }; - QuicSentPacketManager(Perspective perspective, const QuicClock* clock, QuicRandom* random, @@ -386,10 +373,6 @@ return pending_timer_transmission_count_; } - void set_pending_timer_transmission_count(size_t count) { - pending_timer_transmission_count_ = count; - } - QuicTime::Delta peer_max_ack_delay() const { return peer_max_ack_delay_; } void set_peer_max_ack_delay(QuicTime::Delta peer_max_ack_delay) {