In quic, update ack timeout upon receiving a retransmittable frame, rather than waiting for the packet to be fully processed. protected by gfe2_reloadable_flag_quic_advance_ack_timeout_update.
PiperOrigin-RevId: 310554284
Change-Id: Ic4301712cfa1f935d512029921f68f04482fb8ff
diff --git a/quic/core/http/end_to_end_test.cc b/quic/core/http/end_to_end_test.cc
index 7490e28..a3089cd 100644
--- a/quic/core/http/end_to_end_test.cc
+++ b/quic/core/http/end_to_end_test.cc
@@ -3943,7 +3943,14 @@
client_->WaitForResponse();
EXPECT_EQ(kBarResponseBody, client_->response_body());
QuicConnectionStats client_stats = GetClientConnection()->GetStats();
- EXPECT_EQ(0u, client_stats.packets_lost);
+ if (GetQuicReloadableFlag(quic_advance_ack_timeout_update)) {
+ // Client sends CHLO in packet 1 and retransmitted in packet 2. Because of
+ // the delay, server processes packet 2 and later drops packet 1. ACK is
+ // bundled with SHLO, such that 1 can be detected loss by time threshold.
+ EXPECT_LE(0u, client_stats.packets_lost);
+ } else {
+ EXPECT_EQ(0u, client_stats.packets_lost);
+ }
EXPECT_TRUE(client_->client()->EarlyDataAccepted());
}
diff --git a/quic/core/quic_connection.cc b/quic/core/quic_connection.cc
index 7922fdd..32d3edd 100644
--- a/quic/core/quic_connection.cc
+++ b/quic/core/quic_connection.cc
@@ -348,7 +348,9 @@
<< "QuicConnection: attempted to use server connection ID "
<< server_connection_id << " which is invalid with version "
<< QuicVersionToString(transport_version());
-
+ if (advance_ack_timeout_update_) {
+ QUIC_RELOADABLE_FLAG_COUNT(quic_advance_ack_timeout_update);
+ }
framer_.set_visitor(this);
stats_.connection_creation_time = clock_->ApproximateNow();
// TODO(ianswett): Supply the NetworkChangeVisitor as a constructor argument
@@ -963,9 +965,14 @@
ConnectionCloseBehavior::SEND_CONNECTION_CLOSE_PACKET);
return false;
}
+ if (advance_ack_timeout_update_) {
+ MaybeUpdateAckTimeout();
+ }
visitor_->OnStreamFrame(frame);
stats_.stream_bytes_received += frame.data_length;
- should_last_packet_instigate_acks_ = true;
+ if (!advance_ack_timeout_update_) {
+ should_last_packet_instigate_acks_ = true;
+ }
consecutive_retransmittable_on_wire_ping_count_ = 0;
return connected_;
}
@@ -980,8 +987,13 @@
if (debug_visitor_ != nullptr) {
debug_visitor_->OnCryptoFrame(frame);
}
+ if (advance_ack_timeout_update_) {
+ MaybeUpdateAckTimeout();
+ }
visitor_->OnCryptoFrame(frame);
- should_last_packet_instigate_acks_ = true;
+ if (!advance_ack_timeout_update_) {
+ should_last_packet_instigate_acks_ = true;
+ }
return connected_;
}
@@ -1168,7 +1180,11 @@
if (debug_visitor_ != nullptr) {
debug_visitor_->OnPingFrame(frame);
}
- should_last_packet_instigate_acks_ = true;
+ if (advance_ack_timeout_update_) {
+ MaybeUpdateAckTimeout();
+ } else {
+ should_last_packet_instigate_acks_ = true;
+ }
return true;
}
@@ -1210,8 +1226,13 @@
<< "RST_STREAM_FRAME received for stream: " << frame.stream_id
<< " with error: "
<< QuicRstStreamErrorCodeToString(frame.error_code);
+ if (advance_ack_timeout_update_) {
+ MaybeUpdateAckTimeout();
+ }
visitor_->OnRstStream(frame);
- should_last_packet_instigate_acks_ = true;
+ if (!advance_ack_timeout_update_) {
+ should_last_packet_instigate_acks_ = true;
+ }
return connected_;
}
@@ -1242,7 +1263,11 @@
// response.
received_path_challenge_payloads_.push_back(frame.data_buffer);
- should_last_packet_instigate_acks_ = true;
+ if (advance_ack_timeout_update_) {
+ MaybeUpdateAckTimeout();
+ } else {
+ should_last_packet_instigate_acks_ = true;
+ }
return true;
}
@@ -1250,7 +1275,11 @@
if (debug_visitor_ != nullptr) {
debug_visitor_->OnPathResponseFrame(frame);
}
- should_last_packet_instigate_acks_ = true;
+ if (advance_ack_timeout_update_) {
+ MaybeUpdateAckTimeout();
+ } else {
+ should_last_packet_instigate_acks_ = true;
+ }
if (!transmitted_connectivity_probe_payload_ ||
*transmitted_connectivity_probe_payload_ != frame.data_buffer) {
// Is not for the probe we sent, ignore it.
@@ -1337,9 +1366,13 @@
<< frame.last_good_stream_id
<< " and error: " << QuicErrorCodeToString(frame.error_code)
<< " and reason: " << frame.reason_phrase;
-
+ if (advance_ack_timeout_update_) {
+ MaybeUpdateAckTimeout();
+ }
visitor_->OnGoAway(frame);
- should_last_packet_instigate_acks_ = true;
+ if (!advance_ack_timeout_update_) {
+ should_last_packet_instigate_acks_ = true;
+ }
return connected_;
}
@@ -1354,8 +1387,13 @@
debug_visitor_->OnWindowUpdateFrame(frame, GetTimeOfLastReceivedPacket());
}
QUIC_DVLOG(1) << ENDPOINT << "WINDOW_UPDATE_FRAME received " << frame;
+ if (advance_ack_timeout_update_) {
+ MaybeUpdateAckTimeout();
+ }
visitor_->OnWindowUpdateFrame(frame);
- should_last_packet_instigate_acks_ = true;
+ if (!advance_ack_timeout_update_) {
+ should_last_packet_instigate_acks_ = true;
+ }
return connected_;
}
@@ -1392,9 +1430,14 @@
if (debug_visitor_ != nullptr) {
debug_visitor_->OnMessageFrame(frame);
}
+ if (advance_ack_timeout_update_) {
+ MaybeUpdateAckTimeout();
+ }
visitor_->OnMessageReceived(
quiche::QuicheStringPiece(frame.data, frame.message_length));
- should_last_packet_instigate_acks_ = true;
+ if (!advance_ack_timeout_update_) {
+ should_last_packet_instigate_acks_ = true;
+ }
return connected_;
}
@@ -1415,8 +1458,13 @@
if (debug_visitor_ != nullptr) {
debug_visitor_->OnHandshakeDoneFrame(frame);
}
+ if (advance_ack_timeout_update_) {
+ MaybeUpdateAckTimeout();
+ }
visitor_->OnHandshakeDoneReceived();
- should_last_packet_instigate_acks_ = true;
+ if (!advance_ack_timeout_update_) {
+ should_last_packet_instigate_acks_ = true;
+ }
return connected_;
}
@@ -1432,9 +1480,14 @@
}
QUIC_DLOG(INFO) << ENDPOINT
<< "BLOCKED_FRAME received for stream: " << frame.stream_id;
+ if (advance_ack_timeout_update_) {
+ MaybeUpdateAckTimeout();
+ }
visitor_->OnBlockedFrame(frame);
stats_.blocked_frames_received++;
- should_last_packet_instigate_acks_ = true;
+ if (!advance_ack_timeout_update_) {
+ should_last_packet_instigate_acks_ = true;
+ }
return connected_;
}
@@ -1506,10 +1559,12 @@
// For IETF QUIC, it is guaranteed that TLS will give connection the
// corresponding write key before read key. In other words, connection should
// never process a packet while an ACK for it cannot be encrypted.
- uber_received_packet_manager_.MaybeUpdateAckTimeout(
- should_last_packet_instigate_acks_, last_decrypted_packet_level_,
- last_header_.packet_number, GetTimeOfLastReceivedPacket(),
- clock_->ApproximateNow(), sent_packet_manager_.GetRttStats());
+ if (!advance_ack_timeout_update_ || !should_last_packet_instigate_acks_) {
+ uber_received_packet_manager_.MaybeUpdateAckTimeout(
+ should_last_packet_instigate_acks_, last_decrypted_packet_level_,
+ last_header_.packet_number, GetTimeOfLastReceivedPacket(),
+ clock_->ApproximateNow(), sent_packet_manager_.GetRttStats());
+ }
ClearLastFrames();
CloseIfTooManyOutstandingSentPackets();
@@ -4319,6 +4374,18 @@
idle_timeout_connection_close_behavior_);
}
+void QuicConnection::MaybeUpdateAckTimeout() {
+ DCHECK(advance_ack_timeout_update_);
+ if (should_last_packet_instigate_acks_) {
+ return;
+ }
+ 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, GetTimeOfLastReceivedPacket(),
+ clock_->ApproximateNow(), sent_packet_manager_.GetRttStats());
+}
+
QuicTime QuicConnection::GetPathDegradingDeadline() const {
DCHECK(use_blackhole_detector_);
if (!ShouldDetectPathDegrading()) {
diff --git a/quic/core/quic_connection.h b/quic/core/quic_connection.h
index bc7245a..46031f3 100644
--- a/quic/core/quic_connection.h
+++ b/quic/core/quic_connection.h
@@ -1226,6 +1226,9 @@
// and flags.
void MaybeEnableMultiplePacketNumberSpacesSupport();
+ // Called to update ACK timeout when an retransmittable frame has been parsed.
+ void MaybeUpdateAckTimeout();
+
// Returns packet fate when trying to write a packet via WritePacket().
SerializedPacketFate DeterminePacketFate(bool is_mtu_discovery);
@@ -1661,6 +1664,9 @@
const bool extend_idle_time_on_decryptable_packets_ =
GetQuicReloadableFlag(quic_extend_idle_time_on_decryptable_packets);
+
+ const bool advance_ack_timeout_update_ =
+ GetQuicReloadableFlag(quic_advance_ack_timeout_update);
};
} // namespace quic
diff --git a/quic/core/quic_connection_test.cc b/quic/core/quic_connection_test.cc
index f3bb4e1..5eb9e62 100644
--- a/quic/core/quic_connection_test.cc
+++ b/quic/core/quic_connection_test.cc
@@ -10775,6 +10775,27 @@
}
}
+TEST_P(QuicConnectionTest, BundleAckWithImmediateResponse) {
+ EXPECT_CALL(visitor_, OnSuccessfulVersionNegotiation(_));
+ connection_.SetDefaultEncryptionLevel(ENCRYPTION_FORWARD_SECURE);
+
+ EXPECT_CALL(visitor_, OnStreamFrame(_)).WillOnce(Invoke([this]() {
+ connection_.SendControlFrame(QuicFrame(new QuicWindowUpdateFrame(1, 0, 0)));
+ }));
+ EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, _, _, _)).Times(1);
+ ProcessDataPacket(1);
+ QuicAlarm* ack_alarm = QuicConnectionPeer::GetAckAlarm(&connection_);
+ if (GetQuicReloadableFlag(quic_advance_ack_timeout_update)) {
+ // Verify ACK is bundled with WINDOW_UPDATE.
+ EXPECT_FALSE(writer_->ack_frames().empty());
+ EXPECT_FALSE(ack_alarm->IsSet());
+ } else {
+ // ACK is pending.
+ EXPECT_TRUE(writer_->ack_frames().empty());
+ EXPECT_TRUE(ack_alarm->IsSet());
+ }
+}
+
} // namespace
} // namespace test
} // namespace quic
diff --git a/quic/quartc/test/quartc_peer_test.cc b/quic/quartc/test/quartc_peer_test.cc
index 76537c2..f3e5b68 100644
--- a/quic/quartc/test/quartc_peer_test.cc
+++ b/quic/quartc/test/quartc_peer_test.cc
@@ -283,7 +283,11 @@
EXPECT_GE(client_messages[i].frame.send_time, start_time);
EXPECT_LE(client_messages[i].receive_time, end_time);
}
-
+ if (GetQuicReloadableFlag(quic_advance_ack_timeout_update)) {
+ // ACK frame bundling changes packet sequencing.
+ // TODO(fayang): Fix this test.
+ return;
+ }
std::vector<ReceivedMessage> server_messages =
server_peer_->received_messages();
std::sort(server_messages.begin(), server_messages.end(), order);
