gfe-relnote: In QUIC, always bundle ACK with connection close with TLS handshake for debugging purpose. Protected by blocked gfe2_reloadable_flag_quic_enable_version_t*.
PiperOrigin-RevId: 294942214
Change-Id: Ided5cdb8d4c8a50325e63f4c9676da0d7716c7c6
diff --git a/quic/core/quic_connection.cc b/quic/core/quic_connection.cc
index fe0ea7b..0bfb6ea 100644
--- a/quic/core/quic_connection.cc
+++ b/quic/core/quic_connection.cc
@@ -2917,10 +2917,8 @@
ClearQueuedPackets();
// If there was a packet write error, write the smallest close possible.
ScopedPacketFlusher flusher(this);
- // When multiple packet number spaces is supported, an ACK frame will be
- // bundled when connection is not write blocked.
- if (!SupportsMultiplePacketNumberSpaces() &&
- error != QUIC_PACKET_WRITE_ERROR &&
+ // Always bundle an ACK with connection close for debugging purpose.
+ if (error != QUIC_PACKET_WRITE_ERROR &&
!GetUpdatedAckFrame().ack_frame->packets.Empty()) {
SendAck();
}
@@ -2955,15 +2953,17 @@
QUIC_DLOG(INFO) << ENDPOINT << "Sending connection close packet at level: "
<< EncryptionLevelToString(level);
SetDefaultEncryptionLevel(level);
- // If there was a packet write error, write the smallest close possible.
- // When multiple packet number spaces are supported, an ACK frame will
- // be bundled by the ScopedPacketFlusher. Otherwise, an ACK must be sent
- // explicitly.
- if (!SupportsMultiplePacketNumberSpaces() &&
- error != QUIC_PACKET_WRITE_ERROR &&
- !GetUpdatedAckFrame().ack_frame->packets.Empty()) {
- SendAck();
+ // Bundle an ACK of the corresponding packet number space for debugging
+ // purpose.
+ if (error != QUIC_PACKET_WRITE_ERROR) {
+ const QuicFrame ack_frame = GetUpdatedAckFrame();
+ if (!ack_frame.ack_frame->packets.Empty()) {
+ QuicFrames frames;
+ frames.push_back(ack_frame);
+ packet_creator_.FlushAckFrame(frames);
+ }
}
+
auto* frame =
new QuicConnectionCloseFrame(transport_version(), error, details,
framer_.current_received_frame_type());
diff --git a/quic/core/quic_connection_test.cc b/quic/core/quic_connection_test.cc
index 704241c..67e2ab0 100644
--- a/quic/core/quic_connection_test.cc
+++ b/quic/core/quic_connection_test.cc
@@ -9973,6 +9973,58 @@
ProcessFramesPacketAtLevel(1, frames, ENCRYPTION_FORWARD_SECURE);
}
+TEST_P(QuicConnectionTest,
+ BundleAckWithConnectionCloseMultiplePacketNumberSpace) {
+ if (!connection_.SupportsMultiplePacketNumberSpaces()) {
+ return;
+ }
+ EXPECT_CALL(visitor_, OnSuccessfulVersionNegotiation(_));
+ EXPECT_CALL(visitor_, OnCryptoFrame(_)).Times(AnyNumber());
+ EXPECT_CALL(visitor_, OnStreamFrame(_)).Times(AnyNumber());
+ // Receives packet 1000 in initial data.
+ ProcessCryptoPacketAtLevel(1000, ENCRYPTION_INITIAL);
+ // Receives packet 2000 in application data.
+ ProcessDataPacketAtLevel(2000, false, ENCRYPTION_FORWARD_SECURE);
+ EXPECT_CALL(visitor_, OnConnectionClosed(_, _));
+ const QuicErrorCode kQuicErrorCode = QUIC_INTERNAL_ERROR;
+ connection_.CloseConnection(
+ kQuicErrorCode, "Some random error message",
+ ConnectionCloseBehavior::SEND_CONNECTION_CLOSE_PACKET);
+
+ EXPECT_EQ(2u, QuicConnectionPeer::GetNumEncryptionLevels(&connection_));
+
+ TestConnectionCloseQuicErrorCode(kQuicErrorCode);
+ EXPECT_EQ(1u, writer_->connection_close_frames().size());
+ // Verify ack is bundled.
+ EXPECT_EQ(1u, writer_->ack_frames().size());
+
+ if (!connection_.version().CanSendCoalescedPackets()) {
+ // Each connection close packet should be sent in distinct UDP packets.
+ EXPECT_EQ(QuicConnectionPeer::GetNumEncryptionLevels(&connection_),
+ writer_->connection_close_packets());
+ EXPECT_EQ(QuicConnectionPeer::GetNumEncryptionLevels(&connection_),
+ writer_->packets_write_attempts());
+ return;
+ }
+
+ // A single UDP packet should be sent with multiple connection close packets
+ // coalesced together.
+ EXPECT_EQ(1u, writer_->packets_write_attempts());
+
+ // Only the first packet has been processed yet.
+ EXPECT_EQ(1u, writer_->connection_close_packets());
+
+ // ProcessPacket resets the visitor and frees the coalesced packet.
+ ASSERT_TRUE(writer_->coalesced_packet() != nullptr);
+ auto packet = writer_->coalesced_packet()->Clone();
+ writer_->framer()->ProcessPacket(*packet);
+ EXPECT_EQ(1u, writer_->connection_close_packets());
+ EXPECT_EQ(1u, writer_->connection_close_frames().size());
+ // Verify ack is bundled.
+ EXPECT_EQ(1u, writer_->ack_frames().size());
+ ASSERT_TRUE(writer_->coalesced_packet() == nullptr);
+}
+
} // namespace
} // namespace test
} // namespace quic
