Reporting ACK delay based on packet receipt time (rather than packet process time).
Protected by FLAGS_quic_reloadable_flag_quic_update_ack_timeout_on_receipt_time.
PiperOrigin-RevId: 436708428
diff --git a/quic/core/quic_connection.cc b/quic/core/quic_connection.cc
index f59e250..99441b6 100644
--- a/quic/core/quic_connection.cc
+++ b/quic/core/quic_connection.cc
@@ -2199,8 +2199,8 @@
if (!should_last_packet_instigate_acks_) {
uber_received_packet_manager_.MaybeUpdateAckTimeout(
should_last_packet_instigate_acks_, last_decrypted_packet_level_,
- last_header_.packet_number, clock_->ApproximateNow(),
- sent_packet_manager_.GetRttStats());
+ last_header_.packet_number, last_received_packet_info_.receipt_time,
+ clock_->ApproximateNow(), sent_packet_manager_.GetRttStats());
}
ClearLastFrames();
@@ -6330,8 +6330,8 @@
should_last_packet_instigate_acks_ = true;
uber_received_packet_manager_.MaybeUpdateAckTimeout(
/*should_last_packet_instigate_acks=*/true, last_decrypted_packet_level_,
- last_header_.packet_number, clock_->ApproximateNow(),
- sent_packet_manager_.GetRttStats());
+ last_header_.packet_number, last_received_packet_info_.receipt_time,
+ clock_->ApproximateNow(), sent_packet_manager_.GetRttStats());
}
QuicTime QuicConnection::GetPathDegradingDeadline() const {
diff --git a/quic/core/quic_connection_test.cc b/quic/core/quic_connection_test.cc
index aa13e51..86da486 100644
--- a/quic/core/quic_connection_test.cc
+++ b/quic/core/quic_connection_test.cc
@@ -2925,7 +2925,8 @@
}
connection_.ProcessUdpPacket(
kSelfAddress, kPeerAddress,
- QuicReceivedPacket(buffer, encrypted_length, QuicTime::Zero(), false));
+ QuicReceivedPacket(buffer, encrypted_length, clock_.ApproximateNow(),
+ false));
EXPECT_EQ(kMaxOutgoingPacketSize, connection_.max_packet_length());
}
@@ -2972,7 +2973,8 @@
}
connection_.ProcessUdpPacket(
kSelfAddress, kPeerAddress,
- QuicReceivedPacket(buffer, encrypted_length, QuicTime::Zero(), false));
+ QuicReceivedPacket(buffer, encrypted_length, clock_.ApproximateNow(),
+ false));
// Here, the limit imposed by the writer is lower than the size of the packet
// received, so the writer max packet size is used.
@@ -11551,7 +11553,10 @@
// SetFromConfig is always called after construction from InitializeSession.
EXPECT_CALL(visitor_, OnSuccessfulVersionNegotiation(_));
EXPECT_CALL(*send_algorithm_, SetFromConfig(_, _));
- EXPECT_CALL(visitor_, OnHandshakePacketSent()).Times(AnyNumber());
+ EXPECT_CALL(visitor_, OnHandshakePacketSent()).WillOnce(Invoke([this]() {
+ connection_.RemoveEncrypter(ENCRYPTION_INITIAL);
+ connection_.NeuterUnencryptedPackets();
+ }));
QuicConfig config;
connection_.SetFromConfig(config);
connection_.SetDefaultEncryptionLevel(ENCRYPTION_INITIAL);
@@ -11576,13 +11581,29 @@
std::make_unique<TaggingEncrypter>(0x01));
connection_.SetDefaultEncryptionLevel(ENCRYPTION_HANDSHAKE);
// Verify HANDSHAKE packet gets processed.
+ if (GetQuicReloadableFlag(quic_update_ack_timeout_on_receipt_time)) {
+ EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, _, _, _)).Times(1);
+ } else {
+ EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, _, _, _)).Times(2);
+ }
connection_.GetProcessUndecryptablePacketsAlarm()->Fire();
- ASSERT_TRUE(connection_.HasPendingAcks());
- // Send ACKs.
- clock_.AdvanceTime(connection_.GetAckAlarm()->deadline() - clock_.Now());
- EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, _, _, _)).Times(2);
- connection_.GetAckAlarm()->Fire();
+ if (GetQuicReloadableFlag(quic_update_ack_timeout_on_receipt_time)) {
+ // Verify immediate ACK has been sent out when flush went out of scope.
+ ASSERT_FALSE(connection_.HasPendingAcks());
+ } else {
+ ASSERT_TRUE(connection_.HasPendingAcks());
+ // Send ACKs.
+ clock_.AdvanceTime(connection_.GetAckAlarm()->deadline() - clock_.Now());
+ connection_.GetAckAlarm()->Fire();
+ }
ASSERT_FALSE(writer_->ack_frames().empty());
+ if (GetQuicReloadableFlag(quic_update_ack_timeout_on_receipt_time)) {
+ // Verify the ack_delay_time in the sent HANDSHAKE ACK frame is 100ms.
+ EXPECT_EQ(QuicTime::Delta::FromMilliseconds(100),
+ writer_->ack_frames()[0].ack_delay_time);
+ ASSERT_TRUE(writer_->coalesced_packet() == nullptr);
+ return;
+ }
// Verify the ack_delay_time in the INITIAL ACK frame is 1ms.
EXPECT_EQ(QuicTime::Delta::FromMilliseconds(1),
writer_->ack_frames()[0].ack_delay_time);
@@ -15535,6 +15556,73 @@
EXPECT_TRUE(connection_.connected());
}
+TEST_P(QuicConnectionTest, ReportedAckDelayIncludesQueuingDelay) {
+ if (!version().HasIetfQuicFrames()) {
+ return;
+ }
+ EXPECT_CALL(*send_algorithm_, SetFromConfig(_, _));
+ QuicConfig config;
+ config.set_max_undecryptable_packets(3);
+ connection_.SetFromConfig(config);
+
+ // Receive 1-RTT ack-eliciting packet while keys are not available.
+ connection_.RemoveDecrypter(ENCRYPTION_FORWARD_SECURE);
+ peer_framer_.SetEncrypter(ENCRYPTION_FORWARD_SECURE,
+ std::make_unique<TaggingEncrypter>(0x01));
+ QuicFrames frames;
+ frames.push_back(QuicFrame(QuicPingFrame()));
+ frames.push_back(QuicFrame(QuicPaddingFrame(100)));
+ QuicPacketHeader header =
+ ConstructPacketHeader(30, ENCRYPTION_FORWARD_SECURE);
+ std::unique_ptr<QuicPacket> packet(ConstructPacket(header, frames));
+
+ char buffer[kMaxOutgoingPacketSize];
+ size_t encrypted_length = peer_framer_.EncryptPayload(
+ ENCRYPTION_FORWARD_SECURE, QuicPacketNumber(30), *packet, buffer,
+ kMaxOutgoingPacketSize);
+ EXPECT_EQ(0u, QuicConnectionPeer::NumUndecryptablePackets(&connection_));
+ const QuicTime packet_receipt_time = clock_.Now();
+ connection_.ProcessUdpPacket(
+ kSelfAddress, kPeerAddress,
+ QuicReceivedPacket(buffer, encrypted_length, clock_.Now(), false));
+ if (connection_.GetSendAlarm()->IsSet()) {
+ connection_.GetSendAlarm()->Fire();
+ }
+ ASSERT_EQ(1u, QuicConnectionPeer::NumUndecryptablePackets(&connection_));
+ // 1-RTT keys become available after 10ms.
+ const QuicTime::Delta kQueuingDelay = QuicTime::Delta::FromMilliseconds(10);
+ clock_.AdvanceTime(kQueuingDelay);
+ EXPECT_FALSE(connection_.GetProcessUndecryptablePacketsAlarm()->IsSet());
+ connection_.SetDefaultEncryptionLevel(ENCRYPTION_FORWARD_SECURE);
+ SetDecrypter(ENCRYPTION_FORWARD_SECURE,
+ std::make_unique<StrictTaggingDecrypter>(0x01));
+ ASSERT_TRUE(connection_.GetProcessUndecryptablePacketsAlarm()->IsSet());
+
+ connection_.GetProcessUndecryptablePacketsAlarm()->Fire();
+ ASSERT_TRUE(connection_.HasPendingAcks());
+ if (GetQuicReloadableFlag(quic_update_ack_timeout_on_receipt_time)) {
+ EXPECT_EQ(packet_receipt_time + DefaultDelayedAckTime(),
+ connection_.GetAckAlarm()->deadline());
+ clock_.AdvanceTime(packet_receipt_time + DefaultDelayedAckTime() -
+ clock_.Now());
+ } else {
+ EXPECT_EQ(clock_.Now() + DefaultDelayedAckTime(),
+ connection_.GetAckAlarm()->deadline());
+ clock_.AdvanceTime(DefaultDelayedAckTime());
+ }
+ // Fire ACK alarm.
+ connection_.GetAckAlarm()->Fire();
+ ASSERT_EQ(1u, writer_->ack_frames().size());
+ if (GetQuicReloadableFlag(quic_update_ack_timeout_on_receipt_time)) {
+ // Verify ACK delay time does not include queuing delay.
+ EXPECT_EQ(DefaultDelayedAckTime(), writer_->ack_frames()[0].ack_delay_time);
+ } else {
+ // Verify ACK delay time = queuing delay + ack delay
+ EXPECT_EQ(DefaultDelayedAckTime() + kQueuingDelay,
+ writer_->ack_frames()[0].ack_delay_time);
+ }
+}
+
} // namespace
} // namespace test
} // namespace quic
diff --git a/quic/core/quic_flags_list.h b/quic/core/quic_flags_list.h
index 106288e..90def64 100644
--- a/quic/core/quic_flags_list.h
+++ b/quic/core/quic_flags_list.h
@@ -91,6 +91,8 @@
QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_conservative_bursts, false)
// If true, stop resetting ideal_next_packet_send_time_ in pacing sender.
QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_donot_reset_ideal_next_packet_send_time, false)
+// If true, update ACK timeout based on packet receipt time rather than now.
+QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_update_ack_timeout_on_receipt_time, true)
// If true, use BBRv2 as the default congestion controller. Takes precedence over --quic_default_to_bbr.
QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_default_to_bbr_v2, false)
// If true, use new connection ID in connection migration.
diff --git a/quic/core/quic_received_packet_manager.cc b/quic/core/quic_received_packet_manager.cc
index d0f1578..c4e7333 100644
--- a/quic/core/quic_received_packet_manager.cc
+++ b/quic/core/quic_received_packet_manager.cc
@@ -234,7 +234,7 @@
void QuicReceivedPacketManager::MaybeUpdateAckTimeout(
bool should_last_packet_instigate_acks,
QuicPacketNumber last_received_packet_number,
- QuicTime now,
+ QuicTime last_packet_receipt_time, QuicTime now,
const RttStats* rtt_stats) {
if (!ack_frame_updated_) {
// ACK frame has not been updated, nothing to do.
@@ -268,8 +268,24 @@
return;
}
+ QuicTime ack_timeout_base = now;
+ const bool quic_update_ack_timeout_on_receipt_time =
+ GetQuicReloadableFlag(quic_update_ack_timeout_on_receipt_time);
+ if (quic_update_ack_timeout_on_receipt_time) {
+ if (last_packet_receipt_time <= now) {
+ QUIC_CODE_COUNT(quic_update_ack_timeout_on_receipt_time);
+ ack_timeout_base = last_packet_receipt_time;
+ } else {
+ QUIC_CODE_COUNT(quic_update_ack_timeout_on_now);
+ ack_timeout_base = now;
+ }
+ }
QuicTime updated_ack_time =
- now + GetMaxAckDelay(last_received_packet_number, *rtt_stats);
+ ack_timeout_base +
+ GetMaxAckDelay(last_received_packet_number, *rtt_stats);
+ if (quic_update_ack_timeout_on_receipt_time) {
+ updated_ack_time = std::max(now, updated_ack_time);
+ }
if (!ack_timeout_.IsInitialized() || ack_timeout_ > updated_ack_time) {
ack_timeout_ = updated_ack_time;
}
diff --git a/quic/core/quic_received_packet_manager.h b/quic/core/quic_received_packet_manager.h
index 21f7f04..7d8edf8 100644
--- a/quic/core/quic_received_packet_manager.h
+++ b/quic/core/quic_received_packet_manager.h
@@ -64,7 +64,7 @@
// Otherwise, ACK needs to be sent by the specified time.
void MaybeUpdateAckTimeout(bool should_last_packet_instigate_acks,
QuicPacketNumber last_received_packet_number,
- QuicTime now,
+ QuicTime last_packet_receipt_time, QuicTime now,
const RttStats* rtt_stats);
// Resets ACK related states, called after an ACK is successfully sent.
diff --git a/quic/core/quic_received_packet_manager_test.cc b/quic/core/quic_received_packet_manager_test.cc
index cc04381..0bb6160 100644
--- a/quic/core/quic_received_packet_manager_test.cc
+++ b/quic/core/quic_received_packet_manager_test.cc
@@ -67,8 +67,9 @@
uint64_t last_received_packet_number) {
received_manager_.MaybeUpdateAckTimeout(
should_last_packet_instigate_acks,
- QuicPacketNumber(last_received_packet_number), clock_.ApproximateNow(),
- &rtt_stats_);
+ QuicPacketNumber(last_received_packet_number),
+ /*last_packet_receipt_time=*/clock_.ApproximateNow(),
+ /*now=*/clock_.ApproximateNow(), &rtt_stats_);
}
void CheckAckTimeout(QuicTime time) {
@@ -637,6 +638,54 @@
}
}
+TEST_F(QuicReceivedPacketManagerTest, UpdateAckTimeoutOnPacketReceiptTime) {
+ EXPECT_FALSE(HasPendingAck());
+
+ // Received packets 3 and 4.
+ QuicTime packet_receipt_time3 = clock_.ApproximateNow();
+ // Packet 3 gets processed after 10ms.
+ clock_.AdvanceTime(QuicTime::Delta::FromMilliseconds(10));
+ RecordPacketReceipt(3, packet_receipt_time3);
+ received_manager_.MaybeUpdateAckTimeout(
+ kInstigateAck, QuicPacketNumber(3),
+ /*last_packet_receipt_time=*/packet_receipt_time3,
+ clock_.ApproximateNow(), &rtt_stats_);
+ if (GetQuicReloadableFlag(quic_update_ack_timeout_on_receipt_time)) {
+ // Make sure ACK timeout is based on receipt time.
+ CheckAckTimeout(packet_receipt_time3 + kDelayedAckTime);
+ } else {
+ // Make sure ACK timeout is based on now.
+ CheckAckTimeout(clock_.ApproximateNow() + kDelayedAckTime);
+ }
+
+ RecordPacketReceipt(4, clock_.ApproximateNow());
+ MaybeUpdateAckTimeout(kInstigateAck, 4);
+ // Immediate ack is sent.
+ CheckAckTimeout(clock_.ApproximateNow());
+}
+
+TEST_F(QuicReceivedPacketManagerTest,
+ UpdateAckTimeoutOnPacketReceiptTimeLongerQueuingTime) {
+ EXPECT_FALSE(HasPendingAck());
+
+ // Received packets 3 and 4.
+ QuicTime packet_receipt_time3 = clock_.ApproximateNow();
+ // Packet 3 gets processed after 100ms.
+ clock_.AdvanceTime(QuicTime::Delta::FromMilliseconds(100));
+ RecordPacketReceipt(3, packet_receipt_time3);
+ received_manager_.MaybeUpdateAckTimeout(
+ kInstigateAck, QuicPacketNumber(3),
+ /*last_packet_receipt_time=*/packet_receipt_time3,
+ clock_.ApproximateNow(), &rtt_stats_);
+ if (GetQuicReloadableFlag(quic_update_ack_timeout_on_receipt_time)) {
+ // Given 100ms > ack delay, verify immediate ACK.
+ CheckAckTimeout(clock_.ApproximateNow());
+ } else {
+ // Make sure ACK timeout is based on now.
+ CheckAckTimeout(clock_.ApproximateNow() + kDelayedAckTime);
+ }
+}
+
} // namespace
} // namespace test
} // namespace quic
diff --git a/quic/core/uber_received_packet_manager.cc b/quic/core/uber_received_packet_manager.cc
index c25329c..62411d5 100644
--- a/quic/core/uber_received_packet_manager.cc
+++ b/quic/core/uber_received_packet_manager.cc
@@ -76,18 +76,19 @@
bool should_last_packet_instigate_acks,
EncryptionLevel decrypted_packet_level,
QuicPacketNumber last_received_packet_number,
- QuicTime now,
+ QuicTime last_packet_receipt_time, QuicTime now,
const RttStats* rtt_stats) {
if (!supports_multiple_packet_number_spaces_) {
received_packet_managers_[0].MaybeUpdateAckTimeout(
- should_last_packet_instigate_acks, last_received_packet_number, now,
- rtt_stats);
+ should_last_packet_instigate_acks, last_received_packet_number,
+ last_packet_receipt_time, now, rtt_stats);
return;
}
received_packet_managers_[QuicUtils::GetPacketNumberSpace(
decrypted_packet_level)]
.MaybeUpdateAckTimeout(should_last_packet_instigate_acks,
- last_received_packet_number, now, rtt_stats);
+ last_received_packet_number,
+ last_packet_receipt_time, now, rtt_stats);
}
void UberReceivedPacketManager::ResetAckStates(
diff --git a/quic/core/uber_received_packet_manager.h b/quic/core/uber_received_packet_manager.h
index 3dddbda..c0c3e69 100644
--- a/quic/core/uber_received_packet_manager.h
+++ b/quic/core/uber_received_packet_manager.h
@@ -47,7 +47,7 @@
void MaybeUpdateAckTimeout(bool should_last_packet_instigate_acks,
EncryptionLevel decrypted_packet_level,
QuicPacketNumber last_received_packet_number,
- QuicTime now,
+ QuicTime last_packet_receipt_time, QuicTime now,
const RttStats* rtt_stats);
// Resets ACK related states, called after an ACK is successfully sent.
diff --git a/quic/core/uber_received_packet_manager_test.cc b/quic/core/uber_received_packet_manager_test.cc
index 77731fb..8ca3ae9 100644
--- a/quic/core/uber_received_packet_manager_test.cc
+++ b/quic/core/uber_received_packet_manager_test.cc
@@ -85,7 +85,7 @@
manager_->MaybeUpdateAckTimeout(
should_last_packet_instigate_acks, decrypted_packet_level,
QuicPacketNumber(last_received_packet_number), clock_.ApproximateNow(),
- &rtt_stats_);
+ clock_.ApproximateNow(), &rtt_stats_);
}
void CheckAckTimeout(QuicTime time) {
@@ -522,6 +522,40 @@
EXPECT_FALSE(HasPendingAck());
}
+TEST_F(UberReceivedPacketManagerTest,
+ AckTimeoutForPreviouslyUndecryptablePackets) {
+ manager_->EnableMultiplePacketNumberSpacesSupport(Perspective::IS_SERVER);
+ EXPECT_FALSE(HasPendingAck());
+ EXPECT_FALSE(manager_->IsAckFrameUpdated());
+
+ // Received undecryptable 1-RTT packet 4.
+ const QuicTime packet_receipt_time4 = clock_.ApproximateNow();
+ // 1-RTT keys become available after 10ms because HANDSHAKE 5 gets received.
+ clock_.AdvanceTime(QuicTime::Delta::FromMilliseconds(10));
+ RecordPacketReceipt(ENCRYPTION_HANDSHAKE, 5);
+ MaybeUpdateAckTimeout(kInstigateAck, ENCRYPTION_HANDSHAKE, 5);
+ EXPECT_TRUE(HasPendingAck());
+ RecordPacketReceipt(ENCRYPTION_FORWARD_SECURE, 4);
+ manager_->MaybeUpdateAckTimeout(kInstigateAck, ENCRYPTION_FORWARD_SECURE,
+ QuicPacketNumber(4), packet_receipt_time4,
+ clock_.ApproximateNow(), &rtt_stats_);
+
+ // Send delayed handshake ACK.
+ clock_.AdvanceTime(QuicTime::Delta::FromMilliseconds(1));
+ CheckAckTimeout(clock_.ApproximateNow());
+
+ EXPECT_TRUE(HasPendingAck());
+ if (GetQuicReloadableFlag(quic_update_ack_timeout_on_receipt_time)) {
+ // Verify ACK delay is based on packet receipt time.
+ CheckAckTimeout(clock_.ApproximateNow() -
+ QuicTime::Delta::FromMilliseconds(11) + kDelayedAckTime);
+ } else {
+ // Delayed ack is scheduled.
+ CheckAckTimeout(clock_.ApproximateNow() -
+ QuicTime::Delta::FromMilliseconds(1) + kDelayedAckTime);
+ }
+}
+
} // namespace
} // namespace test
} // namespace quic
