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
