Handle lossTime before crypto timer in Quic

Summary:
Currently we handle crypto timer before loss timer. And currently loss
time is for AppData only since for the other two Packet Number spaces, crypto
timer will take care of it. This diff extends loss times to 3 loss times, and
handle them before handle crypto timer. The rational here is that loss time is
shorter than crypto timer, so this will make retransmission during crypto
handshake more aggressive. For app data, this diff doesn't change anything.

Reviewed By: sharma95

Differential Revision: D15552405

fbshipit-source-id: bd5c24b0622c72325ffdea36d0802d4939bae854

quic/api/test/QuicTransportTest.cpp

@@ -1001,7 +1001,9 @@ TEST_F(QuicTransportTest, ClonePathChallenge) {
   conn.outstandingHandshakePacketsCount = 0;
   conn.outstandingPureAckPacketsCount = 0;
   conn.outstandingPackets.clear();
-  conn.lossState.lossTime.clear();
+  conn.lossState.initialLossTime.clear();
+  conn.lossState.handshakeLossTime.clear();
+  conn.lossState.appDataLossTime.clear();
 
   PathChallengeFrame pathChallenge(123);
   conn.pendingEvents.pathChallenge = pathChallenge;
@@ -1062,7 +1064,9 @@ TEST_F(QuicTransportTest, OnlyClonePathValidationIfOutstanding) {
   conn.outstandingHandshakePacketsCount = 0;
   conn.outstandingPureAckPacketsCount = 0;
   conn.outstandingPackets.clear();
-  conn.lossState.lossTime.clear();
+  conn.lossState.initialLossTime.clear();
+  conn.lossState.handshakeLossTime.clear();
+  conn.lossState.appDataLossTime.clear();
 
   PathChallengeFrame pathChallenge(123);
   conn.pendingEvents.pathChallenge = pathChallenge;
@@ -1225,7 +1229,9 @@ TEST_F(QuicTransportTest, ClonePathResponse) {
   conn.outstandingHandshakePacketsCount = 0;
   conn.outstandingPureAckPacketsCount = 0;
   conn.outstandingPackets.clear();
-  conn.lossState.lossTime.clear();
+  conn.lossState.initialLossTime.clear();
+  conn.lossState.handshakeLossTime.clear();
+  conn.lossState.appDataLossTime.clear();
 
   EXPECT_EQ(conn.pendingEvents.frames.size(), 0);
   PathResponseFrame pathResponse(123);
@@ -1337,7 +1343,9 @@ TEST_F(QuicTransportTest, CloneNewConnectionIdFrame) {
   conn.outstandingHandshakePacketsCount = 0;
   conn.outstandingPureAckPacketsCount = 0;
   conn.outstandingPackets.clear();
-  conn.lossState.lossTime.clear();
+  conn.lossState.initialLossTime.clear();
+  conn.lossState.handshakeLossTime.clear();
+  conn.lossState.appDataLossTime.clear();
 
   NewConnectionIdFrame newConnId(
       1, ConnectionId({2, 4, 2, 3}), StatelessResetToken());

quic/loss/QuicLossFunctions.h

@@ -65,7 +65,18 @@ calculateAlarmDuration(const QuicConnectionStateBase& conn) {
   folly::Optional<LossState::AlarmMethod> alarmMethod;
   TimePoint lastSentPacketTime =
       conn.lossState.lastRetransmittablePacketSentTime;
-  if (conn.outstandingHandshakePacketsCount > 0) {
+  auto lossTimeAndSpace = earliestLossTimer(conn);
+  if (lossTimeAndSpace.first) {
+    if (*lossTimeAndSpace.first > lastSentPacketTime) {
+      // We do this so that lastSentPacketTime + alarmDuration = lossTime
+      alarmDuration = std::chrono::duration_cast<std::chrono::microseconds>(
+          *lossTimeAndSpace.first - lastSentPacketTime);
+    } else {
+      // This should trigger an immediate alarm.
+      alarmDuration = 0us;
+    }
+    alarmMethod = LossState::AlarmMethod::EarlyRetransmitOrReordering;
+  } else if (conn.outstandingHandshakePacketsCount > 0) {
     if (conn.lossState.srtt == 0us) {
       alarmDuration = kDefaultInitialRtt * 2;
     } else {
@@ -78,16 +89,6 @@ calculateAlarmDuration(const QuicConnectionStateBase& conn) {
     // Handshake packet loss timer shouldn't be affected by other packets.
     lastSentPacketTime = conn.lossState.lastHandshakePacketSentTime;
     DCHECK_NE(lastSentPacketTime.time_since_epoch().count(), 0);
-  } else if (conn.lossState.lossTime) {
-    if (*conn.lossState.lossTime > lastSentPacketTime) {
-      // We do this so that lastSentPacketTime + alarmDuration = lossTime
-      alarmDuration = std::chrono::duration_cast<std::chrono::microseconds>(
-          *conn.lossState.lossTime - lastSentPacketTime);
-    } else {
-      // This should trigger an immediate alarm.
-      alarmDuration = 0us;
-    }
-    alarmMethod = LossState::AlarmMethod::EarlyRetransmitOrReordering;
   } else {
     auto ptoTimeout = calculatePTO(conn);
     ptoTimeout *= 1 << std::min(conn.lossState.ptoCount, (uint32_t)31);
@@ -208,7 +209,7 @@ folly::Optional<CongestionController::LossEvent> detectLossPackets(
     const LossVisitor& lossVisitor,
     TimePoint lossTime,
     PacketNumberSpace pnSpace) {
-  conn.lossState.lossTime.clear();
+  getLossTime(conn, pnSpace).clear();
   std::chrono::microseconds delayUntilLost =
       std::max(conn.lossState.srtt, conn.lossState.lrtt) * 9 / 8;
   VLOG(10) << __func__ << " outstanding=" << conn.outstandingPackets.size()
@@ -272,10 +273,9 @@ folly::Optional<CongestionController::LossEvent> detectLossPackets(
     iter = conn.outstandingPackets.erase(iter);
   }
 
-  // Because we handle handshake timer before lossTime, lossTime is used by
-  // AppData space only. So this is fine.
   auto earliest = getFirstOutstandingPacket(conn, pnSpace);
-  for (; earliest != conn.outstandingPackets.end(); earliest++) {
+  for (; earliest != conn.outstandingPackets.end();
+       earliest = getNextOutstandingPacket(conn, pnSpace, earliest + 1)) {
     if (!earliest->pureAck &&
         (!earliest->associatedEvent ||
          conn.outstandingPacketEvents.count(*earliest->associatedEvent))) {
@@ -289,7 +289,7 @@ folly::Optional<CongestionController::LossEvent> detectLossPackets(
              << conn.outstandingPackets.empty() << " delayUntilLost"
              << delayUntilLost.count() << "us"
              << " " << conn;
-    conn.lossState.lossTime = delayUntilLost + earliest->time;
+    getLossTime(conn, pnSpace) = delayUntilLost + earliest->time;
   }
   if (lossEvent.largestLostPacketNum.hasValue()) {
     DCHECK(lossEvent.largestLostSentTime && lossEvent.smallestLostSentTime);
@@ -374,18 +374,16 @@ void onLossDetectionAlarm(
     VLOG(10) << "Transmission alarm fired with no outstanding packets " << conn;
     return;
   }
-  // TODO: The specs prioritize EarlyRetransmitOrReordering over crypto timer.
+  if (conn.lossState.currentAlarmMethod ==
-  if (conn.lossState.currentAlarmMethod == LossState::AlarmMethod::Handshake) {
-    onHandshakeAlarm<LossVisitor, ClockType>(conn, lossVisitor);
-  } else if (
-      conn.lossState.currentAlarmMethod ==
       LossState::AlarmMethod::EarlyRetransmitOrReordering) {
+    auto lossTimeAndSpace = earliestLossTimer(conn);
+    CHECK(lossTimeAndSpace.first);
     auto lossEvent = detectLossPackets<LossVisitor>(
         conn,
-        getAckState(conn, PacketNumberSpace::AppData).largestAckedByPeer,
+        getAckState(conn, lossTimeAndSpace.second).largestAckedByPeer,
         lossVisitor,
         now,
-        PacketNumberSpace::AppData);
+        lossTimeAndSpace.second);
     if (conn.congestionController && lossEvent) {
       DCHECK(lossEvent->largestLostSentTime && lossEvent->smallestLostSentTime);
       lossEvent->persistentCongestion = isPersistentCongestion(
@@ -395,6 +393,9 @@ void onLossDetectionAlarm(
       conn.congestionController->onPacketAckOrLoss(
           folly::none, std::move(lossEvent));
     }
+  } else if (
+      conn.lossState.currentAlarmMethod == LossState::AlarmMethod::Handshake) {
+    onHandshakeAlarm<LossVisitor, ClockType>(conn, lossVisitor);
   } else {
     onPTOAlarm(conn);
   }

quic/loss/test/QuicLossFunctionsTest.cpp

@@ -766,7 +766,52 @@ TEST_F(QuicLossFunctionsTest, TestTimeReordering) {
           ->packet.header,
       [](const auto& h) { return h.getPacketSequenceNum(); });
   EXPECT_EQ(packetNum, 6);
-  EXPECT_TRUE(conn->lossState.lossTime);
+  EXPECT_TRUE(conn->lossState.appDataLossTime);
+}
+
+TEST_F(QuicLossFunctionsTest, LossTimePreemptsCryptoTimer) {
+  std::vector<PacketNum> lostPackets;
+  auto conn = createConn();
+  conn->lossState.srtt = 100ms;
+  conn->lossState.lrtt = 100ms;
+  auto expectedDelayUntilLost = 900000us / 8;
+  auto sendTime = Clock::now();
+  // Send two:
+  sendPacket(*conn, sendTime, false, folly::none, PacketType::Handshake);
+  PacketNum second = sendPacket(
+      *conn, sendTime + 1ms, false, folly::none, PacketType::Handshake);
+  auto lossTime = sendTime + 50ms;
+  detectLossPackets<decltype(testingLossMarkFunc(lostPackets))>(
+      *conn,
+      second,
+      testingLossMarkFunc(lostPackets),
+      lossTime,
+      PacketNumberSpace::Handshake);
+  EXPECT_TRUE(lostPackets.empty());
+  EXPECT_TRUE(conn->lossState.handshakeLossTime.hasValue());
+  EXPECT_EQ(
+      expectedDelayUntilLost + sendTime,
+      conn->lossState.handshakeLossTime.value());
+
+  MockClock::mockNow = [=]() { return sendTime; };
+  auto alarm = calculateAlarmDuration<MockClock>(*conn);
+  EXPECT_EQ(
+      std::chrono::duration_cast<std::chrono::milliseconds>(
+          expectedDelayUntilLost),
+      alarm.first);
+  EXPECT_EQ(LossState::AlarmMethod::EarlyRetransmitOrReordering, alarm.second);
+  // Manual set lossState. Calling setLossDetectionAlarm requries a Timeout
+  conn->lossState.currentAlarmMethod = alarm.second;
+
+  // Second packet gets acked:
+  getAckState(*conn, PacketNumberSpace::Handshake).largestAckedByPeer = second;
+  conn->outstandingPackets.pop_back();
+  MockClock::mockNow = [=]() { return sendTime + expectedDelayUntilLost + 5s; };
+  onLossDetectionAlarm<decltype(testingLossMarkFunc(lostPackets)), MockClock>(
+      *conn, testingLossMarkFunc(lostPackets));
+  EXPECT_EQ(1, lostPackets.size());
+  EXPECT_FALSE(conn->lossState.handshakeLossTime.hasValue());
+  EXPECT_TRUE(conn->outstandingPackets.empty());
 }
 
 TEST_F(QuicLossFunctionsTest, PTONoLongerMarksPacketsToBeRetransmitted) {
@@ -805,6 +850,7 @@ TEST_F(
       .WillRepeatedly(Return());
   std::vector<PacketNum> lostPackets;
   PacketNum expectedLargestLostNum = 0;
+  conn->lossState.currentAlarmMethod = LossState::AlarmMethod::Handshake;
   for (auto i = 0; i < 10; i++) {
     // Half are handshakes
     auto sentPacketNum = sendPacket(
@@ -840,6 +886,7 @@ TEST_F(
 TEST_F(QuicLossFunctionsTest, HandshakeAlarmWithOneRttCipher) {
   auto conn = createClientConn();
   conn->oneRttWriteCipher = createNoOpAead();
+  conn->lossState.currentAlarmMethod = LossState::AlarmMethod::Handshake;
   std::vector<PacketNum> lostPackets;
   sendPacket(
       *conn, TimePoint(100ms), false, folly::none, PacketType::Handshake);
@@ -933,7 +980,7 @@ TEST_F(QuicLossFunctionsTest, AlarmDurationHasLossTime) {
   TimePoint lastPacketSentTime = Clock::now();
   auto thisMoment = lastPacketSentTime;
   MockClock::mockNow = [=]() { return thisMoment; };
-  conn->lossState.lossTime = thisMoment + 100ms;
+  conn->lossState.appDataLossTime = thisMoment + 100ms;
   conn->lossState.srtt = 200ms;
   conn->lossState.lrtt = 150ms;
 
@@ -951,7 +998,7 @@ TEST_F(QuicLossFunctionsTest, AlarmDurationLossTimeIsZero) {
   TimePoint lastPacketSentTime = Clock::now();
   auto thisMoment = lastPacketSentTime + 200ms;
   MockClock::mockNow = [=]() { return thisMoment; };
-  conn->lossState.lossTime = lastPacketSentTime + 100ms;
+  conn->lossState.appDataLossTime = lastPacketSentTime + 100ms;
   conn->lossState.srtt = 200ms;
   conn->lossState.lrtt = 150ms;
 

quic/server/test/QuicServerTransportTest.cpp

@@ -1540,7 +1540,9 @@ TEST_F(QuicServerTransportTest, TestCloneStopSending) {
   server->getNonConstConn().outstandingHandshakePacketsCount = 0;
   server->getNonConstConn().outstandingPureAckPacketsCount = 0;
   server->getNonConstConn().outstandingPackets.clear();
-  server->getNonConstConn().lossState.lossTime.clear();
+  server->getNonConstConn().lossState.initialLossTime.clear();
+  server->getNonConstConn().lossState.handshakeLossTime.clear();
+  server->getNonConstConn().lossState.appDataLossTime.clear();
 
   server->stopSending(streamId, GenericApplicationErrorCode::UNKNOWN);
   loopForWrites();

quic/state/QuicStateFunctions.cpp

@@ -11,6 +11,24 @@
 #include <quic/common/TimeUtil.h>
 #include <quic/logging/QuicLogger.h>
 
+namespace {
+template <typename V, typename A>
+std::pair<folly::Optional<V>, A> minOptional(
+    std::pair<folly::Optional<V>, A> p1,
+    std::pair<folly::Optional<V>, A> p2) {
+  if (!p1.first && !p2.first) {
+    return std::make_pair(folly::none, p1.second);
+  }
+  if (!p1.first) {
+    return p2;
+  }
+  if (!p2.first) {
+    return p1;
+  }
+  return *p1.first < *p2.first ? p1 : p2;
+}
+} // namespace
+
 namespace quic {
 
 void updateRtt(
@@ -240,4 +258,30 @@ bool hasReceivedPackets(const QuicConnectionStateBase& conn) noexcept {
       conn.ackStates.handshakeAckState.largestReceivedPacketNum ||
       conn.ackStates.appDataAckState.largestReceivedPacketNum;
 }
+
+folly::Optional<TimePoint>& getLossTime(
+    QuicConnectionStateBase& conn,
+    PacketNumberSpace pnSpace) noexcept {
+  switch (pnSpace) {
+    case PacketNumberSpace::Initial:
+      return conn.lossState.initialLossTime;
+    case PacketNumberSpace::Handshake:
+      return conn.lossState.handshakeLossTime;
+    case PacketNumberSpace::AppData:
+      return conn.lossState.appDataLossTime;
+  }
+  folly::assume_unreachable();
+}
+
+std::pair<folly::Optional<TimePoint>, PacketNumberSpace> earliestLossTimer(
+    const QuicConnectionStateBase& conn) noexcept {
+  return minOptional(
+      minOptional(
+          std::make_pair(
+              conn.lossState.initialLossTime, PacketNumberSpace::Initial),
+          std::make_pair(
+              conn.lossState.handshakeLossTime, PacketNumberSpace::Handshake)),
+      std::make_pair(
+          conn.lossState.appDataLossTime, PacketNumberSpace::AppData));
+}
 } // namespace quic

quic/state/QuicStateFunctions.h

@@ -114,4 +114,11 @@ bool hasNotReceivedNewPacketsSinceLastCloseSent(
 
 void updateLargestReceivedPacketsAtLastCloseSent(
     QuicConnectionStateBase& conn) noexcept;
+
+folly::Optional<TimePoint>& getLossTime(
+    QuicConnectionStateBase& conn,
+    PacketNumberSpace pnSpace) noexcept;
+
+std::pair<folly::Optional<TimePoint>, PacketNumberSpace> earliestLossTimer(
+    const QuicConnectionStateBase& conn) noexcept;
 } // namespace quic

quic/state/StateData.h

@@ -313,9 +313,10 @@ struct LossState {
   // Reordering threshold used
   uint32_t reorderingThreshold{kReorderingThreshold};
   // Timer for time reordering detection or early retransmit alarm.
-  folly::Optional<TimePoint> lossTime;
+  folly::Optional<TimePoint> initialLossTime, handshakeLossTime,
+      appDataLossTime;
   // Current method by which the loss detection alarm is set.
-  AlarmMethod currentAlarmMethod{AlarmMethod::Handshake};
+  AlarmMethod currentAlarmMethod{AlarmMethod::EarlyRetransmitOrReordering};
   // Total number of packet retransmitted on this connection, including packet
   // clones, retransmitted clones, handshake and rejected zero rtt packets.
   uint32_t rtxCount{0};

quic/state/test/QuicStateFunctionsTest.cpp

@@ -529,6 +529,24 @@ TEST_P(QuicStateFunctionsTest, HasNotReceivedNewPacketsSinceLastClose) {
   EXPECT_TRUE(hasReceivedPacketsAtLastCloseSent(conn));
 }
 
+TEST_F(QuicStateFunctionsTest, EarliestLossTimer) {
+  QuicConnectionStateBase conn(QuicNodeType::Server);
+  EXPECT_FALSE(earliestLossTimer(conn).first.hasValue());
+  auto currentTime = Clock::now();
+  conn.lossState.initialLossTime = currentTime;
+  EXPECT_EQ(PacketNumberSpace::Initial, earliestLossTimer(conn).second);
+  EXPECT_EQ(currentTime, earliestLossTimer(conn).first.value());
+  conn.lossState.appDataLossTime = currentTime - 1s;
+  EXPECT_EQ(PacketNumberSpace::AppData, earliestLossTimer(conn).second);
+  EXPECT_EQ(currentTime - 1s, earliestLossTimer(conn).first.value());
+  conn.lossState.handshakeLossTime = currentTime + 1s;
+  EXPECT_EQ(PacketNumberSpace::AppData, earliestLossTimer(conn).second);
+  EXPECT_EQ(currentTime - 1s, earliestLossTimer(conn).first.value());
+  conn.lossState.appDataLossTime= currentTime + 1s;
+  EXPECT_EQ(PacketNumberSpace::Initial, earliestLossTimer(conn).second);
+  EXPECT_EQ(currentTime, earliestLossTimer(conn).first.value());
+}
+
 INSTANTIATE_TEST_CASE_P(
     QuicStateFunctionsTests,
     QuicStateFunctionsTest,