Do not use the regular CloningScheduler to clone DSR packet

Summary: For now let it fallback to ping.

Reviewed By: mjoras

Differential Revision: D27481741

fbshipit-source-id: 874529a06ab882d9651e06f615bc82505c1c2872

quic/api/QuicPacketScheduler.cpp

@@ -689,7 +689,8 @@ bool CloningScheduler::hasData() const {
   // TODO: I'm not completely convinced d6d.outstandingProbes has been updated
   // correctly.
   return frameScheduler_.hasData() ||
-      conn_.outstandings.numOutstanding() > conn_.d6d.outstandingProbes;
+      conn_.outstandings.numOutstanding() >
+      conn_.d6d.outstandingProbes + conn_.outstandings.dsrCount;
 }
 
 SchedulingResult CloningScheduler::scheduleFramesForPacket(
@@ -712,7 +713,8 @@ SchedulingResult CloningScheduler::scheduleFramesForPacket(
   // Look for an outstanding packet that's no larger than the writableBytes
   for (auto& outstandingPacket : conn_.outstandings.packets) {
     if (outstandingPacket.declaredLost ||
-        outstandingPacket.metadata.isD6DProbe) {
+        outstandingPacket.metadata.isD6DProbe ||
+        outstandingPacket.isDSRPacket) {
       continue;
     }
     auto opPnSpace = outstandingPacket.packet.header.getPacketNumberSpace();

quic/api/QuicTransportFunctions.cpp

@@ -815,6 +815,9 @@ void updateConnection(
     conn.lossState.totalBytesCloned += encodedSize;
   }
   pkt.isDSRPacket = isDSRPacket;
+  if (isDSRPacket) {
+    ++conn.outstandings.dsrCount;
+  }
 
   if (conn.congestionController) {
     conn.congestionController->onPacketSent(pkt);

quic/api/test/QuicPacketSchedulerTest.cpp

@@ -478,6 +478,32 @@ TEST_F(QuicPacketSchedulerTest, StreamFrameSchedulerStreamNotExists) {
   EXPECT_EQ(builder.remainingSpaceInPkt(), originalSpace);
 }
 
+TEST_F(QuicPacketSchedulerTest, NoCloningForDSR) {
+  QuicClientConnectionState conn(
+      FizzClientQuicHandshakeContext::Builder().build());
+  FrameScheduler noopScheduler("frame");
+  ASSERT_FALSE(noopScheduler.hasData());
+  CloningScheduler cloningScheduler(noopScheduler, conn, "Juice WRLD", 0);
+  EXPECT_FALSE(cloningScheduler.hasData());
+  addOutstandingPacket(conn);
+  EXPECT_TRUE(cloningScheduler.hasData());
+  conn.outstandings.packets.back().isDSRPacket = true;
+  conn.outstandings.dsrCount++;
+  EXPECT_FALSE(cloningScheduler.hasData());
+  ShortHeader header(
+      ProtectionType::KeyPhaseOne,
+      conn.clientConnectionId.value_or(getTestConnectionId()),
+      getNextPacketNum(conn, PacketNumberSpace::AppData));
+  RegularQuicPacketBuilder builder(
+      conn.udpSendPacketLen,
+      std::move(header),
+      conn.ackStates.appDataAckState.largestAckedByPeer.value_or(0));
+  auto result = cloningScheduler.scheduleFramesForPacket(
+      std::move(builder), kDefaultUDPSendPacketLen);
+  EXPECT_FALSE(result.packetEvent.hasValue());
+  EXPECT_FALSE(result.packet.hasValue());
+}
+
 TEST_F(QuicPacketSchedulerTest, CloningSchedulerTest) {
   QuicClientConnectionState conn(
       FizzClientQuicHandshakeContext::Builder().build());

quic/loss/QuicLossFunctions.h

@@ -262,6 +262,10 @@ folly::Optional<CongestionController::LossEvent> detectLossPackets(
     lossEvent.addLostPacket(pkt);
     observerLossEvent.addLostPacket(lostByTimeout, lostByReorder, pkt);
 
+    if (pkt.isDSRPacket) {
+      CHECK_GT(conn.outstandings.dsrCount, 0);
+      --conn.outstandings.dsrCount;
+    }
     if (pkt.associatedEvent) {
       CHECK(conn.outstandings.clonedPacketCount[pnSpace]);
       --conn.outstandings.clonedPacketCount[pnSpace];

quic/state/AckHandlers.cpp

@@ -48,6 +48,7 @@ void processAckFrame(
   // acks which leads to different number of packets being acked usually.
   ack.ackedPackets.reserve(kDefaultRxPacketsBeforeAckAfterInit);
   auto currentPacketIt = getLastOutstandingPacketIncludingLost(conn, pnSpace);
+  uint64_t dsrPacketsAcked = 0;
   folly::Optional<decltype(conn.lossState.lastAckedPacketSentTime)>
       lastAckedPacketSentTime;
   folly::Optional<Observer::SpuriousLossEvent> spuriousLossEvent;
@@ -139,6 +140,9 @@ void processAckFrame(
         CHECK(conn.outstandings.clonedPacketCount[currentPacketNumberSpace]);
         --conn.outstandings.clonedPacketCount[currentPacketNumberSpace];
       }
+      if (rPacketIt->isDSRPacket) {
+        ++dsrPacketsAcked;
+      }
       // Update RTT if current packet is the largestAcked in the frame:
       auto ackReceiveTimeOrNow = ackReceiveTime > rPacketIt->metadata.time
           ? ackReceiveTime
@@ -229,6 +233,8 @@ void processAckFrame(
   if (lastAckedPacketSentTime) {
     conn.lossState.lastAckedPacketSentTime = *lastAckedPacketSentTime;
   }
+  CHECK_GE(conn.outstandings.dsrCount, dsrPacketsAcked);
+  conn.outstandings.dsrCount -= dsrPacketsAcked;
   CHECK_GE(
       conn.outstandings.packets.size(), conn.outstandings.declaredLostCount);
   auto updatedOustandingPacketsCount = conn.outstandings.numOutstanding();

quic/state/StateData.h

@@ -127,6 +127,10 @@ struct OutstandingsInfo {
   // Number of packets currently declared lost.
   uint64_t declaredLostCount{0};
 
+  // Number of outstanding inflight DSR packet. That is, when a DSR packet is
+  // declared lost, this counter will be decreased.
+  uint64_t dsrCount{0};
+
   // Number of packets outstanding and not declared lost.
   uint64_t numOutstanding() {
     return packets.size() - declaredLostCount;