Remove throws from addition and subtraction CC utilities

Matt Joras · facebook-github-bot · commit 956a5eab8232 · 2025-05-29T14:24:41.000-07:00
Summary: Throwing here causes the connection to close. Rather than trying to preserve that behavior, we instead cap the addition/subtraction, log the error, and allow the connection to continue. This kind of problem shouldn't happen in practice and represents a bug that should be fixed if it does.

Reviewed By: jbeshay

Differential Revision: D75009366

fbshipit-source-id: 8d000f1965191504f2a5be70ddd5141087efaf26
diff --git a/quic/congestion_control/Bbr.cpp b/quic/congestion_control/Bbr.cpp
@@ -128,7 +128,9 @@ void BbrCongestionController::onPacketSent(
     exitingQuiescene_ = true;
   }
   addAndCheckOverflow(
-      conn_.lossState.inflightBytes, packet.metadata.encodedSize);
+      conn_.lossState.inflightBytes,
+      packet.metadata.encodedSize,
+      2 * conn_.transportSettings.maxCwndInMss * conn_.udpSendPacketLen);
   if (!ackAggregationStartTime_) {
     ackAggregationStartTime_ = packet.metadata.time;
   }
diff --git a/quic/congestion_control/Bbr2.cpp b/quic/congestion_control/Bbr2.cpp
@@ -90,7 +90,9 @@ void Bbr2CongestionController::onPacketSent(
   }
 
   addAndCheckOverflow(
-      conn_.lossState.inflightBytes, packet.metadata.encodedSize);
+      conn_.lossState.inflightBytes,
+      packet.metadata.encodedSize,
+      2 * conn_.transportSettings.maxCwndInMss * conn_.udpSendPacketLen);
 
   // Maintain cwndLimited flag. We consider the transport being cwnd limited if
   // we are using > 90% of the cwnd.
