Do not emit empty PTO packets

Summary: Do not emit empty PTO packets

Reviewed By: mjoras

Differential Revision: D34944343

fbshipit-source-id: a49d82cc3d137ed7f113a8e5adab54c84e0e72d0

quic/QuicConstants.h

@@ -285,6 +285,7 @@ enum class QuicVersion : uint32_t {
   MVFST_EXPERIMENTAL = 0xfaceb00e, // Experimental alias for MVFST
   MVFST_ALIAS = 0xfaceb010,
   MVFST_INVALID = 0xfaceb00f,
+  MVFST_EXPERIMENTAL2 = 0xfaceb011, // Experimental alias for MVFST
 };
 
 using QuicVersionType = std::underlying_type<QuicVersion>::type;

quic/api/QuicPacketScheduler.cpp

@@ -312,11 +312,19 @@ SchedulingResult FrameScheduler::scheduleFramesForPacket(
   return SchedulingResult(folly::none, std::move(builder).buildPacket());
 }
 
+void FrameScheduler::writeNextAcks(PacketBuilderInterface& builder) {
+  ackScheduler_->writeNextAcks(builder);
+}
+
 bool FrameScheduler::hasData() const {
   return (ackScheduler_ && ackScheduler_->hasPendingAcks()) ||
       hasImmediateData();
 }
 
