diff --git a/quic/api/QuicTransportBase.cpp b/quic/api/QuicTransportBase.cpp index afd80c33f..35c92ac39 100644 --- a/quic/api/QuicTransportBase.cpp +++ b/quic/api/QuicTransportBase.cpp @@ -45,15 +45,15 @@ QuicTransportBase::QuicTransportBase( d6dTxTimeout_(this), readLooper_(new FunctionLooper( evb, - [this](bool /* ignored */) { invokeReadDataAndCallbacks(); }, + [this]() { invokeReadDataAndCallbacks(); }, LooperType::ReadLooper)), peekLooper_(new FunctionLooper( evb, - [this](bool /* ignored */) { invokePeekDataAndCallbacks(); }, + [this]() { invokePeekDataAndCallbacks(); }, LooperType::PeekLooper)), writeLooper_(new FunctionLooper( evb, - [this](bool fromTimer) { pacedWriteDataToSocket(fromTimer); }, + [this]() { pacedWriteDataToSocket(); }, LooperType::WriteLooper)) { writeLooper_->setPacingFunction([this]() -> auto { if (isConnectionPaced(*conn_)) { @@ -2668,7 +2668,7 @@ void QuicTransportBase::lossTimeoutExpired() noexcept { // probe to be scheduled scheduleD6DRaiseTimeout(); scheduleD6DTxTimeout(); - pacedWriteDataToSocket(false); + pacedWriteDataToSocket(); } catch (const QuicTransportException& ex) { VLOG(4) << __func__ << " " << ex.what() << " " << *this; exceptionCloseWhat_ = ex.what(); @@ -2695,7 +2695,7 @@ void QuicTransportBase::ackTimeoutExpired() noexcept { VLOG(10) << __func__ << " " << *this; FOLLY_MAYBE_UNUSED auto self = sharedGuard(); updateAckStateOnAckTimeout(*conn_); - pacedWriteDataToSocket(false); + pacedWriteDataToSocket(); } void QuicTransportBase::pingTimeoutExpired() noexcept { @@ -3570,7 +3570,7 @@ void QuicTransportBase::runOnEvbAsync( true); } -void QuicTransportBase::pacedWriteDataToSocket(bool /* fromTimer */) { +void QuicTransportBase::pacedWriteDataToSocket() { FOLLY_MAYBE_UNUSED auto self = sharedGuard(); if (!isConnectionPaced(*conn_)) { diff --git a/quic/api/QuicTransportBase.h b/quic/api/QuicTransportBase.h index fb73b0270..5badd8278 100644 --- a/quic/api/QuicTransportBase.h +++ b/quic/api/QuicTransportBase.h @@ -784,13 +784,13 @@ class QuicTransportBase : public QuicSocket, QuicStreamPrioritiesObserver { /** * Paced write data to socket when connection is paced. * - * Whether connection is based will be decided by TransportSettings and + * Whether connection is paced will be decided by TransportSettings and * congection controller. When the connection is paced, this function writes * out a burst size of packets and let the writeLooper schedule a callback to * write another burst after a pacing interval if there are more data to - * write. When the connection isn't paced, this function do a normal write. + * write. When the connection isn't paced, this function does a normal write. */ - void pacedWriteDataToSocket(bool fromTimer); + void pacedWriteDataToSocket(); uint64_t maxWritableOnStream(const QuicStreamState&) const; uint64_t maxWritableOnConn() const; diff --git a/quic/api/test/QuicTransportTest.cpp b/quic/api/test/QuicTransportTest.cpp index cae4c729a..8df33bd5f 100644 --- a/quic/api/test/QuicTransportTest.cpp +++ b/quic/api/test/QuicTransportTest.cpp @@ -4458,7 +4458,7 @@ TEST_F(QuicTransportTest, PacedWriteNoDataToWrite) { WriteDataReason::NO_WRITE, shouldWriteData(transport_->getConnectionState())); EXPECT_CALL(*socket_, write(_, _)).Times(0); - transport_->pacedWrite(true); + transport_->pacedWrite(); } TEST_F(QuicTransportTest, PacingWillBurstFirst) { @@ -4481,7 +4481,7 @@ TEST_F(QuicTransportTest, PacingWillBurstFirst) { EXPECT_CALL(*socket_, write(_, _)).WillOnce(Return(0)); EXPECT_CALL(*rawPacer, updateAndGetWriteBatchSize(_)) .WillRepeatedly(Return(1)); - transport_->pacedWrite(true); + transport_->pacedWrite(); } TEST_F(QuicTransportTest, AlreadyScheduledPacingNoWrite) { @@ -4515,7 +4515,7 @@ TEST_F(QuicTransportTest, AlreadyScheduledPacingNoWrite) { ASSERT_NE(WriteDataReason::NO_WRITE, shouldWriteData(conn)); EXPECT_TRUE(transport_->isPacingScheduled()); EXPECT_CALL(*socket_, write(_, _)).Times(0); - transport_->pacedWrite(true); + transport_->pacedWrite(); } TEST_F(QuicTransportTest, NoScheduleIfNoNewData) { @@ -4540,7 +4540,7 @@ TEST_F(QuicTransportTest, NoScheduleIfNoNewData) { .WillRepeatedly(Return(1)); // This will write out everything. After that because there is no new data, // FunctionLooper won't schedule a pacing timeout. - transport_->pacedWrite(true); + transport_->pacedWrite(); ASSERT_EQ(WriteDataReason::NO_WRITE, shouldWriteData(conn)); EXPECT_FALSE(transport_->isPacingScheduled()); diff --git a/quic/api/test/TestQuicTransport.h b/quic/api/test/TestQuicTransport.h index 13837eb87..96e656790 100644 --- a/quic/api/test/TestQuicTransport.h +++ b/quic/api/test/TestQuicTransport.h @@ -59,8 +59,8 @@ class TestQuicTransport QuicTransportBase::updateWriteLooper(thisIteration); } - void pacedWrite(bool fromTimer) { - pacedWriteDataToSocket(fromTimer); + void pacedWrite() { + pacedWriteDataToSocket(); } bool isPacingScheduled() { diff --git a/quic/common/FunctionLooper.cpp b/quic/common/FunctionLooper.cpp index e3d248085..29a964c6f 100644 --- a/quic/common/FunctionLooper.cpp +++ b/quic/common/FunctionLooper.cpp @@ -14,7 +14,7 @@ using namespace std::chrono_literals; FunctionLooper::FunctionLooper( folly::EventBase* evb, - folly::Function&& func, + folly::Function&& func, LooperType type) : evb_(evb), func_(std::move(func)), type_(type) {} @@ -32,26 +32,26 @@ void FunctionLooper::setPacingFunction( pacingFunc_ = std::move(pacingFunc); } -void FunctionLooper::commonLoopBody(bool fromTimer) noexcept { +void FunctionLooper::commonLoopBody() noexcept { inLoopBody_ = true; SCOPE_EXIT { inLoopBody_ = false; }; auto hasBeenRunning = running_; - func_(fromTimer); + func_(); // callback could cause us to stop ourselves. // Someone could have also called run() in the callback. - VLOG(10) << __func__ << ": " << type_ << " fromTimer=" << fromTimer - << " hasBeenRunning=" << hasBeenRunning << " running_=" << running_; + VLOG(10) << __func__ << ": " << type_ << " hasBeenRunning=" << hasBeenRunning + << " running_=" << running_; if (!running_) { return; } - if (!schedulePacingTimeout(fromTimer)) { + if (!schedulePacingTimeout()) { evb_->runInLoop(this); } } -bool FunctionLooper::schedulePacingTimeout(bool /* fromTimer */) noexcept { +bool FunctionLooper::schedulePacingTimeout() noexcept { if (pacingFunc_ && pacingTimer_ && !isScheduled()) { auto nextPacingTime = (*pacingFunc_)(); if (nextPacingTime != 0us) { @@ -64,7 +64,7 @@ bool FunctionLooper::schedulePacingTimeout(bool /* fromTimer */) noexcept { void FunctionLooper::runLoopCallback() noexcept { folly::DelayedDestruction::DestructorGuard dg(this); - commonLoopBody(false); + commonLoopBody(); } void FunctionLooper::run(bool thisIteration) noexcept { @@ -112,7 +112,7 @@ void FunctionLooper::detachEventBase() { void FunctionLooper::timeoutExpired() noexcept { folly::DelayedDestruction::DestructorGuard dg(this); - commonLoopBody(true); + commonLoopBody(); } void FunctionLooper::callbackCanceled() noexcept { diff --git a/quic/common/FunctionLooper.h b/quic/common/FunctionLooper.h index 39bffd665..5429b301e 100644 --- a/quic/common/FunctionLooper.h +++ b/quic/common/FunctionLooper.h @@ -35,7 +35,7 @@ class FunctionLooper : public folly::EventBase::LoopCallback, explicit FunctionLooper( folly::EventBase* evb, - folly::Function&& func, + folly::Function&& func, LooperType type); void setPacingTimer(TimerHighRes::SharedPtr pacingTimer) noexcept; @@ -84,11 +84,11 @@ class FunctionLooper : public folly::EventBase::LoopCallback, private: ~FunctionLooper() override = default; - void commonLoopBody(bool fromTimer) noexcept; - bool schedulePacingTimeout(bool fromTimer) noexcept; + void commonLoopBody() noexcept; + bool schedulePacingTimeout() noexcept; folly::EventBase* evb_; - folly::Function func_; + folly::Function func_; folly::Optional> pacingFunc_; TimerHighRes::SharedPtr pacingTimer_; bool running_{false}; diff --git a/quic/common/test/FunctionLooperTest.cpp b/quic/common/test/FunctionLooperTest.cpp index fafde8f9e..6940333d8 100644 --- a/quic/common/test/FunctionLooperTest.cpp +++ b/quic/common/test/FunctionLooperTest.cpp @@ -21,7 +21,7 @@ class FunctionLooperTest : public Test {}; TEST(FunctionLooperTest, LooperNotRunning) { EventBase evb; bool called = false; - auto func = [&](bool) { called = true; }; + auto func = [&]() { called = true; }; FunctionLooper::Ptr looper( new FunctionLooper(&evb, std::move(func), LooperType::ReadLooper)); evb.loopOnce(); @@ -34,7 +34,7 @@ TEST(FunctionLooperTest, LooperNotRunning) { TEST(FunctionLooperTest, LooperStarted) { EventBase evb; bool called = false; - auto func = [&](bool) { called = true; }; + auto func = [&]() { called = true; }; FunctionLooper::Ptr looper( new FunctionLooper(&evb, std::move(func), LooperType::ReadLooper)); looper->run(); @@ -49,7 +49,7 @@ TEST(FunctionLooperTest, LooperStarted) { TEST(FunctionLooperTest, LooperStopped) { EventBase evb; bool called = false; - auto func = [&](bool) { called = true; }; + auto func = [&]() { called = true; }; FunctionLooper::Ptr looper( new FunctionLooper(&evb, std::move(func), LooperType::ReadLooper)); looper->run(); @@ -65,7 +65,7 @@ TEST(FunctionLooperTest, LooperStopped) { TEST(FunctionLooperTest, LooperRestarted) { EventBase evb; bool called = false; - auto func = [&](bool) { called = true; }; + auto func = [&]() { called = true; }; FunctionLooper::Ptr looper( new FunctionLooper(&evb, std::move(func), LooperType::ReadLooper)); looper->run(); @@ -86,7 +86,7 @@ TEST(FunctionLooperTest, DestroyLooperDuringFunc) { bool called = false; FunctionLooper::Ptr* looperPtr = nullptr; - auto func = [&](bool) { + auto func = [&]() { called = true; *looperPtr = nullptr; }; @@ -105,7 +105,7 @@ TEST(FunctionLooperTest, StopLooperDuringFunc) { bool called = false; FunctionLooper::Ptr* looperPtr = nullptr; - auto func = [&](bool) { + auto func = [&]() { called = true; (*looperPtr)->stop(); }; @@ -126,7 +126,7 @@ TEST(FunctionLooperTest, RunLooperDuringFunc) { bool called = false; FunctionLooper::Ptr* looperPtr = nullptr; - auto func = [&](bool) { + auto func = [&]() { called = true; (*looperPtr)->run(); }; @@ -145,7 +145,7 @@ TEST(FunctionLooperTest, RunLooperDuringFunc) { TEST(FunctionLooperTest, DetachStopsLooper) { EventBase evb; bool called = false; - auto func = [&](bool) { called = true; }; + auto func = [&]() { called = true; }; FunctionLooper::Ptr looper( new FunctionLooper(&evb, std::move(func), LooperType::ReadLooper)); looper->run(); @@ -159,8 +159,8 @@ TEST(FunctionLooperTest, DetachStopsLooper) { TEST(FunctionLooperTest, PacingOnce) { EventBase evb; TimerHighRes::SharedPtr pacingTimer(TimerHighRes::newTimer(&evb, 1ms)); - std::vector fromTimerVec; - auto func = [&](bool fromTimer) { fromTimerVec.push_back(fromTimer); }; + int count = 0; + auto func = [&]() { ++count; }; bool firstTime = true; auto pacingFunc = [&]() -> auto { if (firstTime) { @@ -175,20 +175,18 @@ TEST(FunctionLooperTest, PacingOnce) { looper->setPacingFunction(std::move(pacingFunc)); looper->run(); evb.loopOnce(); - EXPECT_EQ(1, fromTimerVec.size()); - EXPECT_FALSE(fromTimerVec.back()); + EXPECT_EQ(1, count); EXPECT_TRUE(looper->isScheduled()); looper->timeoutExpired(); - EXPECT_EQ(2, fromTimerVec.size()); - EXPECT_TRUE(fromTimerVec.back()); + EXPECT_EQ(2, count); looper->stop(); } TEST(FunctionLooperTest, KeepPacing) { EventBase evb; TimerHighRes::SharedPtr pacingTimer(TimerHighRes::newTimer(&evb, 1ms)); - std::vector fromTimerVec; - auto func = [&](bool fromTimer) { fromTimerVec.push_back(fromTimer); }; + int count = 0; + auto func = [&]() { ++count; }; bool stopPacing = false; auto pacingFunc = [&]() -> auto { if (stopPacing) { @@ -202,30 +200,26 @@ TEST(FunctionLooperTest, KeepPacing) { looper->setPacingFunction(std::move(pacingFunc)); looper->run(); evb.loopOnce(); - EXPECT_EQ(1, fromTimerVec.size()); - EXPECT_FALSE(fromTimerVec.back()); + EXPECT_EQ(1, count); EXPECT_TRUE(looper->isScheduled()); looper->cancelTimeout(); EXPECT_FALSE(looper->isScheduled()); looper->timeoutExpired(); - EXPECT_EQ(2, fromTimerVec.size()); - EXPECT_TRUE(fromTimerVec.back()); + EXPECT_EQ(2, count); EXPECT_TRUE(looper->isScheduled()); looper->cancelTimeout(); EXPECT_FALSE(looper->isScheduled()); looper->timeoutExpired(); - EXPECT_EQ(3, fromTimerVec.size()); - EXPECT_TRUE(fromTimerVec.back()); + EXPECT_EQ(3, count); EXPECT_TRUE(looper->isScheduled()); stopPacing = true; looper->cancelTimeout(); EXPECT_FALSE(looper->isScheduled()); looper->timeoutExpired(); - EXPECT_EQ(4, fromTimerVec.size()); - EXPECT_TRUE(fromTimerVec.back()); + EXPECT_EQ(4, count); EXPECT_FALSE(looper->isScheduled()); looper->stop(); @@ -235,7 +229,7 @@ TEST(FunctionLooperTest, TimerTickSize) { EventBase evb; TimerHighRes::SharedPtr pacingTimer(TimerHighRes::newTimer(&evb, 123ms)); FunctionLooper::Ptr looper(new FunctionLooper( - &evb, [&](bool) {}, LooperType::ReadLooper)); + &evb, [&]() {}, LooperType::ReadLooper)); looper->setPacingTimer(std::move(pacingTimer)); EXPECT_EQ(123ms, looper->getTimerTickInterval()); } @@ -244,7 +238,7 @@ TEST(FunctionLooperTest, TimerTickSizeAfterNewEvb) { EventBase evb; TimerHighRes::SharedPtr pacingTimer(TimerHighRes::newTimer(&evb, 123ms)); FunctionLooper::Ptr looper(new FunctionLooper( - &evb, [&](bool) {}, LooperType::ReadLooper)); + &evb, [&]() {}, LooperType::ReadLooper)); looper->setPacingTimer(std::move(pacingTimer)); EXPECT_EQ(123ms, looper->getTimerTickInterval()); looper->detachEventBase(); @@ -256,14 +250,7 @@ TEST(FunctionLooperTest, TimerTickSizeAfterNewEvb) { TEST(FunctionLooperTest, NoLoopCallbackInPacingMode) { EventBase evb; TimerHighRes::SharedPtr pacingTimer(TimerHighRes::newTimer(&evb, 1ms)); - uint32_t loopCallbackRunCounter = 0, pacingRunCounter = 0; - auto runFunc = [&](bool fromTimer) { - if (!fromTimer) { - loopCallbackRunCounter++; - } else { - pacingRunCounter++; - } - }; + auto runFunc = [&]() {}; auto pacingFunc = [&]() { return 3600000ms; }; FunctionLooper::Ptr looper( new FunctionLooper(&evb, std::move(runFunc), LooperType::ReadLooper)); @@ -278,44 +265,5 @@ TEST(FunctionLooperTest, NoLoopCallbackInPacingMode) { looper->stop(); } -TEST(FunctionLooperTest, RunConditions) { - EventBase evb; - TimerHighRes::SharedPtr pacingTimer(TimerHighRes::newTimer(&evb, 1ms)); - uint32_t loopCallbackRunCounter = 0, pacingRunCounter = 0; - FunctionLooper::Ptr* looperPtr = nullptr; - auto runFunc = [&](bool fromTimer) { - if (!fromTimer) { - loopCallbackRunCounter++; - } else { - pacingRunCounter++; - } - (*looperPtr)->run(); - }; - auto pacingFunc = [&]() { return 3600000ms; }; - FunctionLooper::Ptr looper( - new FunctionLooper(&evb, std::move(runFunc), LooperType::ReadLooper)); - looperPtr = &looper; - looper->setPacingTimer(std::move(pacingTimer)); - looper->setPacingFunction(std::move(pacingFunc)); - // Nothing scheduled yet, this run will loop - looper->run(); - evb.loopOnce(); - EXPECT_EQ(0, pacingRunCounter); - EXPECT_EQ(1, loopCallbackRunCounter); - - // run() inside runFunc didn't have effect. Loop again won't run anything: - evb.loopOnce(); - EXPECT_EQ(0, pacingRunCounter); - EXPECT_EQ(1, loopCallbackRunCounter); - - // Since pacing is scheduled, explicit run() outside of runFunc won't run - // either: - looper->run(); - evb.loopOnce(); - EXPECT_EQ(0, pacingRunCounter); - EXPECT_EQ(1, loopCallbackRunCounter); - - looper->stop(); -} } // namespace test } // namespace quic