Fallback to write a Ping frame if Quic Probe doesn't write anything

Summary: as title

Reviewed By: mjoras

Differential Revision: D21464423

fbshipit-source-id: 48cec75f6c8ff0be1e41a5d5a6da0fabf064973a

quic/api/QuicTransportFunctions.cpp

@@ -1120,6 +1120,30 @@ uint64_t writeProbingDataToSocket(
       aead,
       headerCipher,
       version);
+  if (written < probesToSend) {
+    // Fall back to send a ping:
+    sendSimpleFrame(connection, PingFrame());
+    auto pingScheduler = std::move(FrameScheduler::Builder(
+                                       connection,
+                                       EncryptionLevel::AppData,
+                                       PacketNumberSpace::AppData,
+                                       "PingScheduler")
+                                       .simpleFrames())
+                             .build();
+    written += writeConnectionDataToSocket(
+        sock,
+        connection,
+        srcConnId,
+        dstConnId,
+        builder,
+        pnSpace,
+        pingScheduler,
+        unlimitedWritableBytes,
+        probesToSend - written,
+        aead,
+        headerCipher,
+        version);
+  }
   VLOG_IF(10, written > 0)
       << nodeToString(connection.nodeType)
       << " writing probes using scheduler=CloningScheduler " << connection;

quic/api/test/QuicTransportFunctionsTest.cpp

@@ -1572,11 +1572,8 @@ TEST_F(QuicTransportFunctionsTest, WriteProbingCryptoData) {
   EXPECT_FALSE(cryptoStream->retransmissionBuffer.empty());
 }
 
