Add option to clone all CRYPTO packets at most once

Summary: Restore the buggy version of `cloneAllPacketsWithCryptoFrame`, which did allow up to two packets to be cloned in the same PTO flight but would only clone them at most once, as an optional mode of operation.

Reviewed By: mjoras

Differential Revision: D67994014

fbshipit-source-id: 16f66fd48a9a4410427ba21dea6e7f6d1eee48e6

quic/api/QuicPacketScheduler.cpp

@@ -938,6 +938,9 @@ SchedulingResult CloningScheduler::scheduleFramesForPacket(
         if (std::find_if(frames.begin(), frames.end(), [](const auto& frame) {
               return frame.type() == QuicWriteFrame::Type::WriteCryptoFrame;
             }) != frames.end()) {
+          if (conn_.transportSettings.cloneCryptoPacketsAtMostOnce) {
+            continue;
+          }
           auto mostRecentOutstandingPacketIdentifier =
               conn_.outstandings.packets.back().maybeClonedPacketIdentifier;
           if (mostRecentOutstandingPacketIdentifier ==

quic/api/test/QuicPacketSchedulerTest.cpp

@@ -630,14 +630,18 @@ TEST_F(QuicPacketSchedulerTest, DoNotCloneProcessedClonedPacket) {
   EXPECT_EQ(expected, result.clonedPacketIdentifier->packetNumber);
 }
 
-class CloneAllPacketsWithCryptoFrameTest : public QuicPacketSchedulerTest,
-                                           public WithParamInterface<bool> {};
+class CloneAllPacketsWithCryptoFrameTest
+    : public QuicPacketSchedulerTest,
+      public WithParamInterface<std::tuple<bool, bool>> {};
 TEST_P(
     CloneAllPacketsWithCryptoFrameTest,
     TestCloneAllPacketsWithCryptoFrameTrueFalse) {
   QuicClientConnectionState conn(
       FizzClientQuicHandshakeContext::Builder().build());
-  conn.transportSettings.cloneAllPacketsWithCryptoFrame = GetParam();
+  auto testParams = GetParam();
+  conn.transportSettings.cloneAllPacketsWithCryptoFrame =
+      std::get<0>(testParams);
+  conn.transportSettings.cloneCryptoPacketsAtMostOnce = std::get<1>(testParams);
   FrameScheduler noopScheduler("frame", conn);
   CloningScheduler cloningScheduler(noopScheduler, conn, "cryptoClone", 0);
 
@@ -671,7 +675,7 @@ TEST_P(
         MaxDataFrame(conn.flowControlState.advertisedMaxOffset));
   }
 
-  // Add a third outstanding packet, which is a clone of the first packet
+  // Add a third outstanding packet, which is a re-clone of the first packet
   {
     addInitialOutstandingPacket(conn);
     conn.outstandings.packets.back().packet.frames.push_back(
@@ -687,6 +691,7 @@ TEST_P(
         MaxDataFrame(conn.flowControlState.advertisedMaxOffset));
   }
 
+  // Schedule a fourth packet
   std::vector<uint8_t> zeroConnIdData(quic::kDefaultConnectionIdSize, 0);
   ConnectionId srcConnId(zeroConnIdData);
   LongHeader header(
@@ -701,18 +706,25 @@ TEST_P(
       conn.ackStates.initialAckState->largestAckedByPeer.value_or(0));
   auto result = cloningScheduler.scheduleFramesForPacket(
       std::move(builder), kDefaultUDPSendPacketLen);
-  EXPECT_TRUE(
-      result.clonedPacketIdentifier.has_value() && result.packet.has_value());
-  EXPECT_EQ(
-      conn.transportSettings.cloneAllPacketsWithCryptoFrame ? secondPacketNum
-                                                            : firstPacketNum,
-      result.clonedPacketIdentifier->packetNumber);
+  if (conn.transportSettings.cloneAllPacketsWithCryptoFrame &&
+      conn.transportSettings.cloneCryptoPacketsAtMostOnce) {
+    // First and second packets already cloned, skip all and schedule no packet
+    EXPECT_FALSE(result.clonedPacketIdentifier.has_value());
+    EXPECT_FALSE(result.packet.has_value());
+  } else {
+    EXPECT_TRUE(
+        result.clonedPacketIdentifier.has_value() && result.packet.has_value());
+    EXPECT_EQ(
+        conn.transportSettings.cloneAllPacketsWithCryptoFrame ? secondPacketNum
+                                                              : firstPacketNum,
+        result.clonedPacketIdentifier->packetNumber);
+  }
 }
 
 INSTANTIATE_TEST_SUITE_P(
     CloneAllPacketsWithCryptoFrameTest,
     CloneAllPacketsWithCryptoFrameTest,
-    Bool());
+    Combine(Bool(), Bool()));
 
 TEST_F(QuicPacketSchedulerTest, DoNotSkipUnclonedCryptoPacket) {
   QuicClientConnectionState conn(

quic/state/TransportSettings.h

@@ -413,6 +413,7 @@ struct TransportSettings {
   // receiving and then processing.
   bool networkDataPerSocketRead{false};
   bool cloneAllPacketsWithCryptoFrame{false};
+  bool cloneCryptoPacketsAtMostOnce{false};
   // Use a reordering threshold heuristic of inflight / 2.
   bool useInflightReorderingThreshold{false};
   // Raise read callbacks for all unidirectional streams first on data