@@ -771,7 +773,10 @@ void Bbr2CongestionController::probeInflightLongTermUpward() {
   if (probeUpAcks_ >= probeUpCount_) {
     auto delta = probeUpAcks_ / probeUpCount_;
     probeUpAcks_ -= delta * probeUpCount_;
-    addAndCheckOverflow(*inflightLongTerm_, delta);
+    addAndCheckOverflow(
+        *inflightLongTerm_,
+        delta,
+        2 * conn_.transportSettings.maxCwndInMss * conn_.udpSendPacketLen);
   }
   if (roundStart_) {
     raiseInflightLongTermSlope();
diff --git a/quic/congestion_control/CongestionControlFunctions.h b/quic/congestion_control/CongestionControlFunctions.h
@@ -10,7 +10,6 @@
 #include <quic/state/StateData.h>
 
 #include <chrono>
-#include <utility>
 
 namespace quic {
 
@@ -26,24 +25,41 @@ PacingRate calculatePacingRate(
     uint64_t minCwndInMss,
     std::chrono::microseconds rtt);
 
-template <class T1, class T2>
-void addAndCheckOverflow(T1& value, const T2& toAdd) {
-  if (std::numeric_limits<T1>::max() - toAdd < value) {
-    // TODO: the error code is CWND_OVERFLOW but this function can totally be
-    // used for inflight bytes.
-    throw quic::QuicInternalException(
-        "Overflow bytes in flight", quic::LocalErrorCode::CWND_OVERFLOW);
+template <class T1, class T2, class T3 = T1>
+void addAndCheckOverflow(
+    T1& value,
+    const T2& toAdd,
+    const T3& maxValue = std::numeric_limits<T1>::max()) {
+  if (std::numeric_limits<T1>::max() - static_cast<T1>(toAdd) < value) {
+    LOG(ERROR) << "Overflow prevented, capping at max value";
+    value = static_cast<T1>(maxValue);
+  } else {
+    T1 newValue = value + static_cast<T1>(toAdd);
+    if (newValue > static_cast<T1>(maxValue)) {
+      LOG(ERROR) << "Value would exceed max limit, capping at max value";
+      value = static_cast<T1>(maxValue);
+    } else {
+      value = newValue;
+    }
   }
-  value += folly::to<T1>(toAdd);
 }
 
-template <class T1, class T2>
-void subtractAndCheckUnderflow(T1& value, const T2& toSub) {
-  if (value < toSub) {
-    // TODO: wrong error code
-    throw quic::QuicInternalException(
-        "Underflow bytes in flight", quic::LocalErrorCode::CWND_OVERFLOW);
+template <class T1, class T2, class T3 = T1>
+void subtractAndCheckUnderflow(
+    T1& value,
+    const T2& toSub,
+    const T3& minValue = 0) {
+  if (value < static_cast<T1>(toSub)) {
+    LOG(ERROR) << "Underflow prevented, capping at min value";
+    value = static_cast<T1>(minValue);
+  } else {
+    T1 newValue = value - static_cast<T1>(toSub);
+    if (newValue < static_cast<T1>(minValue)) {
+      LOG(ERROR) << "Value would be below min limit, capping at min value";
+      value = static_cast<T1>(minValue);
+    } else {
+      value = newValue;
+    }
   }
-  value -= folly::to<T1>(toSub);
 }
 } // namespace quic
diff --git a/quic/congestion_control/Copa.cpp b/quic/congestion_control/Copa.cpp
@@ -46,7 +46,9 @@ void Copa::onRemoveBytesFromInflight(uint64_t bytes) {
 
 void Copa::onPacketSent(const OutstandingPacketWrapper& packet) {
   addAndCheckOverflow(
-      conn_.lossState.inflightBytes, packet.metadata.encodedSize);
+      conn_.lossState.inflightBytes,
+      packet.metadata.encodedSize,
+      2 * conn_.transportSettings.maxCwndInMss * conn_.udpSendPacketLen);
 
   VLOG(10) << __func__ << " writable=" << getWritableBytes()
            << " cwnd=" << cwndBytes_
@@ -239,7 +241,10 @@ void Copa::onPacketAcked(const AckEvent& ack) {
           ack.ackTime - lastCwndDoubleTime_.value() > conn_.lossState.srtt) {
         VLOG(10) << __func__ << " doubling cwnd per RTT from=" << cwndBytes_
                  << " due to slow start" << " " << conn_;
-        addAndCheckOverflow(cwndBytes_, cwndBytes_);
+        addAndCheckOverflow(
+            cwndBytes_,
+            cwndBytes_,
+            conn_.transportSettings.maxCwndInMss * conn_.udpSendPacketLen);
         lastCwndDoubleTime_ = ack.ackTime;
       }
     } else {
@@ -256,7 +261,10 @@ void Copa::onPacketAcked(const AckEvent& ack) {
           (deltaParam_ * cwndBytes_);
       VLOG(10) << __func__ << " increasing cwnd from=" << cwndBytes_ << " by "
                << addition << " " << conn_;
-      addAndCheckOverflow(cwndBytes_, addition);
+      addAndCheckOverflow(
+          cwndBytes_,
+          addition,
+          conn_.transportSettings.maxCwndInMss * conn_.udpSendPacketLen);
     }
   } else {
     if (velocityState_.direction != VelocityState::Direction::Down &&
@@ -278,7 +286,8 @@ void Copa::onPacketAcked(const AckEvent& ack) {
         std::min<uint64_t>(
             reduction,
             cwndBytes_ -
-                conn_.transportSettings.minCwndInMss * conn_.udpSendPacketLen));
+                conn_.transportSettings.minCwndInMss * conn_.udpSendPacketLen),
+        conn_.transportSettings.minCwndInMss * conn_.udpSendPacketLen);
   }
   if (conn_.pacer) {
     conn_.pacer->refreshPacingRate(cwndBytes_ * 2, conn_.lossState.srtt);
diff --git a/quic/congestion_control/Copa2.cpp b/quic/congestion_control/Copa2.cpp
@@ -36,7 +36,9 @@ void Copa2::onRemoveBytesFromInflight(uint64_t bytes) {
 
 void Copa2::onPacketSent(const OutstandingPacketWrapper& packet) {
   addAndCheckOverflow(
-      conn_.lossState.inflightBytes, packet.metadata.encodedSize);
+      conn_.lossState.inflightBytes,
+      packet.metadata.encodedSize,
+      2 * conn_.transportSettings.maxCwndInMss * conn_.udpSendPacketLen);
 
   VLOG(10) << __func__ << " writable=" << getWritableBytes()
            << " cwnd=" << cwndBytes_
diff --git a/quic/congestion_control/NewReno.cpp b/quic/congestion_control/NewReno.cpp
@@ -38,7 +38,9 @@ void NewReno::onRemoveBytesFromInflight(uint64_t bytes) {
 
 void NewReno::onPacketSent(const OutstandingPacketWrapper& packet) {
   addAndCheckOverflow(
-      conn_.lossState.inflightBytes, packet.metadata.encodedSize);
+      conn_.lossState.inflightBytes,
+      packet.metadata.encodedSize,
+      2 * conn_.transportSettings.maxCwndInMss * conn_.udpSendPacketLen);
   VLOG(10) << __func__ << " writable=" << getWritableBytes()
            << " cwnd=" << cwndBytes_
            << " inflight=" << conn_.lossState.inflightBytes
@@ -82,15 +84,20 @@ void NewReno::onPacketAcked(
   }
   if (cwndBytes_ < ssthresh_) {
     addAndCheckOverflow(
-        cwndBytes_, packet.outstandingPacketMetadata.encodedSize);
+        cwndBytes_,
+        packet.outstandingPacketMetadata.encodedSize,
+        conn_.transportSettings.maxCwndInMss * conn_.udpSendPacketLen);
   } else {
     // TODO: I think this may be a bug in the specs. We should use
     // conn_.udpSendPacketLen for the cwnd calculation. But I need to
     // check how Linux handles this.
     uint64_t additionFactor = (kDefaultUDPSendPacketLen *
                                packet.outstandingPacketMetadata.encodedSize) /
         cwndBytes_;
-    addAndCheckOverflow(cwndBytes_, additionFactor);
+    addAndCheckOverflow(
+        cwndBytes_,
+        additionFactor,
+        conn_.transportSettings.maxCwndInMss * conn_.udpSendPacketLen);
   }
 }
 
diff --git a/quic/congestion_control/StaticCwndCongestionController.cpp b/quic/congestion_control/StaticCwndCongestionController.cpp
@@ -26,7 +26,10 @@ void StaticCwndCongestionController::onRemoveBytesFromInflight(
 void StaticCwndCongestionController::onPacketSent(
     const OutstandingPacketWrapper& packet) {
   isAppLimited_ = false;
-  addAndCheckOverflow(inflightBytes_, packet.metadata.encodedSize);
+  addAndCheckOverflow(
+      inflightBytes_,
+      packet.metadata.encodedSize,
+      conn_.transportSettings.maxCwndInMss * conn_.udpSendPacketLen);
 }
 
 void StaticCwndCongestionController::onPacketAckOrLoss(

Original file line number	Diff line number	Diff line change
`@@ -128,7 +128,9 @@ void BbrCongestionController::onPacketSent(`
`128`	`128`	`exitingQuiescene_ = true;`
`129`	`129`	`}`
`130`	`130`	`addAndCheckOverflow(`
`131`		`- conn_.lossState.inflightBytes, packet.metadata.encodedSize);`
	`131`	`+ conn_.lossState.inflightBytes,`
	`132`	`+ packet.metadata.encodedSize,`
	`133`	`+ 2 * conn_.transportSettings.maxCwndInMss * conn_.udpSendPacketLen);`
`132`	`134`	`if (!ackAggregationStartTime_) {`
`133`	`135`	`ackAggregationStartTime_ = packet.metadata.time;`
`134`	`136`	`}`