diff --git a/quic/api/QuicTransportBase.cpp b/quic/api/QuicTransportBase.cpp index 3f1468e7e..20459eb54 100644 --- a/quic/api/QuicTransportBase.cpp +++ b/quic/api/QuicTransportBase.cpp @@ -2706,12 +2706,17 @@ void QuicTransportBase::setTransportSettings( } setCongestionControl(transportSettings.defaultCongestionController); if (conn_->transportSettings.pacingEnabled) { - conn_->pacer = std::make_unique( - *conn_, - transportSettings.defaultCongestionController == - CongestionControlType::BBR - ? kMinCwndInMssForBbr - : conn_->transportSettings.minCwndInMss); + if (writeLooper_->hasPacingTimer()) { + bool usingBbr = conn_->congestionController && + (conn_->congestionController->type() == CongestionControlType::BBR); + conn_->pacer = std::make_unique( + *conn_, + usingBbr ? kMinCwndInMssForBbr + : conn_->transportSettings.minCwndInMss); + } else { + LOG(ERROR) << "Pacing cannot be enabled without a timer"; + conn_->transportSettings.pacingEnabled = false; + } } } @@ -2729,11 +2734,12 @@ void QuicTransportBase::setCongestionControl(CongestionControlType type) { type != conn_->congestionController->type()) { CHECK(ccFactory_); - // We need to enable pacing if we're switching to BBR. - if (type == CongestionControlType::BBR) { - conn_->transportSettings.pacingEnabled = true; - conn_->pacer = - std::make_unique(*conn_, kMinCwndInMssForBbr); + // Fallback to Cubic if Pacing isn't enabled with BBR together + if (type == CongestionControlType::BBR && + (!conn_->transportSettings.pacingEnabled || + !writeLooper_->hasPacingTimer())) { + LOG(ERROR) << "Unpaced BBR isn't supported"; + type = CongestionControlType::Cubic; } conn_->congestionController = diff --git a/quic/api/test/QuicTransportTest.cpp b/quic/api/test/QuicTransportTest.cpp index 9fd937cb4..cd1bd0b79 100644 --- a/quic/api/test/QuicTransportTest.cpp +++ b/quic/api/test/QuicTransportTest.cpp @@ -2450,6 +2450,37 @@ TEST_F(QuicTransportTest, NotifyPendingWriteConnBufferFreeUpSpace) { NetworkData(IOBuf::copyBuffer("fake data"), Clock::now())); } +TEST_F(QuicTransportTest, NoPacingTimerNoPacing) { + TransportSettings transportSettings; + transportSettings.pacingEnabled = true; + transport_->setTransportSettings(transportSettings); + transport_->getConnectionState().canBePaced = true; + EXPECT_FALSE(isConnectionPaced(transport_->getConnectionState())); +} + +TEST_F(QuicTransportTest, SetPacingTimerThenEnablesPacing) { + TransportSettings transportSettings; + transportSettings.pacingEnabled = true; + transport_->setPacingTimer( + TimerHighRes::newTimer(&evb_, transportSettings.pacingTimerTickInterval)); + transport_->setTransportSettings(transportSettings); + transport_->getConnectionState().canBePaced = true; + EXPECT_TRUE(isConnectionPaced(transport_->getConnectionState())); +} + +TEST_F(QuicTransportTest, NoPacingNoBbr) { + TransportSettings transportSettings; + transportSettings.defaultCongestionController = CongestionControlType::BBR; + transportSettings.pacingEnabled = false; + auto ccFactory = std::make_shared(); + transport_->setCongestionControllerFactory(ccFactory); + transport_->setTransportSettings(transportSettings); + EXPECT_FALSE(isConnectionPaced(transport_->getConnectionState())); + EXPECT_NE( + CongestionControlType::BBR, + transport_->getTransportInfo().congestionControlType); +} + TEST_F(QuicTransportTest, NotifyPendingWriteConnBufferUseTotalSpace) { TransportSettings transportSettings; transportSettings.totalBufferSpaceAvailable = 100; diff --git a/quic/common/FunctionLooper.cpp b/quic/common/FunctionLooper.cpp index f8bb1fedb..3bdcfd9cd 100644 --- a/quic/common/FunctionLooper.cpp +++ b/quic/common/FunctionLooper.cpp @@ -23,6 +23,10 @@ void FunctionLooper::setPacingTimer( pacingTimer_ = std::move(pacingTimer); } +bool FunctionLooper::hasPacingTimer() const noexcept { + return pacingTimer_ != nullptr; +} + void FunctionLooper::setPacingFunction( folly::Function&& pacingFunc) { pacingFunc_ = std::move(pacingFunc); diff --git a/quic/common/FunctionLooper.h b/quic/common/FunctionLooper.h index ad986a198..b66639a1b 100644 --- a/quic/common/FunctionLooper.h +++ b/quic/common/FunctionLooper.h @@ -41,6 +41,8 @@ class FunctionLooper : public folly::EventBase::LoopCallback, void setPacingTimer(TimerHighRes::SharedPtr pacingTimer) noexcept; + bool hasPacingTimer() const noexcept; + void runLoopCallback() noexcept override; /** diff --git a/quic/fizz/client/test/QuicClientTransportTest.cpp b/quic/fizz/client/test/QuicClientTransportTest.cpp index 6bb346494..23638d3ba 100644 --- a/quic/fizz/client/test/QuicClientTransportTest.cpp +++ b/quic/fizz/client/test/QuicClientTransportTest.cpp @@ -5087,16 +5087,12 @@ TEST_F(QuicClientTransportAfterStartTest, SetCongestionControlBbr) { auto cc = client->getConn().congestionController.get(); EXPECT_EQ(CongestionControlType::Cubic, cc->type()); - // Pacing should be disabled. - EXPECT_FALSE(isConnectionPaced(client->getConn())); - - // Change to BBR + // Change to BBR, which requires enable pacing first + client->setPacingTimer(TimerHighRes::newTimer(eventbase_.get(), 1ms)); + client->getNonConstConn().transportSettings.pacingEnabled = true; client->setCongestionControl(CongestionControlType::BBR); cc = client->getConn().congestionController.get(); EXPECT_EQ(CongestionControlType::BBR, cc->type()); - - // Pacing should be enabled. - EXPECT_TRUE(isConnectionPaced(client->getConn())); } TEST_F(QuicClientTransportAfterStartTest, PingIsTreatedAsRetransmittable) {