Add option to allow PTOing pure PING packets.

Summary: As in title. Without this the keepalive option is less effective since a single PING packet loss can cause issues.

Reviewed By: kvtsoy

Differential Revision: D63397494

fbshipit-source-id: 7ef6b6f54189609e3a96409ac9c035c475dc0569

quic/api/QuicTransportFunctions.cpp

@@ -784,6 +784,9 @@ void updateConnection(
       case QuicWriteFrame::Type::PingFrame:
         conn.pendingEvents.sendPing = false;
         isPing = true;
+        if (conn.transportSettings.ptoPingFrames) {
+          retransmittable = true;
+        }
         break;
       case QuicWriteFrame::Type::QuicSimpleFrame: {
         const QuicSimpleFrame& simpleFrame = *frame.asQuicSimpleFrame();

quic/api/test/QuicTransportFunctionsTest.cpp

@@ -218,6 +218,18 @@ TEST_F(QuicTransportFunctionsTest, PingPacketGoesToOPList) {
   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());
+  updateConnection(
+      *conn, none, packet.packet, Clock::now(), 50, 0, false /* isDSRPacket */);
+  EXPECT_EQ(1, conn->outstandings.packets.size());
+  EXPECT_TRUE(conn->pendingEvents.setLossDetectionAlarm);
+}
+
 TEST_F(QuicTransportFunctionsTest, TestUpdateConnection) {
   auto conn = createConn();
   auto mockCongestionController =

quic/codec/QuicPacketRebuilder.cpp

@@ -176,6 +176,9 @@ Optional<ClonedPacketIdentifier> PacketRebuilder::rebuildFromPacket(
       case QuicWriteFrame::Type::PingFrame: {
         const PingFrame& pingFrame = *frame.asPingFrame();
         writeSuccess = writeFrame(pingFrame, builder_) != 0;
+        if (conn_.transportSettings.ptoPingFrames) {
+          notPureAck |= writeSuccess;
+        }
         break;
       }
       case QuicWriteFrame::Type::QuicSimpleFrame: {

quic/codec/test/QuicPacketRebuilderTest.cpp

@@ -546,6 +546,31 @@ TEST_F(QuicPacketRebuilderTest, PurePingWontRebuild) {
   EXPECT_EQ(0, conn.outstandings.numClonedPackets());
 }
 
+TEST_F(QuicPacketRebuilderTest, PurePingWillRebuild) {
+  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());
+  conn.transportSettings.ptoPingFrames = true;
+  ShortHeader shortHeader2(
+      ProtectionType::KeyPhaseZero, getTestConnectionId(), 0);
+  RegularQuicPacketBuilder regularBuilder2(
+      kDefaultUDPSendPacketLen, std::move(shortHeader2), 0);
+  regularBuilder2.encodePacketHeader();
+  PacketRebuilder rebuilder(regularBuilder2, conn);
+  EXPECT_TRUE(rebuilder.rebuildFromPacket(outstandingPacket).has_value());
+  EXPECT_TRUE(outstandingPacket.maybeClonedPacketIdentifier.has_value());
+  EXPECT_EQ(1, conn.outstandings.numClonedPackets());
+}
+
 TEST_F(QuicPacketRebuilderTest, LastStreamFrameSkipLen) {
   QuicServerConnectionState conn(
       FizzServerQuicHandshakeContext::Builder().build());

quic/state/TransportSettings.h

@@ -410,6 +410,8 @@ 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};
 };
 
 } // namespace quic