Give caller some control over if writeStreamFrameHeader should write length

Summary:
Becuase when we clone an existing packet, the logic inside the current
writetStreamFrameHeader is no longer correct.

Reviewed By: mjoras

Differential Revision: D21383828

fbshipit-source-id: 8e6bbb048eefd97ca7cf17b89edc2f395f274a73

quic/api/QuicPacketScheduler.cpp

@@ -227,7 +227,8 @@ void RetransmissionScheduler::writeRetransmissionStreams(
           buffer->offset,
           bufferLen, // writeBufferLen -- only the len of the single buffer.
           bufferLen, // flowControlLen -- not relevant, already flow controlled.
-          buffer->eof);
+          buffer->eof,
+          folly::none /* skipLenHint */);
       if (dataLen) {
         writeStreamFrameData(builder, buffer->data, *dataLen);
         VLOG(4) << "Wrote retransmitted stream=" << stream->id
@@ -335,7 +336,8 @@ bool StreamFrameScheduler::writeNextStreamFrame(
       stream->currentWriteOffset,
       bufferLen,
       flowControlLen,
-      canWriteFin);
+      canWriteFin,
+      folly::none /* skipLenHint */);
   if (!dataLen) {
     return false;
   }

quic/codec/QuicPacketRebuilder.cpp