+bool FrameScheduler::hasPendingAcks() const {
+  return ackScheduler_ && ackScheduler_->hasPendingAcks();
+}
+
 bool FrameScheduler::hasImmediateData() const {
   return (cryptoStreamScheduler_ && cryptoStreamScheduler_->hasData()) ||
       (streamFrameScheduler_ && streamFrameScheduler_->hasPendingData()) ||
@@ -794,7 +802,12 @@ SchedulingResult CloningScheduler::scheduleFramesForPacket(
   // The writableBytes in this function shouldn't be limited by cwnd, since
   // we only use CloningScheduler for the cases that we want to bypass cwnd for
   // now.
-  if (frameScheduler_.hasData()) {
+  bool hasData = frameScheduler_.hasData();
+  if (conn_.version.has_value() &&
+      conn_.version.value() == QuicVersion::MVFST_EXPERIMENTAL2) {
+    hasData = frameScheduler_.hasImmediateData();
+  }
+  if (hasData) {
     // Note that there is a possibility that we end up writing nothing here. But
     // if frameScheduler_ hasData() to write, we shouldn't invoke the cloning
     // path if the write fails.
@@ -874,6 +887,14 @@ SchedulingResult CloningScheduler::scheduleFramesForPacket(
     // Rebuilder will write the rest of frames
     auto rebuildResult = rebuilder.rebuildFromPacket(outstandingPacket);
     if (rebuildResult) {
+      if (conn_.version.has_value() &&
+          conn_.version.value() == QuicVersion::MVFST_EXPERIMENTAL2) {
+        // Check if we have any acks pending and write those into the cloned
+        // packet as well.
+        if (frameScheduler_.hasPendingAcks()) {
+          frameScheduler_.writeNextAcks(*internalBuilder);
+        }
+      }
       return SchedulingResult(
           std::move(rebuildResult), std::move(*internalBuilder).buildPacket());
     } else if (

quic/api/QuicPacketScheduler.h

@@ -292,11 +292,17 @@ class FrameScheduler : public QuicPacketScheduler {
   // If any scheduler, including AckScheduler, has pending data to send
   FOLLY_NODISCARD bool hasData() const override;
 
+  // If AckScheduler has any pending acks to write.
+  FOLLY_NODISCARD bool hasPendingAcks() const;
+
   // If any of the non-Ack scheduler has pending data to send
   FOLLY_NODISCARD virtual bool hasImmediateData() const;
 
   FOLLY_NODISCARD folly::StringPiece name() const override;
 
+  // Writes outstanding acks.
+  void writeNextAcks(PacketBuilderInterface& builder);
+
  private:
   folly::Optional<StreamFrameScheduler> streamFrameScheduler_;
   folly::Optional<AckScheduler> ackScheduler_;
@@ -313,11 +319,11 @@ class FrameScheduler : public QuicPacketScheduler {
 
 /**
  * A packet scheduler wrapping a normal FrameScheduler with the ability to clone
- * exiting packets that are still outstanding. A CloningScheduler first trie to
- * write new farmes with new data into a packet. If that fails due to the lack
+ * exiting packets that are still outstanding. A CloningScheduler first tries to
+ * write new frames with new data into a packet. If that fails due to the lack
  * of new data, it falls back to cloning one inflight packet from a connection's
  * oustanding packets if there is at least one outstanding packet that's smaller
- * than the writableBytes limit, and isn't a Handshake packet.
+ * than the writableBytes limit.
  */
 class CloningScheduler : public QuicPacketScheduler {
  public:

quic/api/test/Mocks.h

@@ -35,6 +35,7 @@ class MockFrameScheduler : public FrameScheduler {
   }
 
   MOCK_METHOD((bool), hasData, (), (const));
+  MOCK_METHOD((bool), hasImmediateData, (), (const));
   MOCK_METHOD(
       SchedulingResult,
       _scheduleFramesForPacket,

quic/api/test/QuicPacketSchedulerTest.cpp

@@ -726,6 +726,94 @@ TEST_F(QuicPacketSchedulerTest, CloneSchedulerHasHandshakeData) {
   EXPECT_TRUE(cloningScheduler.hasData());
 }
 
+/**
+ * This test case covers the following scenario:
+   1) conn sent out a handshake packet that did not get acked yet
+   2) conn received some handshake data that needs to be acked
+   3) imitate that we're emitting a PTO packet (that is generated via cloning
+      scheduler)
+   4) emitted cloned packet MUST have both cloned crypto data AND ack
+      frame(s)
+
+    There was a bug that would result in mvfst emit a "empty" PTO packet with
+    acks; this is the test case to cover that scenario.
+ */
+TEST_F(QuicPacketSchedulerTest, CloneSchedulerHasHandshakeDataAndAcks) {
+  QuicServerConnectionState conn(
+      FizzServerQuicHandshakeContext::Builder().build());
+  conn.version = QuicVersion::MVFST_EXPERIMENTAL2;
+
+  FrameScheduler noopScheduler = std::move(FrameScheduler::Builder(
+                                               conn,
+                                               EncryptionLevel::Handshake,
+                                               PacketNumberSpace::Handshake,
+                                               "testScheduler")
+                                               .ackFrames())
+                                     .build();
+  addHandshakeOutstandingPacket(conn);
+
+  // Add some crypto data for the outstanding packet to make it look legit.
+  // This is so cloning scheduler can actually copy something.
+  getCryptoStream(*conn.cryptoState, EncryptionLevel::Handshake)
+      ->retransmissionBuffer.emplace(
+          0,
+          std::make_unique<StreamBuffer>(
+              folly::IOBuf::copyBuffer("test"), 0, false));
+  conn.outstandings.packets.back().packet.frames.push_back(
+      WriteCryptoFrame(0, 4));
+
+  // Make it look like we received some acks from the peer.
+  conn.ackStates.handshakeAckState.acks.insert(10);
+  conn.ackStates.handshakeAckState.largestRecvdPacketTime = Clock::now();
+
+  // Create cloning scheduler.
+  CloningScheduler cloningScheduler(noopScheduler, conn, "CopyCat", 0);
+  EXPECT_TRUE(cloningScheduler.hasData());
+
+  // Get the packet builder going for the clone packet.
+  PacketNum nextPacketNum =
+      getNextPacketNum(conn, PacketNumberSpace::Handshake);
+  std::vector<uint8_t> zeroConnIdData(quic::kDefaultConnectionIdSize, 0);
+  ConnectionId srcConnId(zeroConnIdData);
+  LongHeader header(
+      LongHeader::Types::Handshake,
+      srcConnId,
+      conn.clientConnectionId.value_or(quic::test::getTestConnectionId()),
+      nextPacketNum,
+      QuicVersion::QUIC_DRAFT);
+  RegularQuicPacketBuilder builder(
+      conn.udpSendPacketLen,
+      std::move(header),
+      conn.ackStates.appDataAckState.largestAckedByPeer.value_or(0));
+
+  // Clone the packet.
+  auto result = cloningScheduler.scheduleFramesForPacket(
+      std::move(builder), kDefaultUDPSendPacketLen);
+  EXPECT_TRUE(result.packetEvent.has_value());
+  EXPECT_TRUE(result.packet.has_value());
+
+  // Cloned packet has to have at least one ack frame AND the crypto data.
+  bool hasAckFrame = false;
+  bool hasCryptoFrame = false;
+  for (auto iter = result.packet->packet.frames.cbegin();
+       iter != result.packet->packet.frames.cend();
+       iter++) {
+    const QuicWriteFrame& frame = *iter;
+    switch (frame.type()) {
+      case QuicWriteFrame::Type::WriteAckFrame:
+        hasAckFrame = true;
+        break;
+      case QuicWriteFrame::Type::WriteCryptoFrame:
+        hasCryptoFrame = true;
+        break;
+      default:
+        break;
+    }
+  }
+  EXPECT_TRUE(hasAckFrame);
+  EXPECT_TRUE(hasCryptoFrame);
+}
+
 TEST_F(QuicPacketSchedulerTest, CloneSchedulerHasInitialData) {
   QuicClientConnectionState conn(
       FizzClientQuicHandshakeContext::Builder().build());
@@ -779,13 +867,16 @@ TEST_F(QuicPacketSchedulerTest, DoNotCloneHandshake) {
 TEST_F(QuicPacketSchedulerTest, CloneSchedulerUseNormalSchedulerFirst) {
   QuicClientConnectionState conn(
       FizzClientQuicHandshakeContext::Builder().build());
+  conn.version = QuicVersion::MVFST_EXPERIMENTAL2;
   NiceMock<MockFrameScheduler> mockScheduler(&conn);
   CloningScheduler cloningScheduler(mockScheduler, conn, "Mocker", 0);
   ShortHeader header(
       ProtectionType::KeyPhaseOne,
       conn.clientConnectionId.value_or(getTestConnectionId()),
       getNextPacketNum(conn, PacketNumberSpace::AppData));
-  EXPECT_CALL(mockScheduler, hasData()).Times(1).WillOnce(Return(true));
+  EXPECT_CALL(mockScheduler, hasImmediateData())
+      .Times(1)
+      .WillOnce(Return(true));
 
   EXPECT_CALL(mockScheduler, _scheduleFramesForPacket(_, _))
       .Times(1)

quic/codec/Types.cpp

@@ -451,6 +451,8 @@ std::string toString(QuicVersion version) {
       return "MVFST_ALIAS";
     case QuicVersion::MVFST_INVALID:
       return "MVFST_INVALID";
+    case QuicVersion::MVFST_EXPERIMENTAL2:
+      return "MVFST_EXPERIMENTAL2";
   }
   LOG(WARNING) << "toString has unhandled version type";
   return "UNKNOWN";

quic/server/QuicServer.h

@@ -398,6 +398,7 @@ class QuicServer : public QuicServerWorker::WorkerCallback,
   std::vector<QuicVersion> supportedVersions_{
       {QuicVersion::MVFST,
        QuicVersion::MVFST_EXPERIMENTAL,
+       QuicVersion::MVFST_EXPERIMENTAL2,
        QuicVersion::MVFST_ALIAS,
        QuicVersion::QUIC_V1,
        QuicVersion::QUIC_DRAFT}};

quic/server/state/ServerStateMachine.h

@@ -157,6 +157,7 @@ struct QuicServerConnectionState : public QuicConnectionStateBase {
     supportedVersions = std::vector<QuicVersion>{
         {QuicVersion::MVFST,
          QuicVersion::MVFST_EXPERIMENTAL,
+         QuicVersion::MVFST_EXPERIMENTAL2,
          QuicVersion::MVFST_ALIAS,
          QuicVersion::QUIC_V1,
          QuicVersion::QUIC_DRAFT}};