diff --git a/quic/api/QuicTransportFunctions.cpp b/quic/api/QuicTransportFunctions.cpp index 5fe5bcf10..f8a20830d 100644 --- a/quic/api/QuicTransportFunctions.cpp +++ b/quic/api/QuicTransportFunctions.cpp @@ -399,6 +399,10 @@ void updateConnection( if (conn.pacer && !pureAck) { conn.pacer->onPacketSent(); } + if (conn.pathValidationLimiter && + (conn.pendingEvents.pathChallenge || conn.outstandingPathValidation)) { + conn.pathValidationLimiter->onPacketSent(pkt.encodedSize); + } if (pkt.isHandshake) { ++conn.outstandingHandshakePacketsCount; conn.lossState.lastHandshakePacketSentTime = pkt.time; @@ -440,14 +444,25 @@ void updateConnection( uint64_t congestionControlWritableBytes(const QuicConnectionStateBase& conn) { uint64_t writableBytes = std::numeric_limits::max(); - if (conn.writableBytesLimit) { + + if (conn.pendingEvents.pathChallenge || conn.outstandingPathValidation) { + CHECK(conn.pathValidationLimiter); + // 0-RTT and path validation rate limiting should be mutually exclusive. + CHECK(!conn.writableBytesLimit); + + // Use the default RTT measurement when starting a new path challenge (CC is + // reset). This shouldn't be an RTT sample, so we do not update the CC with + // this value. + writableBytes = conn.pathValidationLimiter->currentCredit( + std::chrono::steady_clock::now(), + conn.lossState.srtt == 0us ? kDefaultInitialRtt : conn.lossState.srtt); + } else if (conn.writableBytesLimit) { if (*conn.writableBytesLimit <= conn.lossState.totalBytesSent) { return 0; } - writableBytes = std::min( - writableBytes, - *conn.writableBytesLimit - conn.lossState.totalBytesSent); + writableBytes = *conn.writableBytesLimit - conn.lossState.totalBytesSent; } + if (conn.congestionController) { writableBytes = std::min( writableBytes, conn.congestionController->getWritableBytes()); @@ -1046,15 +1061,9 @@ WriteDataReason shouldWriteData(const QuicConnectionStateBase& conn) { } const size_t minimumDataSize = std::max( kLongHeaderHeaderSize + kCipherOverheadHeuristic, sizeof(Sample)); - if (conn.writableBytesLimit && - (*conn.writableBytesLimit <= conn.lossState.totalBytesSent || - *conn.writableBytesLimit - conn.lossState.totalBytesSent <= - minimumDataSize)) { - QUIC_STATS(conn.infoCallback, onCwndBlocked); - return WriteDataReason::NO_WRITE; - } - if (conn.congestionController && - conn.congestionController->getWritableBytes() <= minimumDataSize) { + + uint64_t availableWriteWindow = congestionControlWritableBytes(conn); + if (availableWriteWindow <= minimumDataSize) { QUIC_STATS(conn.infoCallback, onCwndBlocked); return WriteDataReason::NO_WRITE; } diff --git a/quic/api/QuicTransportFunctions.h b/quic/api/QuicTransportFunctions.h index caf253c96..9a3c68242 100644 --- a/quic/api/QuicTransportFunctions.h +++ b/quic/api/QuicTransportFunctions.h @@ -146,6 +146,10 @@ void updateConnection( TimePoint time, uint32_t encodedSize); +/** + * Returns the minimum available bytes window out of path validation rate + * limiting, 0-rtt total bytes sent limiting, and the congestion controller. + */ uint64_t congestionControlWritableBytes(const QuicConnectionStateBase& conn); uint64_t unlimitedWritableBytes(const QuicConnectionStateBase&); diff --git a/quic/api/test/QuicTransportFunctionsTest.cpp b/quic/api/test/QuicTransportFunctionsTest.cpp index f0f65651b..ec9c84a10 100644 --- a/quic/api/test/QuicTransportFunctionsTest.cpp +++ b/quic/api/test/QuicTransportFunctionsTest.cpp @@ -1657,6 +1657,67 @@ TEST_F(QuicTransportFunctionsTest, ShouldWriteDataTest) { EXPECT_EQ(WriteDataReason::NO_WRITE, shouldWriteData(*conn)); } +TEST_F(QuicTransportFunctionsTest, ShouldWriteDataTestDuringPathValidation) { + auto conn = createConn(); + + // Create the CC. + auto mockCongestionController = std::make_unique(); + auto rawCongestionController = mockCongestionController.get(); + conn->congestionController = std::move(mockCongestionController); + conn->oneRttWriteCipher = test::createNoOpAead(); + + // Create an outstandingPathValidation + limiter so this will be applied. + auto pathValidationLimiter = std::make_unique(); + MockPendingPathRateLimiter* rawLimiter = pathValidationLimiter.get(); + conn->pathValidationLimiter = std::move(pathValidationLimiter); + conn->outstandingPathValidation = PathChallengeFrame(1000); + + // Have stream data queued up during the test so there's something TO write. + auto stream1 = conn->streamManager->createNextBidirectionalStream().value(); + auto buf = IOBuf::copyBuffer("0123456789"); + writeDataToQuicStream(*stream1, buf->clone(), false); + + // shouldWriteData checks this first + const size_t minimumDataSize = std::max( + kLongHeaderHeaderSize + kCipherOverheadHeuristic, sizeof(Sample)); + + // Only case that we allow the write; both CC / PathLimiter have writablebytes + EXPECT_CALL(*rawCongestionController, getWritableBytes()) + .WillOnce(Return(minimumDataSize + 1)); + EXPECT_CALL(*rawLimiter, currentCredit(_, _)) + .WillOnce(Return(minimumDataSize + 1)); + + EXPECT_CALL(*transportInfoCb_, onCwndBlocked()).Times(0); + EXPECT_NE(WriteDataReason::NO_WRITE, shouldWriteData(*conn)); + + // CC has writableBytes, but PathLimiter doesn't. + EXPECT_CALL(*rawCongestionController, getWritableBytes()) + .WillOnce(Return(minimumDataSize + 1)); + EXPECT_CALL(*rawLimiter, currentCredit(_, _)) + .WillOnce(Return(minimumDataSize - 2)); + + EXPECT_CALL(*transportInfoCb_, onCwndBlocked()); + EXPECT_EQ(WriteDataReason::NO_WRITE, shouldWriteData(*conn)); + + // PathLimiter has writableBytes, CC doesn't + EXPECT_CALL(*rawCongestionController, getWritableBytes()) + .WillOnce(Return(minimumDataSize - 1)); + EXPECT_CALL(*rawLimiter, currentCredit(_, _)) + .WillOnce(Return(minimumDataSize + 1)); + + EXPECT_CALL(*transportInfoCb_, onCwndBlocked()); + EXPECT_EQ(WriteDataReason::NO_WRITE, shouldWriteData(*conn)); + + // Neither PathLimiter or CC have writablebytes + EXPECT_CALL(*rawCongestionController, getWritableBytes()) + .WillOnce(Return(minimumDataSize - 1)); + EXPECT_CALL(*rawLimiter, currentCredit(_, _)) + .WillOnce(Return(minimumDataSize - 1)); + + EXPECT_CALL(*transportInfoCb_, onCwndBlocked()); + EXPECT_EQ(WriteDataReason::NO_WRITE, shouldWriteData(*conn)); +} + TEST_F(QuicTransportFunctionsTest, ShouldWriteStreamsNoCipher) { auto conn = createConn(); auto mockCongestionController = std::make_unique(); diff --git a/quic/api/test/QuicTransportTest.cpp b/quic/api/test/QuicTransportTest.cpp index cc7b55edf..76c87c108 100644 --- a/quic/api/test/QuicTransportTest.cpp +++ b/quic/api/test/QuicTransportTest.cpp @@ -935,6 +935,8 @@ TEST_F(QuicTransportTest, SendPathChallenge) { auto& conn = transport_->getConnectionState(); PathChallengeFrame pathChallenge(123); conn.pendingEvents.pathChallenge = pathChallenge; + conn.pathValidationLimiter = + std::make_unique(conn.udpSendPacketLen); transport_->updateWriteLooper(true); EXPECT_FALSE(conn.pendingEvents.schedulePathValidationTimeout); @@ -974,6 +976,8 @@ TEST_F(QuicTransportTest, PathValidationTimeoutExpired) { auto& conn = transport_->getConnectionState(); PathChallengeFrame pathChallenge(123); conn.pendingEvents.pathChallenge = pathChallenge; + conn.pathValidationLimiter = + std::make_unique(conn.udpSendPacketLen); transport_->updateWriteLooper(true); EXPECT_FALSE(conn.pendingEvents.schedulePathValidationTimeout); @@ -1004,6 +1008,8 @@ TEST_F(QuicTransportTest, SendPathValidationWhileThereIsOutstandingOne) { auto& conn = transport_->getConnectionState(); PathChallengeFrame pathChallenge(123); conn.pendingEvents.pathChallenge = pathChallenge; + conn.pathValidationLimiter = + std::make_unique(conn.udpSendPacketLen); transport_->updateWriteLooper(true); loopForWrites(); EXPECT_FALSE(conn.pendingEvents.pathChallenge); @@ -1044,6 +1050,8 @@ TEST_F(QuicTransportTest, ClonePathChallenge) { PathChallengeFrame pathChallenge(123); conn.pendingEvents.pathChallenge = pathChallenge; + conn.pathValidationLimiter = + std::make_unique(conn.udpSendPacketLen); transport_->updateWriteLooper(true); loopForWrites(); @@ -1078,6 +1086,8 @@ TEST_F(QuicTransportTest, OnlyClonePathValidationIfOutstanding) { PathChallengeFrame pathChallenge(123); conn.pendingEvents.pathChallenge = pathChallenge; + conn.pathValidationLimiter = + std::make_unique(conn.udpSendPacketLen); transport_->updateWriteLooper(true); loopForWrites(); @@ -1106,6 +1116,8 @@ TEST_F(QuicTransportTest, ResendPathChallengeOnLoss) { PathChallengeFrame pathChallenge(123); conn.pendingEvents.pathChallenge = pathChallenge; + conn.pathValidationLimiter = + std::make_unique(conn.udpSendPacketLen); transport_->updateWriteLooper(true); loopForWrites(); @@ -1124,6 +1136,8 @@ TEST_F(QuicTransportTest, DoNotResendLostPathChallengeIfNotOutstanding) { auto& conn = transport_->getConnectionState(); PathChallengeFrame pathChallenge(123); + conn.pathValidationLimiter = + std::make_unique(conn.udpSendPacketLen); conn.pendingEvents.pathChallenge = pathChallenge; transport_->updateWriteLooper(true); loopForWrites(); diff --git a/quic/server/state/ServerStateMachine.cpp b/quic/server/state/ServerStateMachine.cpp index 04397fd0e..4d5541015 100644 --- a/quic/server/state/ServerStateMachine.cpp +++ b/quic/server/state/ServerStateMachine.cpp @@ -373,9 +373,11 @@ void onConnectionMigration( folly::Random::secureRandom(&pathData, sizeof(pathData)); conn.pendingEvents.pathChallenge = PathChallengeFrame(pathData); - // Limit amount of bytes that can be sent to unvalidated source - conn.writableBytesLimit = - conn.transportSettings.limitedCwndInMss * conn.udpSendPacketLen; + // If we are already in the middle of a migration reset + // the available bytes in the rate-limited window, but keep the + // window. + conn.pathValidationLimiter = + std::make_unique(conn.udpSendPacketLen); } else { previousPeerAddresses.erase(it); } @@ -791,7 +793,8 @@ void onServerReadDataFromOpen( break; } case QuicWriteFrame::Type::RstStreamFrame_E: { - const RstStreamFrame& frame = *packetFrame.asRstStreamFrame(); + const RstStreamFrame& frame = + *packetFrame.asRstStreamFrame(); VLOG(4) << "Server received ack for reset stream=" << frame.streamId << " " << conn; auto stream = conn.streamManager->getStream(frame.streamId); diff --git a/quic/server/test/QuicServerTransportTest.cpp b/quic/server/test/QuicServerTransportTest.cpp index 6c87e76c3..bff502ba3 100644 --- a/quic/server/test/QuicServerTransportTest.cpp +++ b/quic/server/test/QuicServerTransportTest.cpp @@ -2162,10 +2162,6 @@ TEST_P(QuicServerTransportAllowMigrationTest, MigrateToUnvalidatedPeer) { deliverData(std::move(packetData), false, &newPeer); EXPECT_TRUE(server->getConn().pendingEvents.pathChallenge); - EXPECT_EQ( - *server->getConn().writableBytesLimit, - server->getConn().transportSettings.limitedCwndInMss * - server->getConn().udpSendPacketLen); EXPECT_EQ(server->getConn().peerAddress, newPeer); EXPECT_EQ(server->getConn().migrationState.previousPeerAddresses.size(), 1); EXPECT_EQ( @@ -2195,7 +2191,8 @@ TEST_P(QuicServerTransportAllowMigrationTest, MigrateToUnvalidatedPeer) { EXPECT_TRUE(server->getConn().outstandingPathValidation); EXPECT_TRUE(server->getConn().pendingEvents.schedulePathValidationTimeout); EXPECT_TRUE(server->pathValidationTimeout().isScheduled()); - EXPECT_TRUE(server->getConn().writableBytesLimit); + + EXPECT_TRUE(server->getConn().pathValidationLimiter != nullptr); ShortHeader header( ProtectionType::KeyPhaseZero, @@ -2215,7 +2212,6 @@ TEST_P(QuicServerTransportAllowMigrationTest, MigrateToUnvalidatedPeer) { EXPECT_FALSE(server->getConn().outstandingPathValidation); EXPECT_FALSE(server->getConn().pendingEvents.schedulePathValidationTimeout); EXPECT_FALSE(server->pathValidationTimeout().isScheduled()); - EXPECT_FALSE(server->getConn().writableBytesLimit); } TEST_P(QuicServerTransportAllowMigrationTest, ResetPathRttPathResponse) { @@ -2310,13 +2306,12 @@ TEST_P(QuicServerTransportAllowMigrationTest, IgnoreInvalidPathResponse) { auto peerAddress = server->getConn().peerAddress; folly::SocketAddress newPeer("100.101.102.103", 23456); + deliverData(std::move(packetData), false, &newPeer); EXPECT_TRUE(server->getConn().pendingEvents.pathChallenge); - EXPECT_EQ( - *server->getConn().writableBytesLimit, - server->getConn().transportSettings.limitedCwndInMss * - server->getConn().udpSendPacketLen); + + EXPECT_TRUE(server->getConn().pathValidationLimiter != nullptr); EXPECT_EQ(server->getConn().peerAddress, newPeer); EXPECT_EQ(server->getConn().migrationState.previousPeerAddresses.size(), 1); EXPECT_EQ( @@ -2328,7 +2323,6 @@ TEST_P(QuicServerTransportAllowMigrationTest, IgnoreInvalidPathResponse) { EXPECT_TRUE(server->getConn().outstandingPathValidation); EXPECT_TRUE(server->getConn().pendingEvents.schedulePathValidationTimeout); EXPECT_TRUE(server->pathValidationTimeout().isScheduled()); - EXPECT_TRUE(server->getConn().writableBytesLimit); ShortHeader header( ProtectionType::KeyPhaseZero, @@ -2349,7 +2343,6 @@ TEST_P(QuicServerTransportAllowMigrationTest, IgnoreInvalidPathResponse) { EXPECT_TRUE(server->getConn().outstandingPathValidation); EXPECT_TRUE(server->getConn().pendingEvents.schedulePathValidationTimeout); EXPECT_TRUE(server->pathValidationTimeout().isScheduled()); - EXPECT_TRUE(server->getConn().writableBytesLimit); } TEST_P( @@ -2373,10 +2366,6 @@ TEST_P( deliverData(std::move(packetData), false, &newPeer); EXPECT_TRUE(server->getConn().pendingEvents.pathChallenge); - EXPECT_EQ( - *server->getConn().writableBytesLimit, - server->getConn().transportSettings.limitedCwndInMss * - server->getConn().udpSendPacketLen); EXPECT_EQ(server->getConn().peerAddress, newPeer); EXPECT_EQ(server->getConn().migrationState.previousPeerAddresses.size(), 1); EXPECT_EQ( @@ -2388,7 +2377,8 @@ TEST_P( EXPECT_TRUE(server->getConn().outstandingPathValidation); EXPECT_TRUE(server->getConn().pendingEvents.schedulePathValidationTimeout); EXPECT_TRUE(server->pathValidationTimeout().isScheduled()); - EXPECT_TRUE(server->getConn().writableBytesLimit); + + EXPECT_TRUE(server->getConn().pathValidationLimiter != nullptr); ShortHeader header( ProtectionType::KeyPhaseZero, @@ -2414,7 +2404,7 @@ TEST_P( EXPECT_TRUE(server->getConn().outstandingPathValidation); EXPECT_FALSE(server->getConn().pendingEvents.schedulePathValidationTimeout); EXPECT_FALSE(server->pathValidationTimeout().isScheduled()); - EXPECT_TRUE(server->getConn().writableBytesLimit); + EXPECT_TRUE(server->getConn().localConnectionError); EXPECT_EQ( server->getConn().localConnectionError->second, @@ -2510,7 +2500,6 @@ TEST_P(QuicServerTransportAllowMigrationTest, MigrateToValidatedPeer) { deliverData(std::move(packetData), false, &newPeer); EXPECT_FALSE(server->getConn().pendingEvents.pathChallenge); - EXPECT_FALSE(server->getConn().writableBytesLimit); EXPECT_EQ(server->getConn().peerAddress, newPeer); EXPECT_EQ(server->getConn().migrationState.previousPeerAddresses.size(), 1); EXPECT_EQ( @@ -2578,7 +2567,7 @@ TEST_P( deliverData(std::move(packetData), false, &newPeer2); EXPECT_TRUE(server->getConn().pendingEvents.pathChallenge); - EXPECT_TRUE(server->getConn().writableBytesLimit); + EXPECT_EQ(server->getConn().peerAddress, newPeer2); EXPECT_EQ(server->getConn().migrationState.previousPeerAddresses.size(), 2); EXPECT_EQ( @@ -2645,7 +2634,6 @@ TEST_P(QuicServerTransportAllowMigrationTest, MigrateToStaleValidatedPeer) { deliverData(std::move(packetData), false, &newPeer); EXPECT_FALSE(server->getConn().pendingEvents.pathChallenge); - EXPECT_FALSE(server->getConn().writableBytesLimit); EXPECT_EQ(server->getConn().peerAddress, newPeer); EXPECT_EQ(server->getConn().migrationState.previousPeerAddresses.size(), 1); EXPECT_EQ( @@ -2927,7 +2915,6 @@ TEST_F(QuicServerTransportTest, ClientPortChangeNATRebinding) { deliverData(std::move(packetData), true, &newPeer); EXPECT_TRUE(server->getConn().outstandingPathValidation); - EXPECT_TRUE(server->getConn().writableBytesLimit); EXPECT_EQ(server->getConn().peerAddress, newPeer); EXPECT_EQ(server->getConn().migrationState.previousPeerAddresses.size(), 1); EXPECT_NE( @@ -2960,7 +2947,7 @@ TEST_F(QuicServerTransportTest, ClientAddressChangeNATRebinding) { deliverData(std::move(packetData), true, &newPeer); EXPECT_TRUE(server->getConn().outstandingPathValidation); - EXPECT_TRUE(server->getConn().writableBytesLimit); + EXPECT_EQ(server->getConn().peerAddress, newPeer); EXPECT_EQ(server->getConn().migrationState.previousPeerAddresses.size(), 1); EXPECT_NE(server->getConn().lossState.srtt, 0us); @@ -2992,7 +2979,7 @@ TEST_F( deliverData(std::move(packetData), true, &newPeer); EXPECT_TRUE(server->getConn().outstandingPathValidation); - EXPECT_TRUE(server->getConn().writableBytesLimit); + EXPECT_EQ(server->getConn().peerAddress, newPeer); EXPECT_EQ(server->getConn().migrationState.previousPeerAddresses.size(), 1); EXPECT_EQ( @@ -3020,7 +3007,7 @@ TEST_F( deliverData(std::move(packetData2), true, &newPeer2); EXPECT_TRUE(server->getConn().outstandingPathValidation); - EXPECT_TRUE(server->getConn().writableBytesLimit); + EXPECT_EQ(server->getConn().peerAddress, newPeer2); EXPECT_EQ(server->getConn().migrationState.previousPeerAddresses.size(), 1); EXPECT_EQ( diff --git a/quic/state/PendingPathRateLimiter.h b/quic/state/PendingPathRateLimiter.h index bc01a1e3e..3092c7daa 100644 --- a/quic/state/PendingPathRateLimiter.h +++ b/quic/state/PendingPathRateLimiter.h @@ -18,11 +18,13 @@ class PendingPathRateLimiter { explicit PendingPathRateLimiter(uint64_t udpSendPacketLen) : maxCredit_(kMinCwndInMss * udpSendPacketLen), credit_(maxCredit_) {} - uint64_t currentCredit( + virtual ~PendingPathRateLimiter() = default; + + virtual uint64_t currentCredit( TimePoint checkTime, std::chrono::microseconds rtt) noexcept; - void onPacketSent(uint64_t sentBytes); + virtual void onPacketSent(uint64_t sentBytes); private: const uint64_t maxCredit_; diff --git a/quic/state/SimpleFrameFunctions.cpp b/quic/state/SimpleFrameFunctions.cpp index 5468014ab..e5a56a93e 100644 --- a/quic/state/SimpleFrameFunctions.cpp +++ b/quic/state/SimpleFrameFunctions.cpp @@ -202,7 +202,6 @@ bool updateSimpleFrameOnPacketReceived( // TODO update source token, conn.outstandingPathValidation = folly::none; conn.pendingEvents.schedulePathValidationTimeout = false; - conn.writableBytesLimit = folly::none; // stop the clock to measure init rtt std::chrono::microseconds sampleRtt = diff --git a/quic/state/StateData.cpp b/quic/state/StateData.cpp index 586260688..ebf7e81be 100644 --- a/quic/state/StateData.cpp +++ b/quic/state/StateData.cpp @@ -10,7 +10,6 @@ #include namespace quic { - QuicStreamState::QuicStreamState(StreamId idIn, QuicConnectionStateBase& connIn) : conn(connIn), id(idIn) { // Note: this will set a windowSize for a locally-initiated unidirectional diff --git a/quic/state/StateData.h b/quic/state/StateData.h index 233ba9ed9..98686a408 100644 --- a/quic/state/StateData.h +++ b/quic/state/StateData.h @@ -478,7 +478,7 @@ struct QuicConnectionStateBase { // This limit should be cleared and set back to max after CFIN is received. folly::Optional writableBytesLimit; - std::unique_ptr limiter; + std::unique_ptr pathValidationLimiter; // TODO: We really really should wrap outstandingPackets, all its associated // counters and the outstandingPacketEvents into one class. diff --git a/quic/state/test/Mocks.h b/quic/state/test/Mocks.h index ee31e49fc..eca212d87 100644 --- a/quic/state/test/Mocks.h +++ b/quic/state/test/Mocks.h @@ -42,5 +42,17 @@ class MockPacer : public Pacer { MOCK_METHOD0(onPacketSent, void()); MOCK_METHOD0(onPacketsLoss, void()); }; + +class MockPendingPathRateLimiter : public PendingPathRateLimiter { + public: + MockPendingPathRateLimiter() : PendingPathRateLimiter(0) {} + MOCK_METHOD1(onPacketSent, void(uint64_t)); + GMOCK_METHOD2_( + , + noexcept, + , + currentCredit, + uint64_t(TimePoint, std::chrono::microseconds)); +}; } // namespace test } // namespace quic