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.
