Do not bundle ACK in connection close packet if there is an queued ACK.
Protected by FLAGS_quic_reloadable_flag_quic_single_ack_in_packet.
PiperOrigin-RevId: 343893573
Change-Id: I920a8a6c171ca0944fe596dcbe5bc132e3d5b6a6
diff --git a/quic/core/quic_connection.cc b/quic/core/quic_connection.cc
index 90a1739..bca806e 100644
--- a/quic/core/quic_connection.cc
+++ b/quic/core/quic_connection.cc
@@ -3935,9 +3935,14 @@
// If there was a packet write error, write the smallest close possible.
ScopedPacketFlusher flusher(this);
// Always bundle an ACK with connection close for debugging purpose.
- if (error != QUIC_PACKET_WRITE_ERROR &&
- !uber_received_packet_manager_.IsAckFrameEmpty(
- QuicUtils::GetPacketNumberSpace(encryption_level_))) {
+ bool send_ack = error != QUIC_PACKET_WRITE_ERROR &&
+ !uber_received_packet_manager_.IsAckFrameEmpty(
+ QuicUtils::GetPacketNumberSpace(encryption_level_));
+ if (GetQuicReloadableFlag(quic_single_ack_in_packet)) {
+ QUIC_RELOADABLE_FLAG_COUNT_N(quic_single_ack_in_packet, 1, 2);
+ send_ack = !packet_creator_.has_ack() && send_ack;
+ }
+ if (send_ack) {
SendAck();
}
QuicConnectionCloseFrame* frame;
@@ -3977,9 +3982,14 @@
use_encryption_level_context_ ? this : nullptr, level);
// Bundle an ACK of the corresponding packet number space for debugging
// purpose.
- if (error != QUIC_PACKET_WRITE_ERROR &&
- !uber_received_packet_manager_.IsAckFrameEmpty(
- QuicUtils::GetPacketNumberSpace(encryption_level_))) {
+ bool send_ack = error != QUIC_PACKET_WRITE_ERROR &&
+ !uber_received_packet_manager_.IsAckFrameEmpty(
+ QuicUtils::GetPacketNumberSpace(encryption_level_));
+ if (GetQuicReloadableFlag(quic_single_ack_in_packet)) {
+ QUIC_RELOADABLE_FLAG_COUNT_N(quic_single_ack_in_packet, 2, 2);
+ send_ack = !packet_creator_.has_ack() && send_ack;
+ }
+ if (send_ack) {
QuicFrames frames;
frames.push_back(GetUpdatedAckFrame());
packet_creator_.FlushAckFrame(frames);
diff --git a/quic/core/quic_connection_test.cc b/quic/core/quic_connection_test.cc
index a329404..2d73601 100644
--- a/quic/core/quic_connection_test.cc
+++ b/quic/core/quic_connection_test.cc
@@ -12826,6 +12826,32 @@
EXPECT_FALSE(connection_.IsPathDegrading());
}
+TEST_P(QuicConnectionTest, SingleAckInPacket) {
+ EXPECT_CALL(visitor_, OnSuccessfulVersionNegotiation(_));
+ EXPECT_CALL(visitor_, OnConnectionClosed(_, _));
+ connection_.SetDefaultEncryptionLevel(ENCRYPTION_FORWARD_SECURE);
+ connection_.RemoveEncrypter(ENCRYPTION_INITIAL);
+ connection_.NeuterUnencryptedPackets();
+ connection_.OnHandshakeComplete();
+
+ EXPECT_CALL(visitor_, OnStreamFrame(_)).WillOnce(Invoke([=]() {
+ connection_.SendStreamData3();
+ connection_.CloseConnection(
+ QUIC_INTERNAL_ERROR, "error",
+ ConnectionCloseBehavior::SEND_CONNECTION_CLOSE_PACKET);
+ }));
+ QuicFrames frames;
+ frames.push_back(QuicFrame(frame1_));
+ ProcessFramesPacketWithAddresses(frames, kSelfAddress, kPeerAddress,
+ ENCRYPTION_FORWARD_SECURE);
+ ASSERT_FALSE(writer_->ack_frames().empty());
+ if (GetQuicReloadableFlag(quic_single_ack_in_packet)) {
+ EXPECT_EQ(1u, writer_->ack_frames().size());
+ } else {
+ EXPECT_EQ(2u, writer_->ack_frames().size());
+ }
+}
+
} // namespace
} // namespace test
} // namespace quic
diff --git a/quic/core/quic_flags_list.h b/quic/core/quic_flags_list.h
index 473106f..3327c5f 100644
--- a/quic/core/quic_flags_list.h
+++ b/quic/core/quic_flags_list.h
@@ -68,6 +68,7 @@
QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_send_path_response, false)
QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_send_timestamps, false)
QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_send_version_negotiation_for_short_connection_ids, true)
+QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_single_ack_in_packet, false)
QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_split_up_send_rst_2, false)
QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_start_peer_migration_earlier, false)
QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_stop_sending_uses_ietf_error_code, true)
diff --git a/quic/core/quic_packet_creator.cc b/quic/core/quic_packet_creator.cc
index 087b427..e1e3478 100644
--- a/quic/core/quic_packet_creator.cc
+++ b/quic/core/quic_packet_creator.cc
@@ -1426,6 +1426,8 @@
bool QuicPacketCreator::FlushAckFrame(const QuicFrames& frames) {
QUIC_BUG_IF(!flusher_attached_) << "Packet flusher is not attached when "
"generator tries to send ACK frame.";
+ QUIC_BUG_IF(GetQuicReloadableFlag(quic_single_ack_in_packet) && has_ack())
+ << "Trying to flush " << frames << " when there is ACK queued";
for (const auto& frame : frames) {
DCHECK(frame.type == ACK_FRAME || frame.type == STOP_WAITING_FRAME);
if (HasPendingFrames()) {
