Fix ack writes scheduling

Summary:
There is a bug in how we decide if we should schedule a write loop due
to sending Acks. Currently if one PN space has needsToWriteAckImmediately to
true and another PN space has hasAcksToSchedule to true, but no PN space
actually has both to true, we will still schdeule a write loop. But that's
wrong. We won't be able to send anything in that case. This diff fixes that.

Reviewed By: JunqiWang

Differential Revision: D15446413

fbshipit-source-id: b7e49332dd7ac7f78fc3ea28f83dc49ccc758bb0

quic/api/QuicPacketScheduler.cpp

@@ -44,33 +44,6 @@ bool hasAcksToSchedule(const AckState& ackState) {
   return *largestAckSend > *(ackState.largestAckScheduled);
 }
 
-bool neverWrittenAcksBefore(const QuicConnectionStateBase& conn) {
-  return (
-      !conn.ackStates.initialAckState.largestAckScheduled &&
-      !conn.ackStates.handshakeAckState.largestAckScheduled &&
-      !conn.ackStates.appDataAckState.largestAckScheduled);
-}
-
-bool hasAcksToSchedule(const QuicConnectionStateBase& conn) {
-  bool initialSpaceHasAcks = hasAcksToSchedule(conn.ackStates.initialAckState);
-  bool handshakeSpaceHasAcks =
-      hasAcksToSchedule(conn.ackStates.handshakeAckState);
-  bool appDataSpaceHasAcks = hasAcksToSchedule(conn.ackStates.appDataAckState);
-  bool cannotWriteInitialAcks =
-      !conn.initialWriteCipher || !initialSpaceHasAcks;
-  bool cannotWriteHandshakeAcks =
-      !conn.handshakeWriteCipher || !handshakeSpaceHasAcks;
-  bool cannotWriteAppDataAcks = !conn.oneRttWriteCipher || !appDataSpaceHasAcks;
-  if (cannotWriteInitialAcks && cannotWriteHandshakeAcks &&
-      cannotWriteAppDataAcks) {
-    return false;
-  }
-  if (neverWrittenAcksBefore(conn)) {
-    return true;
-  }
-  return initialSpaceHasAcks || handshakeSpaceHasAcks || appDataSpaceHasAcks;
-}
-
 folly::Optional<PacketNum> largestAckToSend(const AckState& ackState) {
   if (ackState.acks.empty()) {
     return folly::none;

quic/api/QuicPacketScheduler.h

@@ -215,9 +215,7 @@ class AckScheduler {
  * Returns whether or not the Ack scheduler has acks to schedule. This does not
  * tell you when the ACKs can be written.
  */
-bool hasAcksToSchedule(const QuicConnectionStateBase& conn);
 bool hasAcksToSchedule(const AckState& ackState);
-bool neverWrittenAcksBefore(const QuicConnectionStateBase& conn);
 
 /**
  * Returns the largest packet received which needs to be acked.

quic/api/QuicTransportFunctions.cpp

@@ -56,6 +56,28 @@ std::string largestAckToSendToString(
       optionalToString(largestAckToSend(conn.ackStates.appDataAckState)),
       "]");
 }
+
+bool toWriteInitialAcks(const quic::QuicConnectionStateBase& conn) {
+  return (
+      conn.initialWriteCipher &&
+      hasAcksToSchedule(conn.ackStates.initialAckState) &&
+      conn.ackStates.initialAckState.needsToSendAckImmediately);
+}
+
+bool toWriteHandshakeAcks(const quic::QuicConnectionStateBase& conn) {
+  return (
+      conn.handshakeWriteCipher &&
+      hasAcksToSchedule(conn.ackStates.handshakeAckState) &&
+      conn.ackStates.handshakeAckState.needsToSendAckImmediately);
+}
+
+bool toWriteAppDataAcks(const quic::QuicConnectionStateBase& conn) {
+  return (
+      conn.oneRttWriteCipher &&
+      hasAcksToSchedule(conn.ackStates.appDataAckState) &&
+      conn.ackStates.appDataAckState.needsToSendAckImmediately);
+}
+
 } // namespace
 
 namespace quic {
@@ -1019,16 +1041,17 @@ bool hasAckDataToWrite(const QuicConnectionStateBase& conn) {
   // have an immediate need to schedule the acks then we need to wait till we
   // satisfy a condition where there is immediate need, so we shouldn't
   // consider the acks to be writable.
-  if (needsToSendAckImmediately(conn) && hasAcksToSchedule(conn)) {
-    VLOG(10) << nodeToString(conn.nodeType)
+  bool writeAcks =
+      (toWriteInitialAcks(conn) || toWriteHandshakeAcks(conn) ||
+       toWriteAppDataAcks(conn));
+  VLOG_IF(10, writeAcks) << nodeToString(conn.nodeType)
                          << " needs write because of acks largestAck="
-             << largestAckToSendToString(conn)
-             << " largestSentAck=" << largestAckScheduledToString(conn)
-             << " ackTimeoutSet=" << conn.pendingEvents.scheduleAckTimeout
-             << " " << conn;
-    return true;
-  }
-  return false;
+                         << largestAckToSendToString(conn) << " largestSentAck="
+                         << largestAckScheduledToString(conn)
+                         << " ackTimeoutSet="
+                         << conn.pendingEvents.scheduleAckTimeout << " "
+                         << conn;
+  return writeAcks;
 }
 
 bool hasNonAckDataToWrite(const QuicConnectionStateBase& conn) {

quic/api/test/QuicPacketSchedulerTest.cpp

@@ -666,19 +666,6 @@ TEST_F(QuicPacketSchedulerTest, AckSchedulerHasAcksToSchedule) {
   EXPECT_TRUE(handshakeAckScheduler.hasPendingAcks());
 }
 
-TEST_F(QuicPacketSchedulerTest, ConnHasAcksToSchedule) {
-  QuicClientConnectionState conn;
-  EXPECT_FALSE(hasAcksToSchedule(conn));
-  conn.ackStates.initialAckState.acks.insert(0, 100);
-  EXPECT_FALSE(hasAcksToSchedule(conn));
-  conn.initialWriteCipher = createNoOpAead();
-  EXPECT_TRUE(hasAcksToSchedule(conn));
-  conn.ackStates.initialAckState.acks.clear();
-  EXPECT_FALSE(hasAcksToSchedule(conn));
-  conn.oneRttWriteCipher = createNoOpAead();
-  EXPECT_FALSE(hasAcksToSchedule(conn));
-}
-
 TEST_F(QuicPacketSchedulerTest, LargestAckToSend) {
   QuicClientConnectionState conn;
   EXPECT_EQ(folly::none, largestAckToSend(conn.ackStates.initialAckState));

quic/api/test/QuicTransportFunctionsTest.cpp

@@ -1382,6 +1382,19 @@ TEST_F(QuicTransportFunctionsTest, HasAckDataToWrite) {
   EXPECT_TRUE(hasAckDataToWrite(*conn));
 }
 
+TEST_F(QuicTransportFunctionsTest, HasAckDataToWriteMismatch) {
+  // When one ack space has needsToSendAckImmediately = true and another has
+  // hasAckToSchedule = true, but no ack space has both of them to true, we
+  // should not send.
+  auto conn = createConn();
+  EXPECT_FALSE(hasAckDataToWrite(*conn));
+  conn->ackStates.initialAckState.needsToSendAckImmediately = true;
+  EXPECT_FALSE(hasAckDataToWrite(*conn));
+  conn->ackStates.handshakeAckState.acks.insert(0, 10);
+  conn->handshakeWriteCipher = test::createNoOpAead();
+  EXPECT_FALSE(hasAckDataToWrite(*conn));
+}
+
 TEST_F(QuicTransportFunctionsTest, HasCryptoDataToWrite) {
   auto conn = createConn();
   conn->cryptoState->initialStream.lossBuffer.emplace_back(

quic/state/QuicStateFunctions.cpp

@@ -165,12 +165,6 @@ AckStateVersion currentAckStateVersion(
       conn.ackStates.appDataAckState.acks.insertVersion());
 }
 
-bool needsToSendAckImmediately(const QuicConnectionStateBase& conn) noexcept {
-  return conn.ackStates.initialAckState.needsToSendAckImmediately ||
-      conn.ackStates.handshakeAckState.needsToSendAckImmediately ||
-      conn.ackStates.appDataAckState.needsToSendAckImmediately;
-}
-
 PacketNum getNextPacketNum(
     const QuicConnectionStateBase& conn,
     PacketNumberSpace pnSpace) noexcept {

quic/state/QuicStateFunctions.h

@@ -65,8 +65,6 @@ const AckState& getAckState(
 AckStateVersion currentAckStateVersion(
     const QuicConnectionStateBase& conn) noexcept;
 
-bool needsToSendAckImmediately(const QuicConnectionStateBase& conn) noexcept;
-
 PacketNum getNextPacketNum(
     const QuicConnectionStateBase& conn,
     PacketNumberSpace pnSpace) noexcept;