Add missing MaybeUpdateAckTimeout for ack-eliciting frames. Protected by FLAGS_quic_reloadable_flag_quic_add_missing_update_ack_timeout. PiperOrigin-RevId: 384760052
diff --git a/quic/core/frames/quic_new_connection_id_frame.h b/quic/core/frames/quic_new_connection_id_frame.h index 9c3d205..235d2af 100644 --- a/quic/core/frames/quic_new_connection_id_frame.h +++ b/quic/core/frames/quic_new_connection_id_frame.h
@@ -32,7 +32,7 @@ QuicConnectionId connection_id = EmptyQuicConnectionId(); QuicConnectionIdSequenceNumber sequence_number = 0; StatelessResetToken stateless_reset_token; - uint64_t retire_prior_to; + uint64_t retire_prior_to = 0; }; } // namespace quic
diff --git a/quic/core/frames/quic_streams_blocked_frame.h b/quic/core/frames/quic_streams_blocked_frame.h index 94676fb..2d73890 100644 --- a/quic/core/frames/quic_streams_blocked_frame.h +++ b/quic/core/frames/quic_streams_blocked_frame.h
@@ -35,7 +35,7 @@ QuicControlFrameId control_frame_id = kInvalidControlFrameId; // The number of streams that the sender wishes to exceed - QuicStreamCount stream_count; + QuicStreamCount stream_count = 0; // Whether uni- or bi-directional streams bool unidirectional = false;
diff --git a/quic/core/quic_connection.cc b/quic/core/quic_connection.cc index 9defbe3..96428a3 100644 --- a/quic/core/quic_connection.cc +++ b/quic/core/quic_connection.cc
@@ -378,6 +378,10 @@ QUIC_RELOADABLE_FLAG_COUNT(quic_use_encryption_level_context); } + if (add_missing_update_ack_timeout_) { + QUIC_RELOADABLE_FLAG_COUNT(quic_add_missing_update_ack_timeout); + } + support_multiple_connection_ids_ = version().HasIetfQuicFrames() && GetQuicRestartFlag(quic_time_wait_list_support_multiple_cid_v2) && @@ -1399,6 +1403,9 @@ ConnectionCloseBehavior::SEND_CONNECTION_CLOSE_PACKET); return false; } + // TODO(fayang): Consider moving UpdatePacketContent and + // MaybeUpdateAckTimeout to a stand-alone function instead of calling them for + // all frames. MaybeUpdateAckTimeout(); visitor_->OnStreamFrame(frame); stats_.stream_bytes_received += frame.data_length; @@ -1707,7 +1714,9 @@ QUIC_DLOG(INFO) << ENDPOINT << "STOP_SENDING frame received for stream: " << frame.stream_id << " with error: " << frame.ietf_error_code; - + if (add_missing_update_ack_timeout_) { + MaybeUpdateAckTimeout(); + } visitor_->OnStopSendingFrame(frame); return connected_; } @@ -1931,6 +1940,9 @@ if (debug_visitor_ != nullptr) { debug_visitor_->OnMaxStreamsFrame(frame); } + if (add_missing_update_ack_timeout_) { + MaybeUpdateAckTimeout(); + } return visitor_->OnMaxStreamsFrame(frame) && connected_; } @@ -1947,6 +1959,9 @@ if (debug_visitor_ != nullptr) { debug_visitor_->OnStreamsBlockedFrame(frame); } + if (add_missing_update_ack_timeout_) { + MaybeUpdateAckTimeout(); + } return visitor_->OnStreamsBlockedFrame(frame) && connected_; } @@ -2077,6 +2092,9 @@ OnClientConnectionIdAvailable(); } QUIC_RELOADABLE_FLAG_COUNT_N(quic_connection_support_multiple_cids_v4, 1, 2); + if (add_missing_update_ack_timeout_) { + MaybeUpdateAckTimeout(); + } return true; } @@ -2137,6 +2155,9 @@ QUIC_RELOADABLE_FLAG_COUNT_N(quic_connection_support_multiple_cids_v4, 2, 2); // Count successfully received RETIRE_CONNECTION_ID frames. QUIC_RELOADABLE_FLAG_COUNT_N(quic_connection_migration_use_new_cid_v2, 5, 6); + if (add_missing_update_ack_timeout_) { + MaybeUpdateAckTimeout(); + } return true; }
diff --git a/quic/core/quic_connection.h b/quic/core/quic_connection.h index 9a02675..6e989ba 100644 --- a/quic/core/quic_connection.h +++ b/quic/core/quic_connection.h
@@ -2283,6 +2283,9 @@ const bool reset_per_packet_state_for_undecryptable_packets_ = GetQuicReloadableFlag( quic_reset_per_packet_state_for_undecryptable_packets); + + const bool add_missing_update_ack_timeout_ = + GetQuicReloadableFlag(quic_add_missing_update_ack_timeout); }; } // namespace quic
diff --git a/quic/core/quic_connection_test.cc b/quic/core/quic_connection_test.cc index 5e6c8ee..f16604b 100644 --- a/quic/core/quic_connection_test.cc +++ b/quic/core/quic_connection_test.cc
@@ -23,6 +23,7 @@ #include "quic/core/frames/quic_connection_close_frame.h" #include "quic/core/frames/quic_new_connection_id_frame.h" #include "quic/core/frames/quic_path_response_frame.h" +#include "quic/core/frames/quic_rst_stream_frame.h" #include "quic/core/quic_connection_id.h" #include "quic/core/quic_constants.h" #include "quic/core/quic_error_codes.h" @@ -12079,6 +12080,7 @@ new_writer.BlockOnNextWrite(); EXPECT_CALL(visitor_, OnWriteBlocked()).Times(0); EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, _, _, _)) + .Times(AtLeast(1)) .WillOnce(Invoke([&]() { // Even though the socket is blocked, the PATH_CHALLENGE should still be // treated as sent. @@ -12099,7 +12101,11 @@ new_writer.SetWritable(); // Write event on the default socket shouldn't make any difference. connection_.OnCanWrite(); - EXPECT_EQ(0u, writer_->packets_write_attempts()); + if (GetQuicReloadableFlag(quic_add_missing_update_ack_timeout)) { + EXPECT_EQ(1u, writer_->packets_write_attempts()); + } else { + EXPECT_EQ(0u, writer_->packets_write_attempts()); + } EXPECT_EQ(1u, new_writer.packets_write_attempts()); } @@ -15206,6 +15212,149 @@ EXPECT_EQ(0x01010101u, writer_->final_bytes_of_last_packet()); } +TEST_P(QuicConnectionTest, AckElicitingFrames) { + if (!version().HasIetfQuicFrames() || + !connection_.support_multiple_connection_ids() || + !GetQuicReloadableFlag(quic_add_missing_update_ack_timeout)) { + return; + } + EXPECT_CALL(visitor_, OnRstStream(_)); + EXPECT_CALL(visitor_, OnWindowUpdateFrame(_)); + EXPECT_CALL(visitor_, OnBlockedFrame(_)); + EXPECT_CALL(visitor_, OnHandshakeDoneReceived()); + EXPECT_CALL(visitor_, OnStreamFrame(_)); + EXPECT_CALL(*send_algorithm_, OnCongestionEvent(_, _, _, _, _)); + EXPECT_CALL(visitor_, OnMaxStreamsFrame(_)); + EXPECT_CALL(visitor_, OnStreamsBlockedFrame(_)); + EXPECT_CALL(visitor_, OnStopSendingFrame(_)); + EXPECT_CALL(visitor_, OnMessageReceived("")); + EXPECT_CALL(visitor_, OnNewTokenReceived("")); + + connection_.CreateConnectionIdManager(); + connection_.set_can_receive_ack_frequency_frame(); + + QuicAckFrame ack_frame = InitAckFrame(1); + QuicRstStreamFrame rst_stream_frame; + QuicWindowUpdateFrame window_update_frame; + QuicPathChallengeFrame path_challenge_frame; + QuicStopSendingFrame stop_sending_frame; + QuicNewConnectionIdFrame new_connection_id_frame; + QuicPathResponseFrame path_response_frame; + QuicMessageFrame message_frame; + QuicNewTokenFrame new_token_frame; + QuicAckFrequencyFrame ack_frequency_frame; + QuicBlockedFrame blocked_frame; + size_t packet_number = 1; + + connection_.SetDefaultEncryptionLevel(ENCRYPTION_FORWARD_SECURE); + + for (uint8_t i = 0; i < NUM_FRAME_TYPES; ++i) { + QuicFrameType frame_type = static_cast<QuicFrameType>(i); + bool skipped = false; + QuicFrame frame; + QuicFrames frames; + // Add some padding to fullfill the min size requirement of header + // protection. + frames.push_back(QuicFrame(QuicPaddingFrame(10))); + switch (frame_type) { + case PADDING_FRAME: + frame = QuicFrame(QuicPaddingFrame(10)); + break; + case MTU_DISCOVERY_FRAME: + frame = QuicFrame(QuicMtuDiscoveryFrame()); + break; + case PING_FRAME: + frame = QuicFrame(QuicPingFrame()); + break; + case MAX_STREAMS_FRAME: + frame = QuicFrame(QuicMaxStreamsFrame()); + break; + case STOP_WAITING_FRAME: + // Not supported. + skipped = true; + break; + case STREAMS_BLOCKED_FRAME: + frame = QuicFrame(QuicStreamsBlockedFrame()); + break; + case STREAM_FRAME: + frame = QuicFrame(QuicStreamFrame()); + break; + case HANDSHAKE_DONE_FRAME: + frame = QuicFrame(QuicHandshakeDoneFrame()); + break; + case ACK_FRAME: + frame = QuicFrame(&ack_frame); + break; + case RST_STREAM_FRAME: + frame = QuicFrame(&rst_stream_frame); + break; + case CONNECTION_CLOSE_FRAME: + // Do not test connection close. + skipped = true; + break; + case GOAWAY_FRAME: + // Does not exist in IETF QUIC. + skipped = true; + break; + case BLOCKED_FRAME: + frame = QuicFrame(&blocked_frame); + break; + case WINDOW_UPDATE_FRAME: + frame = QuicFrame(&window_update_frame); + break; + case PATH_CHALLENGE_FRAME: + frame = QuicFrame(&path_challenge_frame); + break; + case STOP_SENDING_FRAME: + frame = QuicFrame(&stop_sending_frame); + break; + case NEW_CONNECTION_ID_FRAME: + frame = QuicFrame(&new_connection_id_frame); + break; + case RETIRE_CONNECTION_ID_FRAME: + // TODO(haoyuewang): add test coverage for RETIRE_CONNECTION_ID_FRAME. + skipped = true; + break; + case PATH_RESPONSE_FRAME: + frame = QuicFrame(&path_response_frame); + break; + case MESSAGE_FRAME: + frame = QuicFrame(&message_frame); + break; + case CRYPTO_FRAME: + // CRYPTO_FRAME is ack eliciting is covered by other tests. + skipped = true; + break; + case NEW_TOKEN_FRAME: + frame = QuicFrame(&new_token_frame); + break; + case ACK_FREQUENCY_FRAME: + frame = QuicFrame(&ack_frequency_frame); + break; + case NUM_FRAME_TYPES: + skipped = true; + break; + } + if (skipped) { + continue; + } + ASSERT_EQ(frame_type, frame.type); + frames.push_back(frame); + EXPECT_FALSE(connection_.HasPendingAcks()); + // Process frame. + ProcessFramesPacketAtLevel(packet_number++, frames, + ENCRYPTION_FORWARD_SECURE); + if (QuicUtils::IsAckElicitingFrame(frame_type)) { + ASSERT_TRUE(connection_.HasPendingAcks()) << frame; + // Flush ACK. + clock_.AdvanceTime(DefaultDelayedAckTime()); + connection_.GetAckAlarm()->Fire(); + } + EXPECT_FALSE(connection_.HasPendingAcks()); + ASSERT_TRUE(connection_.connected()); + } +} + } // namespace } // namespace test } // namespace quic
diff --git a/quic/core/quic_flags_list.h b/quic/core/quic_flags_list.h index 55e7751..8abba28 100644 --- a/quic/core/quic_flags_list.h +++ b/quic/core/quic_flags_list.h
@@ -27,6 +27,8 @@ QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_abort_qpack_on_stream_reset, true) // If true, ack frequency frame can be sent from server to client. QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_can_send_ack_frequency, true) +// If true, add missing MaybeUpdateAckTimeout for ack-eliciting frames. +QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_add_missing_update_ack_timeout, true) // If true, allow client to enable BBRv2 on server via connection option \'B2ON\'. QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_allow_client_enabled_bbr_v2, false) // If true, allow ticket open to be ignored in TlsServerHandshaker. Also fixes TlsServerHandshaker::ResumptionAttempted when handshake hints is used.
diff --git a/quic/core/quic_utils.cc b/quic/core/quic_utils.cc index 65c9ced..98fbf55 100644 --- a/quic/core/quic_utils.cc +++ b/quic/core/quic_utils.cc
@@ -695,6 +695,19 @@ } // static +bool QuicUtils::IsAckElicitingFrame(QuicFrameType type) { + switch (type) { + case PADDING_FRAME: + case STOP_WAITING_FRAME: + case ACK_FRAME: + case CONNECTION_CLOSE_FRAME: + return false; + default: + return true; + } +} + +// static bool QuicUtils::AreStatelessResetTokensEqual( const StatelessResetToken& token1, const StatelessResetToken& token2) {
diff --git a/quic/core/quic_utils.h b/quic/core/quic_utils.h index 6fcc5f2..c335bc0 100644 --- a/quic/core/quic_utils.h +++ b/quic/core/quic_utils.h
@@ -244,6 +244,9 @@ // comparison in constant time. static bool AreStatelessResetTokensEqual(const StatelessResetToken& token1, const StatelessResetToken& token2); + + // Return ture if this frame is an ack-eliciting frame. + static bool IsAckElicitingFrame(QuicFrameType type); }; // Returns true if the specific ID is a valid WebTransport session ID that our