-TEST_F(QuicTransportFunctionsTest, WriteProbesNoNewDataNoCryptoDataNoOldData) {
+TEST_F(QuicTransportFunctionsTest, ProbingFallbackToPing) {
   auto conn = createConn();
-  // writeProbingDataToSocketForTest uses ShortHeader, thus it writes at
-  // AppTraffic level
-  auto currentPacketSeqNum = conn->ackStates.appDataAckState.nextPacketNum;
   auto mockCongestionController =
       std::make_unique<NiceMock<MockCongestionController>>();
   auto rawCongestionController = mockCongestionController.get();
@@ -1585,16 +1582,16 @@ TEST_F(QuicTransportFunctionsTest, WriteProbesNoNewDataNoCryptoDataNoOldData) {
   auto socket =
       std::make_unique<NiceMock<folly::test::MockAsyncUDPSocket>>(&evb);
   auto rawSocket = socket.get();
-  auto stream1 = conn->streamManager->createNextBidirectionalStream().value();
+  EXPECT_CALL(*rawCongestionController, onPacketSent(_)).Times(1);
-  auto buf = buildRandomInputData(0);
+  EXPECT_CALL(*rawSocket, write(_, _))
-  writeDataToQuicStream(*stream1, buf->clone(), false /* eof */);
+      .Times(1)
-
+      .WillOnce(Invoke([&](const SocketAddress&,
-  auto currentStreamWriteOffset = stream1->currentWriteOffset;
+                           const std::unique_ptr<folly::IOBuf>& iobuf) {
-  EXPECT_CALL(*rawCongestionController, onPacketSent(_)).Times(0);
+        return iobuf->computeChainDataLength();
-  EXPECT_CALL(*rawSocket, write(_, _)).Times(0);
+      }));
   uint8_t probesToSend = 1;
   EXPECT_EQ(
-      0,
+      1,
       writeProbingDataToSocketForTest(
           *rawSocket,
           *conn,
@@ -1602,70 +1599,14 @@ TEST_F(QuicTransportFunctionsTest, WriteProbesNoNewDataNoCryptoDataNoOldData) {
           *aead,
           *headerCipher,
           getVersion(*conn)));
-  EXPECT_EQ(1, probesToSend);
+  EXPECT_EQ(1, conn->outstandingPackets.size());
+  EXPECT_EQ(1, conn->outstandingPackets[0].packet.frames.size());
   EXPECT_EQ(
-      currentPacketSeqNum + 1, conn->ackStates.appDataAckState.nextPacketNum);
+      QuicWriteFrame::Type::QuicSimpleFrame_E,
-  EXPECT_TRUE(conn->outstandingPackets.empty());
+      conn->outstandingPackets[0].packet.frames[0].type());
-  EXPECT_FALSE(conn->pendingEvents.setLossDetectionAlarm);
-  EXPECT_EQ(stream1->currentWriteOffset, currentStreamWriteOffset);
-  EXPECT_TRUE(stream1->retransmissionBuffer.empty());
-}
-
-TEST_F(QuicTransportFunctionsTest, ProbingNotWriteOtherFrames) {
-  auto conn = createConn();
-  // writeProbingDataToSocketForTest uses ShortHeader, thus it writes at
-  // AppTraffic level
-  auto currentPacketSeqNum = conn->ackStates.appDataAckState.nextPacketNum;
-  auto mockCongestionController =
-      std::make_unique<NiceMock<MockCongestionController>>();
-  auto rawCongestionController = mockCongestionController.get();
-  conn->congestionController = std::move(mockCongestionController);
-  EventBase evb;
-  auto socket =
-      std::make_unique<NiceMock<folly::test::MockAsyncUDPSocket>>(&evb);
-  auto rawSocket = socket.get();
-  auto stream1 = conn->streamManager->createNextBidirectionalStream().value();
-  RstStreamFrame rstFrame(stream1->id, GenericApplicationErrorCode::UNKNOWN, 0);
-  conn->pendingEvents.resets.emplace(stream1->id, rstFrame);
-  conn->pendingEvents.connWindowUpdate = true;
-  conn->streamManager->queueWindowUpdate(stream1->id);
-
-  auto currentStreamWriteOffset = stream1->currentWriteOffset;
-  auto currentStreamWindow = stream1->flowControlState.advertisedMaxOffset;
-  EXPECT_CALL(*rawCongestionController, onPacketSent(_)).Times(0);
-  EXPECT_CALL(*rawSocket, write(_, _)).Times(0);
-  uint8_t probesToSend = 1;
   EXPECT_EQ(
-      0,
+      QuicSimpleFrame::Type::PingFrame_E,
-      writeProbingDataToSocketForTest(
+      conn->outstandingPackets[0].packet.frames[0].asQuicSimpleFrame()->type());
-          *rawSocket,
-          *conn,
-          probesToSend,
-          *aead,
-          *headerCipher,
-          getVersion(*conn)));
-  EXPECT_EQ(1, probesToSend);
-  EXPECT_EQ(
-      currentPacketSeqNum + 1, conn->ackStates.appDataAckState.nextPacketNum);
-  EXPECT_TRUE(conn->outstandingPackets.empty());
-  EXPECT_FALSE(conn->pendingEvents.setLossDetectionAlarm);
-  EXPECT_EQ(stream1->currentWriteOffset, currentStreamWriteOffset);
-  EXPECT_TRUE(stream1->retransmissionBuffer.empty());
-  // No Ack scheduled:
-  EXPECT_FALSE(conn->ackStates.initialAckState.largestAckScheduled.has_value());
-  EXPECT_FALSE(
-      conn->ackStates.handshakeAckState.largestAckScheduled.has_value());
-  EXPECT_FALSE(conn->ackStates.appDataAckState.largestAckScheduled.has_value());
-  // Pending resets are still here:
-  EXPECT_NE(
-      conn->pendingEvents.resets.end(),
-      conn->pendingEvents.resets.find(stream1->id));
-  // Pending connWindowUpdate is still here:
-  EXPECT_TRUE(conn->pendingEvents.connWindowUpdate);
-  // Stream window update ain't changed:
-  EXPECT_EQ(currentStreamWindow, stream1->flowControlState.advertisedMaxOffset);
-  // Pending streamWindowUpdates are still here:
-  EXPECT_TRUE(conn->streamManager->pendingWindowUpdate(stream1->id));
 }
 
 TEST_F(QuicTransportFunctionsTest, TestCryptoWritingIsHandshakeInOutstanding) {