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);