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

quic/core/quic_connection.cc

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

quic/core/quic_connection_test.cc

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