@@ -52,6 +52,7 @@ folly::Optional<PacketEvent> PacketRebuilder::rebuildFromPacket(
   for (auto iter = packet.packet.frames.cbegin();
        iter != packet.packet.frames.cend();
        iter++) {
+    bool lastFrame = iter == packet.packet.frames.cend() - 1;
     const QuicWriteFrame& frame = *iter;
     switch (frame.type()) {
       case QuicWriteFrame::Type::WriteAckFrame_E: {
@@ -82,7 +83,8 @@ folly::Optional<PacketEvent> PacketRebuilder::rebuildFromPacket(
               streamFrame.offset,
               bufferLen,
               bufferLen,
-              streamFrame.fin);
+              streamFrame.fin,
+              lastFrame && bufferLen);
           bool ret = dataLen.has_value() && *dataLen == streamFrame.len;
           if (ret) {
             // Writing 0 byte for stream data is legit if the stream frame has

quic/codec/QuicWriteCodec.cpp

@@ -35,7 +35,8 @@ folly::Optional<uint64_t> writeStreamFrameHeader(
     uint64_t offset,
     uint64_t writeBufferLen,
     uint64_t flowControlLen,
-    bool fin) {
+    bool fin,
+    folly::Optional<bool> skipLenHint) {
   if (builder.remainingSpaceInPkt() == 0) {
     return folly::none;
   }
@@ -70,10 +71,18 @@ folly::Optional<uint64_t> writeStreamFrameHeader(
   // a zero length fin-only stream frame and omitting the length field.
   uint64_t dataLen = std::min(writeBufferLen, flowControlLen);
   uint64_t dataLenLen = 0;
-  if (dataLen > 0 && dataLen >= builder.remainingSpaceInPkt() - headerSize) {
+  bool shouldSkipLengthField;
-    // We can fill this entire packet with the rest of this stream frame.
+  if (skipLenHint) {
-    dataLen = builder.remainingSpaceInPkt() - headerSize;
+    shouldSkipLengthField = *skipLenHint;
   } else {
+    // Check if we can fill the entire packet with the rest of this stream frame
+    shouldSkipLengthField =
+        dataLen > 0 && dataLen >= builder.remainingSpaceInPkt() - headerSize;
+  }
+  // At most we can write the minimal between data length and what the packet
+  // builder allows us to write.
+  dataLen = std::min(dataLen, builder.remainingSpaceInPkt() - headerSize);
+  if (!shouldSkipLengthField) {
     if (dataLen <= kOneByteLimit - 1) {
       dataLenLen = 1;
     } else if (dataLen <= kTwoByteLimit - 2) {

quic/codec/QuicWriteCodec.h

@@ -65,6 +65,11 @@ size_t writeFrame(QuicWriteFrame&& frame, PacketBuilderInterface& builder);
  * the bytes of data that can be written following the header. The number of
  * bytes are communicated by an optional that can be >= 0. It is expected that
  * the call is followed by writeStreamFrameData.
+ *
+ * skipLenHint: When this value is present, caller will decide if the stream
+ *   length field should be skipped. Otherwise, the function has its own logic
+ *   to decide it. When skipLenHint is true, the field is skipped. When it's
+ *   false, it will be encoded into the header.
  */
 folly::Optional<uint64_t> writeStreamFrameHeader(
     PacketBuilderInterface& builder,
@@ -72,7 +77,8 @@ folly::Optional<uint64_t> writeStreamFrameHeader(
     uint64_t offset,
     uint64_t writeBufferLen,
     uint64_t flowControlLen,
-    bool fin);
+    bool fin,
+    folly::Optional<bool> skipLenHint);
 
 /**
  * Write stream frama data into builder

quic/codec/test/QuicPacketRebuilderTest.cpp

@@ -10,6 +10,7 @@
 
 #include <quic/codec/QuicPacketBuilder.h>
 #include <quic/codec/QuicPacketRebuilder.h>
+#include <quic/codec/test/Mocks.h>
 #include <quic/common/test/TestUtils.h>
 #include <quic/server/state/ServerStateMachine.h>
 #include <quic/state/QuicStateFunctions.h>
@@ -86,7 +87,8 @@ TEST_F(QuicPacketRebuilderTest, RebuildPacket) {
       0,
       buf->computeChainDataLength(),
       buf->computeChainDataLength(),
-      true);
+      true,
+      folly::none /* skipLenHint */);
   writeStreamFrameData(
       regularBuilder1, buf->clone(), buf->computeChainDataLength());
   writeFrame(maxDataFrame, regularBuilder1);
@@ -212,7 +214,8 @@ TEST_F(QuicPacketRebuilderTest, RebuildAfterResetStream) {
       0,
       buf->computeChainDataLength(),
       buf->computeChainDataLength(),
-      true);
+      true,
+      folly::none /* skipLenHint */);
   writeStreamFrameData(
       regularBuilder1, buf->clone(), buf->computeChainDataLength());
   auto packet1 = std::move(regularBuilder1).buildPacket();
@@ -242,7 +245,8 @@ TEST_F(QuicPacketRebuilderTest, FinOnlyStreamRebuild) {
   auto streamId = stream->id;
 
   // Write them with a regular builder
-  writeStreamFrameHeader(regularBuilder1, streamId, 0, 0, 0, true);
+  writeStreamFrameHeader(
+      regularBuilder1, streamId, 0, 0, 0, true, folly::none /* skipLenHint */);
   auto packet1 = std::move(regularBuilder1).buildPacket();
   stream->retransmissionBuffer.emplace(
       std::piecewise_construct,
@@ -295,7 +299,8 @@ TEST_F(QuicPacketRebuilderTest, RebuildDataStreamAndEmptyCryptoStream) {
       0,
       buf->computeChainDataLength(),
       buf->computeChainDataLength(),
-      true);
+      true,
+      folly::none /* skipLenHint */);
   writeStreamFrameData(
       regularBuilder1, buf->clone(), buf->computeChainDataLength());
   writeCryptoFrame(cryptoOffset, cryptoBuf->clone(), regularBuilder1);
@@ -397,7 +402,8 @@ TEST_F(QuicPacketRebuilderTest, CannotRebuild) {
       0,
       buf->computeChainDataLength(),
       buf->computeChainDataLength(),
-      true);
+      true,
+      folly::none /* skipLenHint */);
   writeStreamFrameData(
       regularBuilder1, buf->clone(), buf->computeChainDataLength());
   auto packet1 = std::move(regularBuilder1).buildPacket();
@@ -445,5 +451,150 @@ TEST_F(QuicPacketRebuilderTest, CloneCounter) {
   EXPECT_EQ(1, conn.outstandingClonedPacketsCount);
 }
 
+TEST_F(QuicPacketRebuilderTest, LastStreamFrameSkipLen) {
+  QuicServerConnectionState conn;
+  conn.streamManager->setMaxLocalBidirectionalStreams(100);
+  auto stream = conn.streamManager->createNextBidirectionalStream().value();
+  auto streamId = stream->id;
+  auto buf1 =
+      folly::IOBuf::copyBuffer("Remember your days are fully numbered.");
+  auto buf2 = folly::IOBuf::copyBuffer("Just march on");
+
+  ShortHeader shortHeader(
+      ProtectionType::KeyPhaseZero, getTestConnectionId(), 0);
+  RegularQuicPacketBuilder regularBuilder(
+      kDefaultUDPSendPacketLen, std::move(shortHeader), 0);
+  regularBuilder.encodePacketHeader();
+  writeStreamFrameHeader(
+      regularBuilder,
+      streamId,
+      0,
+      buf1->computeChainDataLength(),
+      buf1->computeChainDataLength(),
+      false,
+      folly::none);
+  writeStreamFrameData(
+      regularBuilder, buf1->clone(), buf1->computeChainDataLength());
+  writeStreamFrameHeader(
+      regularBuilder,
+      streamId,
+      buf1->computeChainDataLength(),
+      buf2->computeChainDataLength(),
+      buf2->computeChainDataLength(),
+      true,
+      folly::none);
+  writeStreamFrameData(
+      regularBuilder, buf2->clone(), buf2->computeChainDataLength());
+  auto packet = std::move(regularBuilder).buildPacket();
+  auto outstandingPacket = makeDummyOutstandingPacket(packet.packet, 1200);
+  stream->retransmissionBuffer.emplace(
+      std::piecewise_construct,
+      std::forward_as_tuple(0),
+      std::forward_as_tuple(
+          std::make_unique<StreamBuffer>(buf1->clone(), 0, false)));
+  stream->retransmissionBuffer.emplace(
+      std::piecewise_construct,
+      std::forward_as_tuple(buf1->computeChainDataLength()),
+      std::forward_as_tuple(std::make_unique<StreamBuffer>(
+          buf2->clone(), buf1->computeChainDataLength(), true)));
+
+  MockQuicPacketBuilder mockBuilder;
+  size_t packetLimit = 1200;
+  EXPECT_CALL(mockBuilder, remainingSpaceInPkt()).WillRepeatedly(Invoke([&]() {
+    return packetLimit;
+  }));
+  // write data twice
+  EXPECT_CALL(mockBuilder, insert(_, _))
+      .Times(2)
+      .WillRepeatedly(
+          Invoke([&](const BufQueue&, size_t limit) { packetLimit -= limit; }));
+  // Append frame twice
+  EXPECT_CALL(mockBuilder, appendFrame(_)).Times(2);
+  // initial byte:
+  EXPECT_CALL(mockBuilder, writeBEUint8(_))
+      .Times(2)
+      .WillRepeatedly(Invoke([&](uint8_t) { packetLimit--; }));
+  // Write streamId twice, offset once, then data len only once:
+  EXPECT_CALL(mockBuilder, write(_))
+      .Times(4)
+      .WillRepeatedly(Invoke([&](const QuicInteger& quicInt) {
+        packetLimit -= quicInt.getSize();
+      }));
+
+  PacketRebuilder rebuilder(mockBuilder, conn);
+  EXPECT_TRUE(rebuilder.rebuildFromPacket(outstandingPacket).has_value());
+}
+
+TEST_F(QuicPacketRebuilderTest, LastStreamFrameFinOnlyNotSkipLen) {
+  QuicServerConnectionState conn;
+  conn.streamManager->setMaxLocalBidirectionalStreams(100);
+  auto stream = conn.streamManager->createNextBidirectionalStream().value();
+  auto streamId = stream->id;
+  auto buf1 =
+      folly::IOBuf::copyBuffer("Remember your days are fully numbered.");
+
+  ShortHeader shortHeader(
+      ProtectionType::KeyPhaseZero, getTestConnectionId(), 0);
+  RegularQuicPacketBuilder regularBuilder(
+      kDefaultUDPSendPacketLen, std::move(shortHeader), 0);
+  regularBuilder.encodePacketHeader();
+  writeStreamFrameHeader(
+      regularBuilder,
+      streamId,
+      0,
+      buf1->computeChainDataLength(),
+      buf1->computeChainDataLength(),
+      false,
+      folly::none);
+  writeStreamFrameData(
+      regularBuilder, buf1->clone(), buf1->computeChainDataLength());
+  writeStreamFrameHeader(
+      regularBuilder,
+      streamId,
+      buf1->computeChainDataLength(),
+      0,
+      0,
+      true,
+      folly::none);
+  writeStreamFrameData(regularBuilder, nullptr, 0);
+  auto packet = std::move(regularBuilder).buildPacket();
+  auto outstandingPacket = makeDummyOutstandingPacket(packet.packet, 1200);
+  stream->retransmissionBuffer.emplace(
+      std::piecewise_construct,
+      std::forward_as_tuple(0),
+      std::forward_as_tuple(
+          std::make_unique<StreamBuffer>(buf1->clone(), 0, false)));
+  stream->retransmissionBuffer.emplace(
+      std::piecewise_construct,
+      std::forward_as_tuple(buf1->computeChainDataLength()),
+      std::forward_as_tuple(std::make_unique<StreamBuffer>(
+          nullptr, buf1->computeChainDataLength(), true)));
+
+  MockQuicPacketBuilder mockBuilder;
+  size_t packetLimit = 1200;
+  EXPECT_CALL(mockBuilder, remainingSpaceInPkt()).WillRepeatedly(Invoke([&]() {
+    return packetLimit;
+  }));
+  // write data only
+  EXPECT_CALL(mockBuilder, insert(_, _))
+      .Times(1)
+      .WillOnce(
+          Invoke([&](const BufQueue&, size_t limit) { packetLimit -= limit; }));
+  // Append frame twice
+  EXPECT_CALL(mockBuilder, appendFrame(_)).Times(2);
+  // initial byte:
+  EXPECT_CALL(mockBuilder, writeBEUint8(_))
+      .Times(2)
+      .WillRepeatedly(Invoke([&](uint8_t) { packetLimit--; }));
+  // Write streamId twice, offset once, then data len twice:
+  EXPECT_CALL(mockBuilder, write(_))
+      .Times(5)
+      .WillRepeatedly(Invoke([&](const QuicInteger& quicInt) {
+        packetLimit -= quicInt.getSize();
+      }));
+
+  PacketRebuilder rebuilder(mockBuilder, conn);
+  EXPECT_TRUE(rebuilder.rebuildFromPacket(outstandingPacket).has_value());
+}
 } // namespace test
 } // namespace quic

quic/codec/test/QuicWriteCodecTest.cpp

@@ -133,8 +133,8 @@ TEST_F(QuicWriteCodecTest, WriteStreamFrameToEmptyPacket) {
   StreamId streamId = 1;
   uint64_t offset = 0;
   bool fin = false;
-  auto dataLen =
+  auto dataLen = writeStreamFrameHeader(
-      writeStreamFrameHeader(pktBuilder, streamId, offset, 10, 10, fin);
+      pktBuilder, streamId, offset, 10, 10, fin, folly::none /* skipLenHint */);
   ASSERT_TRUE(dataLen);
   ASSERT_EQ(*dataLen, 10);
   writeStreamFrameData(pktBuilder, inputBuf->clone(), 10);
@@ -179,8 +179,8 @@ TEST_F(QuicWriteCodecTest, WriteStreamFrameToPartialPacket) {
   // 4 bytes offset
   // 1 byte for length
   // => 8 bytes of header
-  auto dataLen =
+  auto dataLen = writeStreamFrameHeader(
-      writeStreamFrameHeader(pktBuilder, streamId, offset, 20, 20, fin);
+      pktBuilder, streamId, offset, 20, 20, fin, folly::none /* skipLenHint */);
   ASSERT_TRUE(dataLen);
   EXPECT_EQ(*dataLen, 20);
   writeStreamFrameData(pktBuilder, inputBuf->clone(), 20);
@@ -228,8 +228,14 @@ TEST_F(QuicWriteCodecTest, WriteTwoStreamFrames) {
   uint64_t offset1 = 65535;
   bool fin1 = false;
   auto inputBuf = buildRandomInputData(30);
-  auto dataLen =
+  auto dataLen = writeStreamFrameHeader(
-      writeStreamFrameHeader(pktBuilder, streamId1, offset1, 30, 30, fin1);
+      pktBuilder,
+      streamId1,
+      offset1,
+      30,
+      30,
+      fin1,
+      folly::none /* skipLenHint */);
   ASSERT_TRUE(dataLen);
   ASSERT_EQ(*dataLen, 30);
   writeStreamFrameData(pktBuilder, inputBuf->clone(), 30);
@@ -250,7 +256,13 @@ TEST_F(QuicWriteCodecTest, WriteTwoStreamFrames) {
   // 4 bytes for offset
   // => 7 bytes
   dataLen = writeStreamFrameHeader(
-      pktBuilder, streamId2, offset2, remainingSpace, remainingSpace, fin2);
+      pktBuilder,
+      streamId2,
+      offset2,
+      remainingSpace,
+      remainingSpace,
+      fin2,
+      folly::none /* skipLenHint */);
   ASSERT_TRUE(dataLen);
   ASSERT_EQ(*dataLen, remainingSpace - 7);
   writeStreamFrameData(pktBuilder, inputBuf2->clone(), remainingSpace - 7);
@@ -319,8 +331,8 @@ TEST_F(QuicWriteCodecTest, WriteStreamFramePartialData) {
   // 2 bytes for stream id
   // 4 bytes for offset
   // => 7 bytes for header
-  auto dataLen =
+  auto dataLen = writeStreamFrameHeader(
-      writeStreamFrameHeader(pktBuilder, streamId, offset, 50, 50, fin);
+      pktBuilder, streamId, offset, 50, 50, fin, folly::none /* skipLenHint */);
   ASSERT_TRUE(dataLen);
   ASSERT_EQ(*dataLen, 33);
   writeStreamFrameData(pktBuilder, inputBuf->clone(), 33);
@@ -358,8 +370,8 @@ TEST_F(QuicWriteCodecTest, WriteStreamFrameTooSmallForStreamHeader) {
   StreamId streamId = 1;
   uint64_t offset = 65535;
   bool fin = false;
-  auto dataLen =
+  auto dataLen = writeStreamFrameHeader(
-      writeStreamFrameHeader(pktBuilder, streamId, offset, 1, 1, fin);
+      pktBuilder, streamId, offset, 1, 1, fin, folly::none /* skipLenHint */);
   EXPECT_FALSE(dataLen);
   EXPECT_EQ(1, pktBuilder.remainingSpaceInPkt());
 }
@@ -376,8 +388,8 @@ TEST_F(QuicWriteCodecTest, WriteStreamNoSpaceForData) {
   // 1 byte for stream id
   // 1 byte for offset
   // => 3 bytes
-  auto dataLen =
+  auto dataLen = writeStreamFrameHeader(
-      writeStreamFrameHeader(pktBuilder, streamId, offset, 10, 10, fin);
+      pktBuilder, streamId, offset, 10, 10, fin, folly::none /* skipLenHint */);
   EXPECT_FALSE(dataLen.has_value());
   EXPECT_EQ(pktBuilder.remainingSpaceInPkt(), 3);
 }
@@ -398,8 +410,14 @@ TEST_F(QuicWriteCodecTest, WriteStreamSpaceForOneByte) {
   // 1 byte for stream id
   // 1 byte for offet
   // => 3 bytes
-  auto dataLen =
+  auto dataLen = writeStreamFrameHeader(
-      writeStreamFrameHeader(pktBuilder, streamId, offset, 100, 100, fin);
+      pktBuilder,
+      streamId,
+      offset,
+      100,
+      100,
+      fin,
+      folly::none /* skipLenHint */);
   ASSERT_TRUE(dataLen);
   ASSERT_EQ(*dataLen, 1);
   writeStreamFrameData(pktBuilder, inputBuf->clone(), 1);
@@ -444,8 +462,8 @@ TEST_F(QuicWriteCodecTest, WriteFinToEmptyPacket) {
   StreamId streamId = 1;
   uint64_t offset = 0;
   bool fin = true;
-  auto dataLen =
+  auto dataLen = writeStreamFrameHeader(
-      writeStreamFrameHeader(pktBuilder, streamId, offset, 10, 10, fin);
+      pktBuilder, streamId, offset, 10, 10, fin, folly::none /* skipLenHint */);
   ASSERT_TRUE(dataLen);
   ASSERT_EQ(*dataLen, 10);
   writeStreamFrameData(pktBuilder, inputBuf->clone(), 10);
@@ -496,7 +514,13 @@ TEST_F(QuicWriteCodecTest, TestWriteIncompleteDataAndFin) {
   uint64_t offset = 0;
   bool fin = true;
   auto dataLen = writeStreamFrameHeader(
-      pktBuilder, streamId, offset, inDataSize, inDataSize, fin);
+      pktBuilder,
+      streamId,
+      offset,
+      inDataSize,
+      inDataSize,
+      fin,
+      folly::none /* skipLenHint */);
   ASSERT_TRUE(dataLen);
   EXPECT_LT(*dataLen, inDataSize);
 }
@@ -512,8 +536,8 @@ TEST_F(QuicWriteCodecTest, TestWriteNoDataAndFin) {
   uint64_t offset = 0;
   bool fin = true;
   Buf empty;
-  auto dataLen =
+  auto dataLen = writeStreamFrameHeader(
-      writeStreamFrameHeader(pktBuilder, streamId, offset, 0, 0, fin);
+      pktBuilder, streamId, offset, 0, 0, fin, folly::none /* skipLenHint */);
   ASSERT_TRUE(dataLen);
   EXPECT_EQ(*dataLen, 0);
 }
@@ -526,7 +550,14 @@ TEST_F(QuicWriteCodecTest, TestWriteNoDataAndNoFin) {
   bool fin = false;
   Buf empty;
   EXPECT_THROW(
-      writeStreamFrameHeader(pktBuilder, streamId, offset, 0, 0, fin),
+      writeStreamFrameHeader(
+          pktBuilder,
+          streamId,
+          offset,
+          0,
+          0,
+          fin,
+          folly::none /* skipLenHint */),
       QuicInternalException);
 }
 
@@ -543,8 +574,8 @@ TEST_F(QuicWriteCodecTest, PacketOnlyHasSpaceForStreamHeader) {
   StreamId streamId = 1;
   uint64_t offset = 0;
   bool fin = true;
-  auto dataLen =
+  auto dataLen = writeStreamFrameHeader(
-      writeStreamFrameHeader(pktBuilder, streamId, offset, 20, 20, fin);
+      pktBuilder, streamId, offset, 20, 20, fin, folly::none /* skipLenHint */);
   EXPECT_FALSE(dataLen.has_value());
   EXPECT_EQ(pktBuilder.remainingSpaceInPkt(), 2);
 }
@@ -560,8 +591,8 @@ TEST_F(QuicWriteCodecTest, PacketOnlyHasSpaceForStreamHeaderWithFin) {
   StreamId streamId = 1;
   uint64_t offset = 0;
   bool fin = true;
-  auto dataLen =
+  auto dataLen = writeStreamFrameHeader(
-      writeStreamFrameHeader(pktBuilder, streamId, offset, 0, 0, fin);
+      pktBuilder, streamId, offset, 0, 0, fin, folly::none /* skipLenHint */);
   ASSERT_TRUE(dataLen.has_value());
   EXPECT_EQ(*dataLen, 0);
   EXPECT_EQ(pktBuilder.remainingSpaceInPkt(), 0);
@@ -578,12 +609,112 @@ TEST_F(QuicWriteCodecTest, PacketNotEnoughSpaceForStreamHeaderWithFin) {
   StreamId streamId = 1;
   uint64_t offset = 0;
   bool fin = true;
-  auto dataLen =
+  auto dataLen = writeStreamFrameHeader(
-      writeStreamFrameHeader(pktBuilder, streamId, offset, 0, 0, fin);
+      pktBuilder, streamId, offset, 0, 0, fin, folly::none /* skipLenHint */);
   ASSERT_FALSE(dataLen.has_value());
   EXPECT_EQ(pktBuilder.remainingSpaceInPkt(), 2);
 }
 
+TEST_F(QuicWriteCodecTest, WriteStreamFrameHeadeSkipLen) {
+  MockQuicPacketBuilder pktBuilder;
+  size_t packetLimit = 1200;
+  EXPECT_CALL(pktBuilder, appendFrame(_)).Times(1);
+  EXPECT_CALL(pktBuilder, remainingSpaceInPkt()).WillRepeatedly(Invoke([&]() {
+    return packetLimit;
+  }));
+  // initial byte:
+  EXPECT_CALL(pktBuilder, writeBEUint8(_)).WillOnce(Invoke([&](uint8_t) {
+    packetLimit--;
+  }));
+  // write twice: stream id and offste
+  EXPECT_CALL(pktBuilder, write(_))
+      .Times(2)
+      .WillRepeatedly(Invoke([&](const QuicInteger& quicInt) {
+        packetLimit -= quicInt.getSize();
+      }));
+  StreamId streamId = 0;
+  uint64_t offset = 10;
+  bool fin = false;
+  auto dataLen = writeStreamFrameHeader(
+      pktBuilder, streamId, offset, 1200 * 2, 1200 * 2, fin, folly::none);
+  EXPECT_LT(*dataLen, 1200);
+}
+
+TEST_F(QuicWriteCodecTest, WriteStreamFrameHeadeNotSkipLen) {
+  MockQuicPacketBuilder pktBuilder;
+  size_t packetLimit = 1200;
+  EXPECT_CALL(pktBuilder, appendFrame(_)).Times(1);
+  EXPECT_CALL(pktBuilder, remainingSpaceInPkt()).WillRepeatedly(Invoke([&]() {
+    return packetLimit;
+  }));
+  // initial byte:
+  EXPECT_CALL(pktBuilder, writeBEUint8(_)).WillOnce(Invoke([&](uint8_t) {
+    packetLimit--;
+  }));
+  // write three times: stream id and offste and data len
+  EXPECT_CALL(pktBuilder, write(_))
+      .Times(3)
+      .WillRepeatedly(Invoke([&](const QuicInteger& quicInt) {
+        packetLimit -= quicInt.getSize();
+      }));
+  StreamId streamId = 0;
+  uint64_t offset = 10;
+  bool fin = false;
+  auto dataLen = writeStreamFrameHeader(
+      pktBuilder, streamId, offset, 200, 200, fin, folly::none);
+  EXPECT_EQ(*dataLen, 200);
+}
+
+TEST_F(QuicWriteCodecTest, WriteStreamFrameHeadeLengthHintTrue) {
+  MockQuicPacketBuilder pktBuilder;
+  size_t packetLimit = 1200;
+  EXPECT_CALL(pktBuilder, appendFrame(_)).Times(1);
+  EXPECT_CALL(pktBuilder, remainingSpaceInPkt()).WillRepeatedly(Invoke([&]() {
+    return packetLimit;
+  }));
+  // initial byte:
+  EXPECT_CALL(pktBuilder, writeBEUint8(_)).WillOnce(Invoke([&](uint8_t) {
+    packetLimit--;
+  }));
+  // write twice: stream id and offste
+  EXPECT_CALL(pktBuilder, write(_))
+      .Times(2)
+      .WillRepeatedly(Invoke([&](const QuicInteger& quicInt) {
+        packetLimit -= quicInt.getSize();
+      }));
+  StreamId streamId = 0;
+  uint64_t offset = 10;
+  bool fin = false;
+  auto dataLen =
+      writeStreamFrameHeader(pktBuilder, streamId, offset, 200, 200, fin, true);
+  EXPECT_EQ(*dataLen, 200);
+}
+
+TEST_F(QuicWriteCodecTest, WriteStreamFrameHeadeLengthHintFalse) {
+  MockQuicPacketBuilder pktBuilder;
+  size_t packetLimit = 1200;
+  EXPECT_CALL(pktBuilder, appendFrame(_)).Times(1);
+  EXPECT_CALL(pktBuilder, remainingSpaceInPkt()).WillRepeatedly(Invoke([&]() {
+    return packetLimit;
+  }));
+  // initial byte:
+  EXPECT_CALL(pktBuilder, writeBEUint8(_)).WillOnce(Invoke([&](uint8_t) {
+    packetLimit--;
+  }));
+  // write three times: stream id and offste and data len
+  EXPECT_CALL(pktBuilder, write(_))
+      .Times(3)
+      .WillRepeatedly(Invoke([&](const QuicInteger& quicInt) {
+        packetLimit -= quicInt.getSize();
+      }));
+  StreamId streamId = 0;
+  uint64_t offset = 10;
+  bool fin = false;
+  auto dataLen = writeStreamFrameHeader(
+      pktBuilder, streamId, offset, 1200 * 2, 1200 * 2, fin, false);
+  EXPECT_LT(*dataLen, 1200);
+}
+
 TEST_F(QuicWriteCodecTest, AckFrameGapExceedsRepresentation) {
   MockQuicPacketBuilder pktBuilder;
   setupCommonExpects(pktBuilder);

quic/common/test/TestUtils.cpp

@@ -337,7 +337,8 @@ RegularQuicPacketBuilder::Packet createStreamPacket(
       offset,
       data.computeChainDataLength(),
       data.computeChainDataLength(),
-      eof);
+      eof,
+      folly::none /* skipLenHint */);
   writeStreamFrameData(*builder, data.clone(), data.computeChainDataLength());
   return std::move(*builder).buildPacket();
 }

quic/fizz/client/test/QuicClientTransportTest.cpp

@@ -4117,7 +4117,8 @@ TEST_F(
       0,
       data->computeChainDataLength(),
       data->computeChainDataLength(),
-      false);
+      false,
+      folly::none /* skipLenHint */);
   writeStreamFrameData(builder2, data->clone(), data->computeChainDataLength());
   auto packetObject = std::move(builder2).buildPacket();
   auto packet2 = packetToBuf(std::move(packetObject));

quic/server/test/QuicServerTest.cpp

@@ -912,7 +912,8 @@ auto createInitialStream(
       0,
       streamData->computeChainDataLength(),
       streamData->computeChainDataLength(),
-      true);
+      true,
+      folly::none /* skipLenHint */);
   EXPECT_TRUE(dataLen);
   writeStreamFrameData(builder, std::move(streamData), *dataLen);
   return packetToBuf(std::move(builder).buildPacket());
@@ -1361,9 +1362,8 @@ class QuicServerTest : public Test {
     if (stats) {
       CHECK_EQ(evbs.size(), 1);
       EXPECT_CALL(*transportStatsFactory_, make())
-          .WillRepeatedly(Invoke([stats]() {
+          .WillRepeatedly(Invoke(
-            return std::unique_ptr<MockQuicStats>(stats);
+              [stats]() { return std::unique_ptr<MockQuicStats>(stats); }));
-          }));
     } else {
       EXPECT_CALL(*transportStatsFactory_, make()).WillRepeatedly(Invoke([&]() {
         auto mockInfoCb = std::make_unique<NiceMock<MockQuicStats>>();

quic/server/test/QuicServerTransportTest.cpp

@@ -696,7 +696,8 @@ TEST_F(QuicServerTransportTest, TestReadMultipleStreams) {
       0,
       buf1->computeChainDataLength(),
       buf1->computeChainDataLength(),
-      true);
+      true,
+      folly::none /* skipLenHint */);
   ASSERT_TRUE(dataLen);
   ASSERT_EQ(*dataLen, buf1->computeChainDataLength());
   writeStreamFrameData(builder, buf1->clone(), buf1->computeChainDataLength());
@@ -707,7 +708,8 @@ TEST_F(QuicServerTransportTest, TestReadMultipleStreams) {
       0,
       buf1->computeChainDataLength(),
       buf1->computeChainDataLength(),
-      true);
+      true,
+      folly::none /* skipLenHint */);
   ASSERT_TRUE(dataLen);
   ASSERT_EQ(*dataLen, buf1->computeChainDataLength());
   writeStreamFrameData(builder, buf2->clone(), buf2->computeChainDataLength());
@@ -1105,7 +1107,8 @@ TEST_F(QuicServerTransportTest, NoDataExceptCloseProcessedAfterClosing) {
       0,
       buf->computeChainDataLength(),
       buf->computeChainDataLength(),
-      true);
+      true,
+      folly::none /* skipLenHint */);
   writeStreamFrameData(builder, buf->clone(), buf->computeChainDataLength());
   std::string errMsg = "Mind the gap";
   ConnectionCloseFrame connClose(
@@ -1317,7 +1320,8 @@ TEST_F(QuicServerTransportTest, ReceiveRstStreamNonExistentAndOtherFrame) {
       0,
       data->computeChainDataLength(),
       data->computeChainDataLength(),
-      false);
+      false,
+      folly::none /* skipLenHint */);
   writeStreamFrameData(builder2, data->clone(), data->computeChainDataLength());
   auto packetObject = std::move(builder2).buildPacket();
   auto packet2 = packetToBuf(std::move(packetObject));
@@ -1575,7 +1579,13 @@ TEST_F(QuicServerTransportTest, RecvStopSendingFrameAfterHalfCloseRemote) {
       streamId, GenericApplicationErrorCode::UNKNOWN);
   ASSERT_TRUE(builder.canBuildPacket());
   auto dataLen = writeStreamFrameHeader(
-      builder, 0x00, stream->currentReadOffset, 0, 10, true);
+      builder,
+      0x00,
+      stream->currentReadOffset,
+      0,
+      10,
+      true,
+      folly::none /* skipLenHint */);
   ASSERT_TRUE(dataLen.has_value());
   ASSERT_EQ(*dataLen, 0);
   writeFrame(QuicSimpleFrame(stopSendingFrame), builder);