No more Pending mode in Quic ack writing

Summary:
The AckScheduler right now has two modes: Immediate mode which always
write acks into the current packet, pending mode which only write if
needsToSendAckImmediately is true. The FrameScheduler choose the Immdiate mode
if there are other data to write as well. Otherwise, it chooses the Pending
mode. But if there is no other data to write and needsToSendAckImmediately is
false, the FrameScheduler will end up writing nothing.

This isn't a problem today because to be on the write path, the shouldWriteData
function would make sure we either have non-ack data to write, or
needsToSendAckImmediately is true for a packet number space. But once we allow
packets in Initial and Handshake space to be cloned, we would be on the write
path when there are probe quota. The FrameScheduler's hasData function doesn't
check needsToSendAckImmediately. It will think it has data to write as long as
AckState has changed, but can ends up writing nothing with the Pending ack
mode.

I think given the write looper won't be schedule to loop when there is no
non-ack data to write and needsToSendAckImmediately is true, it's safe to
remove Pending ack mode from AckScheduler.

Reviewed By: mjoras

Differential Revision: D22044741

fbshipit-source-id: 26fcaabdd5c45c1cae12d459ee5924a30936e209

quic/api/QuicPacketScheduler-inl.h

@@ -15,30 +15,6 @@ namespace quic {
 
 template <typename ClockType>
 inline folly::Optional<PacketNum> AckScheduler::writeNextAcks(
-    PacketBuilderInterface& builder,
-    AckMode mode) {
-  switch (mode) {
-    case AckMode::Immediate: {
-      return writeAcksImpl<ClockType>(builder);
-    }
-    case AckMode::Pending: {
-      return writeAcksIfPending<ClockType>(builder);
-    }
-  }
-  folly::assume_unreachable();
-}
-
-template <typename ClockType>
-inline folly::Optional<PacketNum> AckScheduler::writeAcksIfPending(
-    PacketBuilderInterface& builder) {
-  if (ackState_.needsToSendAckImmediately) {
-    return writeAcksImpl<ClockType>(builder);
-  }
-  return folly::none;
-}
-
-template <typename ClockType>
-folly::Optional<PacketNum> AckScheduler::writeAcksImpl(
     PacketBuilderInterface& builder) {
   // Use default ack delay for long headers. Usually long headers are sent
   // before crypto negotiation, so the peer might not know about the ack delay

quic/api/QuicPacketScheduler.cpp

@@ -125,7 +125,6 @@ SchedulingResult FrameScheduler::scheduleFramesForPacket(
   // We cannot return early if the writablyBytes dropps to 0 here, since pure
   // acks can skip writableBytes entirely.
   PacketBuilderWrapper wrapper(builder, writableBytes);
-  auto ackMode = hasImmediateData() ? AckMode::Immediate : AckMode::Pending;
   bool cryptoDataWritten = false;
   bool rstWritten = false;
   if (cryptoStreamScheduler_ && cryptoStreamScheduler_->hasData()) {
@@ -134,18 +133,19 @@ SchedulingResult FrameScheduler::scheduleFramesForPacket(
   if (rstScheduler_ && rstScheduler_->hasPendingRsts()) {
     rstWritten = rstScheduler_->writeRsts(wrapper);
   }
+  // TODO: Long time ago we decided RST has higher priority than Acks. Why tho?
   if (ackScheduler_ && ackScheduler_->hasPendingAcks()) {
     if (cryptoDataWritten || rstWritten) {
       // If packet has non ack data, it is subject to congestion control. We
       // need to use the wrapper/
-      ackScheduler_->writeNextAcks(wrapper, ackMode);
+      ackScheduler_->writeNextAcks(wrapper);
     } else {
       // If we start with writing acks, we will let the ack scheduler write
       // up to the full packet space. If the ack bytes exceeds the writable
       // bytes, this will be a pure ack packet and it will skip congestion
       // controller. Otherwise, we will give other schedulers an opportunity to
       // write up to writable bytes.
-      ackScheduler_->writeNextAcks(builder, ackMode);
+      ackScheduler_->writeNextAcks(builder);
     }
   }
   if (windowUpdateScheduler_ &&

quic/api/QuicPacketScheduler.h

@@ -60,9 +60,6 @@ class QuicPacketScheduler {
   virtual std::string name() const = 0;
 };
 
-// A tag to denote how we should schedule ack in this packet.
-enum class AckMode { Pending, Immediate };
-
 class RetransmissionScheduler {
  public:
   explicit RetransmissionScheduler(const QuicConnectionStateBase& conn);
@@ -198,23 +195,11 @@ class AckScheduler {
 
   template <typename ClockType = Clock>
   folly::Optional<PacketNum> writeNextAcks(
-      PacketBuilderInterface& builder,
+      PacketBuilderInterface& builder);
-      AckMode mode);
 
   bool hasPendingAcks() const;
 
  private:
-  /* Write out pending acks if needsToSendAckImmeidately in the connection's
-   * pendingEvent is true.
-   */
-  template <typename ClockType>
-  folly::Optional<PacketNum> writeAcksIfPending(
-      PacketBuilderInterface& builder);
-
-  // Write out pending acks
-  template <typename ClockType>
-  folly::Optional<PacketNum> writeAcksImpl(PacketBuilderInterface& builder);
-
   const QuicConnectionStateBase& conn_;
   const AckState& ackState_;
 };

quic/api/test/QuicTransportFunctionsTest.cpp

@@ -1856,10 +1856,11 @@ TEST_F(QuicTransportFunctionsTest, HasAckDataToWriteCipherAndAckStateMatch) {
 TEST_F(QuicTransportFunctionsTest, HasAckDataToWriteNoImmediateAcks) {
   auto conn = createConn();
   conn->oneRttWriteCipher = test::createNoOpAead();
-  conn->ackStates.initialAckState.needsToSendAckImmediately = false;
+  conn->ackStates.appDataAckState.acks.insert(0, 100);
-  conn->ackStates.handshakeAckState.needsToSendAckImmediately = false;
   conn->ackStates.appDataAckState.needsToSendAckImmediately = false;
   EXPECT_FALSE(hasAckDataToWrite(*conn));
+  conn->ackStates.appDataAckState.needsToSendAckImmediately = true;
+  EXPECT_TRUE(hasAckDataToWrite(*conn));
 }
 
 TEST_F(QuicTransportFunctionsTest, HasAckDataToWriteNoAcksScheduled) {

quic/api/test/QuicTransportTest.cpp

@@ -800,26 +800,6 @@ TEST_F(QuicTransportTest, WriteImmediateAcks) {
   EXPECT_EQ(WriteDataReason::NO_WRITE, shouldWriteData(conn));
 }
 
-TEST_F(QuicTransportTest, NotWriteAcksIfNoData) {
-  auto& conn = transport_->getConnectionState();
-
-  addAckStatesWithCurrentTimestamps(conn.ackStates.appDataAckState, 0, 100);
-  conn.ackStates.appDataAckState.needsToSendAckImmediately = false;
-  conn.ackStates.appDataAckState.numNonRxPacketsRecvd = 3;
-  // Should not write ack blocks if there is only ack to write
-  EXPECT_EQ(
-      0,
-      writeQuicDataToSocket(
-          *socket_,
-          conn,
-          *conn.clientConnectionId,
-          *conn.serverConnectionId,
-          *aead_,
-          *headerCipher_,
-          transport_->getVersion(),
-          conn.transportSettings.writeConnectionDataPacketsLimit));
-}
-
 TEST_F(QuicTransportTest, WritePendingAckIfHavingData) {
   auto& conn = transport_->getConnectionState();
   auto streamId = transport_->createBidirectionalStream().value();