From 28748bfad2bce7a5280bd404d1e6fc17e012347d Mon Sep 17 00:00:00 2001
From: Matt Joras <mjoras@meta.com>
Date: Tue, 8 Oct 2024 20:29:33 -0700
Subject: [PATCH] Default to PTOing PING frames.

Summary: This kinda makes sense to just use as a default.

Reviewed By: kvtsoy, sharmafb

Differential Revision: D64066392

fbshipit-source-id: 0915f163c0483af6bec014bde61e82b6ee2ac6cb
---
 quic/api/QuicTransportFunctions.cpp          |  4 +---
 quic/api/test/QuicTransportFunctionsTest.cpp | 13 ----------
 quic/codec/QuicPacketRebuilder.cpp           |  4 +---
 quic/codec/test/QuicPacketRebuilderTest.cpp  | 25 --------------------
 quic/state/TransportSettings.h               |  2 --
 5 files changed, 2 insertions(+), 46 deletions(-)

diff --git a/quic/api/QuicTransportFunctions.cpp b/quic/api/QuicTransportFunctions.cpp
index 6cbecbc83..fd60bb7a6 100644
--- a/quic/api/QuicTransportFunctions.cpp
+++ b/quic/api/QuicTransportFunctions.cpp
@@ -801,9 +801,7 @@ void updateConnection(
       case QuicWriteFrame::Type::PingFrame:
         conn.pendingEvents.sendPing = false;
         isPing = true;
-        if (conn.transportSettings.ptoPingFrames) {
-          retransmittable = true;
-        }
+        retransmittable = true;
         conn.numPingFramesSent++;
         break;
       case QuicWriteFrame::Type::QuicSimpleFrame: {
diff --git a/quic/api/test/QuicTransportFunctionsTest.cpp b/quic/api/test/QuicTransportFunctionsTest.cpp
index 2c34af839..28d7beba5 100644
--- a/quic/api/test/QuicTransportFunctionsTest.cpp
+++ b/quic/api/test/QuicTransportFunctionsTest.cpp
@@ -206,21 +206,8 @@ class QuicTransportFunctionsTest : public Test {
   std::unique_ptr<MockQuicStats> quicStats_;
 };
 
-TEST_F(QuicTransportFunctionsTest, PingPacketGoesToOPList) {
-  auto conn = createConn();
-  auto packet = buildEmptyPacket(*conn, PacketNumberSpace::AppData);
-  packet.packet.frames.push_back(PingFrame());
-  EXPECT_EQ(0, conn->outstandings.packets.size());
-  updateConnection(
-      *conn, none, packet.packet, Clock::now(), 50, 0, false /* isDSRPacket */);
-  EXPECT_EQ(1, conn->outstandings.packets.size());
-  // But it won't set loss detection alarm
-  EXPECT_FALSE(conn->pendingEvents.setLossDetectionAlarm);
-}
-
 TEST_F(QuicTransportFunctionsTest, PingPacketGoesToOPListAndLossAlarm) {
   auto conn = createConn();
-  conn->transportSettings.ptoPingFrames = true;
   auto packet = buildEmptyPacket(*conn, PacketNumberSpace::AppData);
   packet.packet.frames.push_back(PingFrame());
   EXPECT_EQ(0, conn->outstandings.packets.size());
diff --git a/quic/codec/QuicPacketRebuilder.cpp b/quic/codec/QuicPacketRebuilder.cpp
index c7f149dea..01b271ee1 100644
--- a/quic/codec/QuicPacketRebuilder.cpp
+++ b/quic/codec/QuicPacketRebuilder.cpp
@@ -176,9 +176,7 @@ Optional<ClonedPacketIdentifier> PacketRebuilder::rebuildFromPacket(
       case QuicWriteFrame::Type::PingFrame: {
         const PingFrame& pingFrame = *frame.asPingFrame();
         writeSuccess = writeFrame(pingFrame, builder_) != 0;
-        if (conn_.transportSettings.ptoPingFrames) {
-          notPureAck |= writeSuccess;
-        }
+        notPureAck |= writeSuccess;
         break;
       }
       case QuicWriteFrame::Type::QuicSimpleFrame: {
diff --git a/quic/codec/test/QuicPacketRebuilderTest.cpp b/quic/codec/test/QuicPacketRebuilderTest.cpp
index abc257544..8337eb04e 100644
--- a/quic/codec/test/QuicPacketRebuilderTest.cpp
+++ b/quic/codec/test/QuicPacketRebuilderTest.cpp
@@ -522,30 +522,6 @@ TEST_F(QuicPacketRebuilderTest, CloneCounter) {
   EXPECT_EQ(1, conn.outstandings.numClonedPackets());
 }
 
-TEST_F(QuicPacketRebuilderTest, PurePingWontRebuild) {
-  ShortHeader shortHeader1(
-      ProtectionType::KeyPhaseZero, getTestConnectionId(), 0);
-  RegularQuicPacketBuilder regularBuilder(
-      kDefaultUDPSendPacketLen, std::move(shortHeader1), 0);
-  regularBuilder.encodePacketHeader();
-  PingFrame pingFrame;
-  writeFrame(pingFrame, regularBuilder);
-  auto packet = std::move(regularBuilder).buildPacket();
-  auto outstandingPacket = makeDummyOutstandingPacket(packet.packet, 50);
-  EXPECT_EQ(1, outstandingPacket.packet.frames.size());
-  QuicServerConnectionState conn(
-      FizzServerQuicHandshakeContext::Builder().build());
-  ShortHeader shortHeader2(
-      ProtectionType::KeyPhaseZero, getTestConnectionId(), 0);
-  RegularQuicPacketBuilder regularBuilder2(
-      kDefaultUDPSendPacketLen, std::move(shortHeader2), 0);
-  regularBuilder2.encodePacketHeader();
-  PacketRebuilder rebuilder(regularBuilder2, conn);
-  EXPECT_EQ(none, rebuilder.rebuildFromPacket(outstandingPacket));
-  EXPECT_FALSE(outstandingPacket.maybeClonedPacketIdentifier.has_value());
-  EXPECT_EQ(0, conn.outstandings.numClonedPackets());
-}
-
 TEST_F(QuicPacketRebuilderTest, PurePingWillRebuild) {
   ShortHeader shortHeader1(
       ProtectionType::KeyPhaseZero, getTestConnectionId(), 0);
@@ -559,7 +535,6 @@ TEST_F(QuicPacketRebuilderTest, PurePingWillRebuild) {
   EXPECT_EQ(1, outstandingPacket.packet.frames.size());
   QuicServerConnectionState conn(
       FizzServerQuicHandshakeContext::Builder().build());
-  conn.transportSettings.ptoPingFrames = true;
   ShortHeader shortHeader2(
       ProtectionType::KeyPhaseZero, getTestConnectionId(), 0);
   RegularQuicPacketBuilder regularBuilder2(
diff --git a/quic/state/TransportSettings.h b/quic/state/TransportSettings.h
index 65efacab4..75d926f2c 100644
--- a/quic/state/TransportSettings.h
+++ b/quic/state/TransportSettings.h
@@ -410,8 +410,6 @@ struct TransportSettings {
   // receiving and then processing.
   bool networkDataPerSocketRead{false};
   bool cloneAllPacketsWithCryptoFrame{false};
-  // Whether we allow PTOing of packets with just PING frames.
-  bool ptoPingFrames{false};
   // Always send multiple PTOs even if we don't have multiple packets.
   bool alwaysPtoMultiple{false};
 };