gfe-relnote: In QUIC, when arming PTO timer, ignore application data until handshake completes rather than handshake confirmed. Not affecting prod, not protected. Also refactor GetEarliestPacketSentTimeForPto to make the code cleaner. PiperOrigin-RevId: 296039749 Change-Id: I6ed4507a5126cc641dc09259830f2b0ce2f1012f
diff --git a/quic/core/quic_sent_packet_manager.cc b/quic/core/quic_sent_packet_manager.cc index 92e6e84..e4f46d8 100644 --- a/quic/core/quic_sent_packet_manager.cc +++ b/quic/core/quic_sent_packet_manager.cc
@@ -104,7 +104,8 @@ pto_exponential_backoff_start_point_(0), pto_rttvar_multiplier_(4), num_tlp_timeout_ptos_(0), - one_rtt_packet_acked_(false) { + one_rtt_packet_acked_(false), + one_rtt_packet_sent_(false) { SetSendAlgorithm(congestion_control_type); } @@ -475,24 +476,33 @@ for (int8_t i = 0; i < NUM_PACKET_NUMBER_SPACES; ++i) { const QuicTime sent_time = unacked_packets_.GetLastInFlightPacketSentTime( static_cast<PacketNumberSpace>(i)); - if (!earliest_sent_time.IsInitialized()) { - earliest_sent_time = sent_time; - *packet_number_space = static_cast<PacketNumberSpace>(i); + if (!ShouldArmPtoForApplicationData() && i == APPLICATION_DATA) { continue; } - if (i == APPLICATION_DATA) { - // Not arming PTO for application data to prioritize the completion of - // handshake. - break; + if (!sent_time.IsInitialized() || (earliest_sent_time.IsInitialized() && + earliest_sent_time <= sent_time)) { + continue; } - if (sent_time.IsInitialized()) { - earliest_sent_time = std::min(earliest_sent_time, sent_time); - *packet_number_space = static_cast<PacketNumberSpace>(i); - } + earliest_sent_time = sent_time; + *packet_number_space = static_cast<PacketNumberSpace>(i); } + return earliest_sent_time; } +bool QuicSentPacketManager::ShouldArmPtoForApplicationData() const { + DCHECK(supports_multiple_packet_number_spaces()); + // Application data must be ignored before handshake completes (1-RTT key + // is available). Not arming PTO for application data to prioritize the + // completion of handshake. On the server side, handshake_finished_ + // indicates handshake complete (and confirmed). On the client side, + // one_rtt_packet_sent_ indicates handshake complete (while handshake + // confirmation will happen later). + return handshake_finished_ || + (unacked_packets_.perspective() == Perspective::IS_CLIENT && + one_rtt_packet_sent_); +} + void QuicSentPacketManager::MarkForRetransmission( QuicPacketNumber packet_number, TransmissionType transmission_type) { @@ -635,6 +645,10 @@ serialized_packet->encrypted_length, has_retransmittable_data); } + if (serialized_packet->encryption_level == ENCRYPTION_FORWARD_SECURE) { + one_rtt_packet_sent_ = true; + } + unacked_packets_.AddSentPacket(serialized_packet, transmission_type, sent_time, in_flight); // Reset the retransmission timer anytime a pending packet is sent.
diff --git a/quic/core/quic_sent_packet_manager.h b/quic/core/quic_sent_packet_manager.h index 61631c2..d8fc069 100644 --- a/quic/core/quic_sent_packet_manager.h +++ b/quic/core/quic_sent_packet_manager.h
@@ -515,6 +515,10 @@ QuicTime GetEarliestPacketSentTimeForPto( PacketNumberSpace* packet_number_space) const; + // Returns true if application data should be used to arm PTO. Only used when + // multiple packet number space is enabled. + bool ShouldArmPtoForApplicationData() const; + // Newly serialized retransmittable packets are added to this map, which // contains owning pointers to any contained frames. If a packet is // retransmitted, this map will contain entries for both the old and the new @@ -641,6 +645,9 @@ // True if any 1-RTT packet gets acknowledged. bool one_rtt_packet_acked_; + + // True if any 1-RTT packet gets sent. + bool one_rtt_packet_sent_; }; } // namespace quic
diff --git a/quic/core/quic_sent_packet_manager_test.cc b/quic/core/quic_sent_packet_manager_test.cc index 2449cd6..08e918e 100644 --- a/quic/core/quic_sent_packet_manager_test.cc +++ b/quic/core/quic_sent_packet_manager_test.cc
@@ -3210,6 +3210,7 @@ rtt_stats->UpdateRtt(QuicTime::Delta::FromMilliseconds(100), QuicTime::Delta::Zero(), QuicTime::Zero()); QuicTime::Delta srtt = rtt_stats->smoothed_rtt(); + QuicSentPacketManagerPeer::SetPerspective(&manager_, Perspective::IS_CLIENT); // Send packet 1. SendDataPacket(1, ENCRYPTION_INITIAL); @@ -3247,10 +3248,9 @@ EXPECT_EQ(packet3_sent_time + expected_pto_delay * 2, manager_.GetRetransmissionTime()); - // Send packet 4 in application data. + // Send packet 4 in application data with 0-RTT. clock_.AdvanceTime(QuicTime::Delta::FromMilliseconds(10)); - SendDataPacket(4, ENCRYPTION_FORWARD_SECURE); - const QuicTime packet4_sent_time = clock_.Now(); + SendDataPacket(4, ENCRYPTION_ZERO_RTT); // Verify PTO timeout is still based on packet 3. EXPECT_EQ(packet3_sent_time + expected_pto_delay * 2, manager_.GetRetransmissionTime()); @@ -3258,14 +3258,31 @@ // Send packet 5 in handshake. clock_.AdvanceTime(QuicTime::Delta::FromMilliseconds(10)); SendDataPacket(5, ENCRYPTION_HANDSHAKE); - // Verify PTO timeout is now based on packet 5. + const QuicTime packet5_sent_time = clock_.Now(); + // Verify PTO timeout is now based on packet 5 because packet 4 should be + // ignored. EXPECT_EQ(clock_.Now() + expected_pto_delay * 2, manager_.GetRetransmissionTime()); + // Send packet 6 in 1-RTT. + clock_.AdvanceTime(QuicTime::Delta::FromMilliseconds(10)); + SendDataPacket(6, ENCRYPTION_FORWARD_SECURE); + const QuicTime packet6_sent_time = clock_.Now(); + // Verify PTO timeout is now based on packet 5. + EXPECT_EQ(packet5_sent_time + expected_pto_delay * 2, + manager_.GetRetransmissionTime()); + + // Send packet 7 in handshake. + clock_.AdvanceTime(QuicTime::Delta::FromMilliseconds(10)); + SendDataPacket(7, ENCRYPTION_HANDSHAKE); + // Verify PTO timeout is now based on packet 6. + EXPECT_EQ(packet6_sent_time + expected_pto_delay * 2, + manager_.GetRetransmissionTime()); + // Neuter handshake key. manager_.SetHandshakeConfirmed(); - // Verify PTO timeout is now based on packet 4. - EXPECT_EQ(packet4_sent_time + expected_pto_delay * 2, + // Verify PTO timeout remains unchanged. + EXPECT_EQ(packet6_sent_time + expected_pto_delay * 2, manager_.GetRetransmissionTime()); } @@ -3294,22 +3311,36 @@ // Send packet 2 in handshake. clock_.AdvanceTime(QuicTime::Delta::FromMilliseconds(10)); SendDataPacket(2, ENCRYPTION_HANDSHAKE); + const QuicTime packet2_sent_time = clock_.Now(); // Verify PTO timeout is still based on packet 1. EXPECT_EQ(packet1_sent_time + expected_pto_delay, manager_.GetRetransmissionTime()); - // Discard both initial and handshake keys. + // Discard initial keys. EXPECT_CALL(notifier_, IsFrameOutstanding(_)).WillRepeatedly(Return(false)); manager_.NeuterUnencryptedPackets(); - manager_.SetHandshakeConfirmed(); - EXPECT_EQ(QuicTime::Zero(), manager_.GetRetransmissionTime()); - // Send packet 3 in application data. + // Send packet 3 in 1-RTT. clock_.AdvanceTime(QuicTime::Delta::FromMilliseconds(10)); SendDataPacket(3, ENCRYPTION_FORWARD_SECURE); - // Verify PTO timeout is correctly set. + // Verify PTO timeout is based on packet 2. + const QuicTime packet3_sent_time = clock_.Now(); + EXPECT_EQ(packet2_sent_time + expected_pto_delay, + manager_.GetRetransmissionTime()); + + // Send packet 4 in handshake. + clock_.AdvanceTime(QuicTime::Delta::FromMilliseconds(10)); + SendDataPacket(4, ENCRYPTION_HANDSHAKE); + // Verify PTO timeout is based on packet 4 as application data is ignored. EXPECT_EQ(clock_.Now() + expected_pto_delay, manager_.GetRetransmissionTime()); + + // Discard handshake keys. + manager_.SetHandshakeConfirmed(); + // Verify PTO timeout is now based on packet 3 as handshake is + // complete/confirmed. + EXPECT_EQ(packet3_sent_time + expected_pto_delay, + manager_.GetRetransmissionTime()); } // Regresstion test for b/148841700.