From d7d19c74b58460aca42d876a9597e16f68999907 Mon Sep 17 00:00:00 2001 From: Yang Chi Date: Thu, 12 Dec 2019 13:18:39 -0800 Subject: [PATCH] Stop tracking pure ack packets in Quic Summary: Previously we track them since we thought we can get some additional RTT samples. But these are bad RTT samples since peer can delays the acking of pure acks. Now we no longer trust such RTT samples, there is no reason to keep tracking pure ack packets. Reviewed By: mjoras Differential Revision: D18946081 fbshipit-source-id: 0a92d88e709edf8475d67791ba064c3e8b7f627a --- quic/api/QuicPacketScheduler.cpp | 10 +- quic/api/QuicTransportBase.cpp | 1 - quic/api/QuicTransportFunctions.cpp | 43 ++-- quic/api/test/QuicPacketSchedulerTest.cpp | 46 +---- quic/api/test/QuicTransportBaseTest.cpp | 2 - quic/api/test/QuicTransportFunctionsTest.cpp | 62 +++--- quic/api/test/QuicTransportTest.cpp | 24 +-- quic/client/test/QuicClientTransportTest.cpp | 5 +- quic/codec/test/QuicPacketRebuilderTest.cpp | 7 +- quic/common/test/TestUtils.cpp | 3 +- quic/common/test/TestUtils.h | 1 - .../test/BbrBandwidthSamplerTest.cpp | 20 +- quic/congestion_control/test/BbrTest.cpp | 26 +-- quic/congestion_control/test/CopaTest.cpp | 4 +- .../test/CubicHystartTest.cpp | 35 ++-- quic/congestion_control/test/NewRenoTest.cpp | 15 +- quic/loss/QuicLossFunctions.h | 54 ++--- quic/loss/test/QuicLossFunctionsTest.cpp | 189 ++++-------------- quic/server/test/QuicServerTransportTest.cpp | 21 +- quic/state/AckHandlers.cpp | 17 +- quic/state/StateData.h | 8 +- quic/state/test/AckHandlersTest.cpp | 188 +++-------------- quic/state/test/QuicStateFunctionsTest.cpp | 7 +- quic/state/test/StateDataTest.cpp | 7 +- 24 files changed, 208 insertions(+), 587 deletions(-) diff --git a/quic/api/QuicPacketScheduler.cpp b/quic/api/QuicPacketScheduler.cpp index f5ee8e95f..f17942e19 100644 --- a/quic/api/QuicPacketScheduler.cpp +++ b/quic/api/QuicPacketScheduler.cpp @@ -526,9 +526,8 @@ CloningScheduler::CloningScheduler( bool CloningScheduler::hasData() const { return frameScheduler_.hasData() || (!conn_.outstandingPackets.empty() && - (conn_.outstandingPackets.size() != - conn_.outstandingHandshakePacketsCount + - conn_.outstandingPureAckPacketsCount)); + conn_.outstandingPackets.size() != + conn_.outstandingHandshakePacketsCount); } std::pair< @@ -568,9 +567,8 @@ CloningScheduler::scheduleFramesForPacket( getAckState(conn_, builderPnSpace).largestAckedByPeer, conn_.version.value_or(*conn_.originalVersion)); PacketRebuilder rebuilder(regularBuilder, conn_); - // We shouldn't clone Handshake packet. For PureAcks, cloning them bring - // perf down as shown by load test. - if (iter->isHandshake || iter->pureAck) { + // We shouldn't clone Handshake packet. + if (iter->isHandshake) { continue; } // If the packet is already a clone that has been processed, we don't clone diff --git a/quic/api/QuicTransportBase.cpp b/quic/api/QuicTransportBase.cpp index a44019ab6..9f97efeab 100644 --- a/quic/api/QuicTransportBase.cpp +++ b/quic/api/QuicTransportBase.cpp @@ -382,7 +382,6 @@ void QuicTransportBase::closeImpl( // Don't need outstanding packets. conn_->outstandingPackets.clear(); conn_->outstandingHandshakePacketsCount = 0; - conn_->outstandingPureAckPacketsCount = 0; // We don't need no congestion control. conn_->congestionController = nullptr; diff --git a/quic/api/QuicTransportFunctions.cpp b/quic/api/QuicTransportFunctions.cpp index 8804d07fa..4745d9b53 100644 --- a/quic/api/QuicTransportFunctions.cpp +++ b/quic/api/QuicTransportFunctions.cpp @@ -339,16 +339,26 @@ void updateConnection( } } - // TODO: Now pureAck is equivalent to non retransmittable packet. This might - // change in the future. - auto pureAck = !retransmittable; + increaseNextPacketNum(conn, packetNumberSpace); + conn.lossState.largestSent = std::max(conn.lossState.largestSent, packetNum); + // updateConnection may be called multiple times during write. If before or + // during any updateConnection, setLossDetectionAlarm is already set, we + // shouldn't clear it: + if (!conn.pendingEvents.setLossDetectionAlarm) { + conn.pendingEvents.setLossDetectionAlarm = retransmittable; + } + conn.lossState.totalBytesSent += encodedSize; + + if (!retransmittable) { + DCHECK(!packetEvent); + return; + } OutstandingPacket pkt( std::move(packet), std::move(sentTime), encodedSize, isHandshake, - pureAck, - conn.lossState.totalBytesSent + encodedSize); + conn.lossState.totalBytesSent); pkt.isAppLimited = conn.congestionController ? conn.congestionController->isAppLimited() : false; @@ -362,17 +372,12 @@ void updateConnection( } if (packetEvent) { DCHECK(conn.outstandingPacketEvents.count(*packetEvent)); - // CloningScheduler doesn't clone handshake packets or pureAck, and the - // clone result cannot be pureAck either. DCHECK(!isHandshake); - DCHECK(!pureAck); pkt.associatedEvent = std::move(packetEvent); conn.lossState.totalBytesCloned += encodedSize; } - increaseNextPacketNum(conn, packetNumberSpace); - conn.lossState.largestSent = std::max(conn.lossState.largestSent, packetNum); - if (conn.congestionController && !pureAck) { + if (conn.congestionController) { conn.congestionController->onPacketSent(pkt); // An approximation of the app being blocked. The app // technically might not have bytes to write. @@ -386,7 +391,7 @@ void updateConnection( conn.congestionController->getCongestionWindow()); } } - if (conn.pacer && !pureAck) { + if (conn.pacer) { conn.pacer->onPacketSent(); } if (conn.pathValidationLimiter && @@ -397,11 +402,7 @@ void updateConnection( ++conn.outstandingHandshakePacketsCount; conn.lossState.lastHandshakePacketSentTime = pkt.time; } - if (pureAck) { - ++conn.outstandingPureAckPacketsCount; - } else { - conn.lossState.lastRetransmittablePacketSentTime = pkt.time; - } + conn.lossState.lastRetransmittablePacketSentTime = pkt.time; if (pkt.associatedEvent) { CHECK_EQ(packetNumberSpace, PacketNumberSpace::AppData); ++conn.outstandingClonedPacketsCount; @@ -420,16 +421,8 @@ void updateConnection( conn.outstandingPackets.insert(packetIt, std::move(pkt)); auto opCount = conn.outstandingPackets.size(); - DCHECK_GE(opCount, conn.outstandingPureAckPacketsCount); DCHECK_GE(opCount, conn.outstandingHandshakePacketsCount); DCHECK_GE(opCount, conn.outstandingClonedPacketsCount); - // updateConnection may be called multiple times during write. If before or - // during any updateConnection, setLossDetectionAlarm is already set, we - // shouldn't clear it: - if (!conn.pendingEvents.setLossDetectionAlarm) { - conn.pendingEvents.setLossDetectionAlarm = retransmittable; - } - conn.lossState.totalBytesSent += encodedSize; } uint64_t congestionControlWritableBytes(const QuicConnectionStateBase& conn) { diff --git a/quic/api/test/QuicPacketSchedulerTest.cpp b/quic/api/test/QuicPacketSchedulerTest.cpp index f34c71e90..1e576225a 100644 --- a/quic/api/test/QuicPacketSchedulerTest.cpp +++ b/quic/api/test/QuicPacketSchedulerTest.cpp @@ -36,7 +36,7 @@ PacketNum addInitialOutstandingPacket(QuicConnectionStateBase& conn) { nextPacketNum, QuicVersion::QUIC_DRAFT); RegularQuicWritePacket packet(std::move(header)); - conn.outstandingPackets.emplace_back(packet, Clock::now(), 0, true, false, 0); + conn.outstandingPackets.emplace_back(packet, Clock::now(), 0, true, 0); conn.outstandingHandshakePacketsCount++; increaseNextPacketNum(conn, PacketNumberSpace::Handshake); return nextPacketNum; @@ -54,24 +54,12 @@ PacketNum addHandshakeOutstandingPacket(QuicConnectionStateBase& conn) { nextPacketNum, QuicVersion::QUIC_DRAFT); RegularQuicWritePacket packet(std::move(header)); - conn.outstandingPackets.emplace_back(packet, Clock::now(), 0, true, false, 0); + conn.outstandingPackets.emplace_back(packet, Clock::now(), 0, true, 0); conn.outstandingHandshakePacketsCount++; increaseNextPacketNum(conn, PacketNumberSpace::Handshake); return nextPacketNum; } -PacketNum addPureAckOutstandingPacket(QuicConnectionStateBase& conn) { - PacketNum nextPacketNum = getNextPacketNum(conn, PacketNumberSpace::AppData); - ShortHeader header( - ProtectionType::KeyPhaseOne, - conn.clientConnectionId.value_or(quic::test::getTestConnectionId()), - nextPacketNum); - RegularQuicWritePacket packet(std::move(header)); - conn.outstandingPackets.emplace_back(packet, Clock::now(), 0, false, true, 0); - increaseNextPacketNum(conn, PacketNumberSpace::AppData); - return nextPacketNum; -} - PacketNum addOutstandingPacket(QuicConnectionStateBase& conn) { PacketNum nextPacketNum = getNextPacketNum(conn, PacketNumberSpace::AppData); ShortHeader header( @@ -79,8 +67,7 @@ PacketNum addOutstandingPacket(QuicConnectionStateBase& conn) { conn.clientConnectionId.value_or(quic::test::getTestConnectionId()), nextPacketNum); RegularQuicWritePacket packet(std::move(header)); - conn.outstandingPackets.emplace_back( - packet, Clock::now(), 0, false, false, 0); + conn.outstandingPackets.emplace_back(packet, Clock::now(), 0, false, 0); increaseNextPacketNum(conn, PacketNumberSpace::AppData); return nextPacketNum; } @@ -439,33 +426,6 @@ TEST_F(QuicPacketSchedulerTest, DoNotCloneProcessedClonedPacket) { EXPECT_EQ(expected, *result.first); } -TEST_F(QuicPacketSchedulerTest, DoNotClonePureAck) { - QuicClientConnectionState conn( - FizzClientQuicHandshakeContext::Builder().build()); - FrameScheduler noopScheduler("frame"); - CloningScheduler cloningScheduler(noopScheduler, conn, "CopyCat", 0); - // Add two outstanding packets, with second one being pureAck - auto expected = addOutstandingPacket(conn); - // There needs to have retransmittable frame for the rebuilder to work - conn.outstandingPackets.back().packet.frames.push_back( - MaxDataFrame(conn.flowControlState.advertisedMaxOffset)); - addPureAckOutstandingPacket(conn); - conn.outstandingPackets.back().packet.frames.push_back(WriteAckFrame()); - - ShortHeader header( - ProtectionType::KeyPhaseOne, - conn.clientConnectionId.value_or(getTestConnectionId()), - getNextPacketNum(conn, PacketNumberSpace::AppData)); - RegularQuicPacketBuilder builder( - conn.udpSendPacketLen, - std::move(header), - conn.ackStates.appDataAckState.largestAckedByPeer); - auto result = cloningScheduler.scheduleFramesForPacket( - std::move(builder), kDefaultUDPSendPacketLen); - EXPECT_TRUE(result.first.hasValue() && result.second.hasValue()); - EXPECT_EQ(expected, *result.first); -} - TEST_F(QuicPacketSchedulerTest, CloneSchedulerHasDataIgnoresNonAppData) { QuicClientConnectionState conn( FizzClientQuicHandshakeContext::Builder().build()); diff --git a/quic/api/test/QuicTransportBaseTest.cpp b/quic/api/test/QuicTransportBaseTest.cpp index 2da504965..174775b46 100644 --- a/quic/api/test/QuicTransportBaseTest.cpp +++ b/quic/api/test/QuicTransportBaseTest.cpp @@ -1090,10 +1090,8 @@ TEST_F(QuicTransportImplTest, CancelAllDeliveryCallbacksMap) { } TEST_F(QuicTransportImplTest, CloseTransportCleansupOutstandingCounters) { - transport->transportConn->outstandingPureAckPacketsCount = 100; transport->transportConn->outstandingHandshakePacketsCount = 200; transport->closeNow(folly::none); - EXPECT_EQ(0, transport->transportConn->outstandingPureAckPacketsCount); EXPECT_EQ(0, transport->transportConn->outstandingHandshakePacketsCount); } diff --git a/quic/api/test/QuicTransportFunctionsTest.cpp b/quic/api/test/QuicTransportFunctionsTest.cpp index e0eb9c39e..daa99aa43 100644 --- a/quic/api/test/QuicTransportFunctionsTest.cpp +++ b/quic/api/test/QuicTransportFunctionsTest.cpp @@ -351,6 +351,16 @@ TEST_F(QuicTransportFunctionsTest, TestUpdateConnectionPacketSorting) { auto handshakePacket = buildEmptyPacket(*conn, PacketNumberSpace::Handshake); auto appDataPacket = buildEmptyPacket(*conn, PacketNumberSpace::AppData); + auto stream = conn->streamManager->createNextBidirectionalStream().value(); + writeDataToQuicStream( + *stream, + folly::IOBuf::copyBuffer("The sun is cold and the rain is hard."), + true); + WriteStreamFrame writeStreamFrame(stream->id, 0, 5, false); + initialPacket.packet.frames.push_back(writeStreamFrame); + handshakePacket.packet.frames.push_back(writeStreamFrame); + appDataPacket.packet.frames.push_back(writeStreamFrame); + updateConnection( *conn, folly::none, @@ -539,7 +549,6 @@ TEST_F(QuicTransportFunctionsTest, TestUpdateConnectionPureAckCounter) { packet.packet.frames.push_back(std::move(ackFrame)); updateConnection( *conn, folly::none, packet.packet, TimePoint(), getEncodedSize(packet)); - EXPECT_EQ(1, conn->outstandingPureAckPacketsCount); auto nonHandshake = buildEmptyPacket(*conn, PacketNumberSpace::Handshake); packetEncodedSize = @@ -557,7 +566,6 @@ TEST_F(QuicTransportFunctionsTest, TestUpdateConnectionPureAckCounter) { updateConnection( *conn, folly::none, packet2.packet, TimePoint(), getEncodedSize(packet)); - EXPECT_EQ(1, conn->outstandingPureAckPacketsCount); // verify QLogger contains correct packet and frame information std::shared_ptr qLogger = @@ -588,7 +596,6 @@ TEST_F(QuicTransportFunctionsTest, TestPaddingPureAckPacketIsStillPureAck) { packet.packet.frames.push_back(PaddingFrame()); updateConnection( *conn, folly::none, packet.packet, TimePoint(), getEncodedSize(packet)); - EXPECT_EQ(1, conn->outstandingPureAckPacketsCount); // verify QLogger contains correct packet and frames information std::shared_ptr qLogger = @@ -761,9 +768,7 @@ TEST_F(QuicTransportFunctionsTest, TestUpdateConnectionWithPureAck) { EXPECT_CALL(*rawPacer, onPacketSent()).Times(0); updateConnection( *conn, folly::none, packet.packet, TimePoint(), getEncodedSize(packet)); - EXPECT_EQ(1, conn->outstandingPackets.size()); - EXPECT_TRUE( - getFirstOutstandingPacket(*conn, PacketNumberSpace::Handshake)->pureAck); + EXPECT_EQ(0, conn->outstandingPackets.size()); EXPECT_EQ(0, conn->lossState.totalBytesAcked); std::shared_ptr qLogger = std::dynamic_pointer_cast(conn->qLogger); @@ -785,7 +790,13 @@ TEST_F(QuicTransportFunctionsTest, TestUpdateConnectionWithPureAck) { TEST_F(QuicTransportFunctionsTest, TestUpdateConnectionWithBytesStats) { auto conn = createConn(); conn->qLogger = std::make_shared(VantagePoint::CLIENT); + auto stream = conn->streamManager->createNextBidirectionalStream().value(); auto packet = buildEmptyPacket(*conn, PacketNumberSpace::Handshake); + // This is clearly not 555 bytes. I just need some data inside the packet. + writeDataToQuicStream( + *stream, folly::IOBuf::copyBuffer("Im gonna cut your hair."), true); + WriteStreamFrame writeStreamFrame(stream->id, 0, 5, false); + packet.packet.frames.push_back(std::move(writeStreamFrame)); conn->lossState.totalBytesSent = 13579; conn->lossState.totalBytesAcked = 8642; auto currentTime = Clock::now(); @@ -1585,18 +1596,18 @@ TEST_F(QuicTransportFunctionsTest, WritePureAckWhenNoWritableBytes) { return iobuf->computeChainDataLength(); })); EXPECT_CALL(*rawCongestionController, onPacketSent(_)).Times(0); - writeQuicDataToSocket( - *rawSocket, - *conn, - *conn->clientConnectionId, - *conn->serverConnectionId, - *aead, - *headerCipher, - getVersion(*conn), - conn->transportSettings.writeConnectionDataPacketsLimit); - EXPECT_EQ(1, conn->outstandingPackets.size()); - auto packet = *getFirstOutstandingPacket(*conn, PacketNumberSpace::AppData); - EXPECT_TRUE(packet.pureAck); + EXPECT_GT( + writeQuicDataToSocket( + *rawSocket, + *conn, + *conn->clientConnectionId, + *conn->serverConnectionId, + *aead, + *headerCipher, + getVersion(*conn), + conn->transportSettings.writeConnectionDataPacketsLimit), + 0); + EXPECT_EQ(0, conn->outstandingPackets.size()); } TEST_F(QuicTransportFunctionsTest, ShouldWriteDataTest) { @@ -1872,10 +1883,7 @@ TEST_F(QuicTransportFunctionsTest, ClearBlockedFromPendingEvents) { *conn, folly::none, packet.packet, TimePoint(), getEncodedSize(packet)); EXPECT_FALSE(conn->streamManager->hasBlocked()); EXPECT_FALSE(conn->outstandingPackets.empty()); - EXPECT_EQ(0, conn->outstandingPureAckPacketsCount); EXPECT_EQ(0, conn->outstandingClonedPacketsCount); - EXPECT_FALSE( - getLastOutstandingPacket(*conn, PacketNumberSpace::Handshake)->pureAck); } TEST_F(QuicTransportFunctionsTest, ClonedBlocked) { @@ -1890,10 +1898,7 @@ TEST_F(QuicTransportFunctionsTest, ClonedBlocked) { updateConnection( *conn, packetEvent, packet.packet, TimePoint(), getEncodedSize(packet)); EXPECT_FALSE(conn->outstandingPackets.empty()); - EXPECT_EQ(0, conn->outstandingPureAckPacketsCount); EXPECT_EQ(1, conn->outstandingClonedPacketsCount); - EXPECT_FALSE( - getLastOutstandingPacket(*conn, PacketNumberSpace::AppData)->pureAck); } TEST_F(QuicTransportFunctionsTest, TwoConnWindowUpdateWillCrash) { @@ -1925,9 +1930,6 @@ TEST_F(QuicTransportFunctionsTest, WriteStreamFrameIsNotPureAck) { updateConnection( *conn, folly::none, packet.packet, TimePoint(), getEncodedSize(packet)); EXPECT_FALSE(conn->outstandingPackets.empty()); - EXPECT_EQ(0, conn->outstandingPureAckPacketsCount); - EXPECT_FALSE( - getLastOutstandingPacket(*conn, PacketNumberSpace::Handshake)->pureAck); } TEST_F(QuicTransportFunctionsTest, ClearRstFromPendingEvents) { @@ -1942,10 +1944,7 @@ TEST_F(QuicTransportFunctionsTest, ClearRstFromPendingEvents) { *conn, folly::none, packet.packet, TimePoint(), getEncodedSize(packet)); EXPECT_TRUE(conn->pendingEvents.resets.empty()); EXPECT_FALSE(conn->outstandingPackets.empty()); - EXPECT_EQ(0, conn->outstandingPureAckPacketsCount); EXPECT_EQ(0, conn->outstandingClonedPacketsCount); - EXPECT_FALSE( - getLastOutstandingPacket(*conn, PacketNumberSpace::Handshake)->pureAck); } TEST_F(QuicTransportFunctionsTest, ClonedRst) { @@ -1961,10 +1960,7 @@ TEST_F(QuicTransportFunctionsTest, ClonedRst) { updateConnection( *conn, packetEvent, packet.packet, TimePoint(), getEncodedSize(packet)); EXPECT_FALSE(conn->outstandingPackets.empty()); - EXPECT_EQ(0, conn->outstandingPureAckPacketsCount); EXPECT_EQ(1, conn->outstandingClonedPacketsCount); - EXPECT_FALSE( - getLastOutstandingPacket(*conn, PacketNumberSpace::AppData)->pureAck); } TEST_F(QuicTransportFunctionsTest, TotalBytesSentUpdate) { diff --git a/quic/api/test/QuicTransportTest.cpp b/quic/api/test/QuicTransportTest.cpp index 02da75d13..64a65fe14 100644 --- a/quic/api/test/QuicTransportTest.cpp +++ b/quic/api/test/QuicTransportTest.cpp @@ -784,24 +784,7 @@ TEST_F(QuicTransportTest, WriteImmediateAcks) { *headerCipher_, transport_->getVersion(), conn.transportSettings.writeConnectionDataPacketsLimit); - EXPECT_EQ(conn.outstandingPackets.size(), 1); - auto& packet = - getFirstOutstandingPacket(conn, PacketNumberSpace::AppData)->packet; - EXPECT_GE(packet.frames.size(), 1); - - bool ackFound = false; - for (auto& frame : packet.frames) { - auto ackFrame = frame.asWriteAckFrame(); - if (!ackFrame) { - continue; - } - EXPECT_EQ(ackFrame->ackBlocks.size(), 1); - EXPECT_EQ(start, ackFrame->ackBlocks.front().start); - EXPECT_EQ(end, ackFrame->ackBlocks.front().end); - ackFound = true; - } - EXPECT_TRUE(ackFound); - + EXPECT_TRUE(conn.outstandingPackets.empty()); EXPECT_EQ(conn.ackStates.appDataAckState.largestAckScheduled, end); EXPECT_FALSE(conn.ackStates.appDataAckState.needsToSendAckImmediately); EXPECT_EQ(0, conn.ackStates.appDataAckState.numNonRxPacketsRecvd); @@ -1044,7 +1027,6 @@ TEST_F(QuicTransportTest, ClonePathChallenge) { auto& conn = transport_->getConnectionState(); // knock every handshake outstanding packets out conn.outstandingHandshakePacketsCount = 0; - conn.outstandingPureAckPacketsCount = 0; conn.outstandingPackets.clear(); conn.lossState.initialLossTime.clear(); conn.lossState.handshakeLossTime.clear(); @@ -1080,7 +1062,6 @@ TEST_F(QuicTransportTest, OnlyClonePathValidationIfOutstanding) { auto& conn = transport_->getConnectionState(); // knock every handshake outstanding packets out conn.outstandingHandshakePacketsCount = 0; - conn.outstandingPureAckPacketsCount = 0; conn.outstandingPackets.clear(); conn.lossState.initialLossTime.clear(); conn.lossState.handshakeLossTime.clear(); @@ -1223,7 +1204,6 @@ TEST_F(QuicTransportTest, ClonePathResponse) { auto& conn = transport_->getConnectionState(); // knock every handshake outstanding packets out conn.outstandingHandshakePacketsCount = 0; - conn.outstandingPureAckPacketsCount = 0; conn.outstandingPackets.clear(); conn.lossState.initialLossTime.clear(); conn.lossState.handshakeLossTime.clear(); @@ -1307,7 +1287,6 @@ TEST_F(QuicTransportTest, CloneNewConnectionIdFrame) { auto& conn = transport_->getConnectionState(); // knock every handshake outstanding packets out conn.outstandingHandshakePacketsCount = 0; - conn.outstandingPureAckPacketsCount = 0; conn.outstandingPackets.clear(); conn.lossState.initialLossTime.clear(); conn.lossState.handshakeLossTime.clear(); @@ -1446,7 +1425,6 @@ TEST_F(QuicTransportTest, CloneRetireConnectionIdFrame) { auto& conn = transport_->getConnectionState(); // knock every handshake outstanding packets out conn.outstandingHandshakePacketsCount = 0; - conn.outstandingPureAckPacketsCount = 0; conn.outstandingPackets.clear(); conn.lossState.initialLossTime.clear(); conn.lossState.handshakeLossTime.clear(); diff --git a/quic/client/test/QuicClientTransportTest.cpp b/quic/client/test/QuicClientTransportTest.cpp index 0b727a50d..a628c38a1 100644 --- a/quic/client/test/QuicClientTransportTest.cpp +++ b/quic/client/test/QuicClientTransportTest.cpp @@ -3738,9 +3738,8 @@ TEST_F(QuicClientTransportAfterStartTest, IdleTimerResetNoOutstandingPackets) { // Clear out all the outstanding packets to simulate quiescent state. client->getNonConstConn().receivedNewPacketBeforeWrite = false; client->getNonConstConn().outstandingPackets.clear(); - client->getNonConstConn().outstandingPureAckPacketsCount = - client->getNonConstConn().outstandingHandshakePacketsCount = - client->getNonConstConn().outstandingClonedPacketsCount = 0; + client->getNonConstConn().outstandingHandshakePacketsCount = 0; + client->getNonConstConn().outstandingClonedPacketsCount = 0; client->idleTimeout().cancelTimeout(); auto streamId = client->createBidirectionalStream().value(); auto expected = folly::IOBuf::copyBuffer("hello"); diff --git a/quic/codec/test/QuicPacketRebuilderTest.cpp b/quic/codec/test/QuicPacketRebuilderTest.cpp index 18f8c04df..f907386e1 100644 --- a/quic/codec/test/QuicPacketRebuilderTest.cpp +++ b/quic/codec/test/QuicPacketRebuilderTest.cpp @@ -27,12 +27,7 @@ OutstandingPacket makeDummyOutstandingPacket( const RegularQuicWritePacket& writePacket, uint64_t totalBytesSentOnConnection) { OutstandingPacket packet( - writePacket, - Clock::now(), - 1000, - false, - false, - totalBytesSentOnConnection); + writePacket, Clock::now(), 1000, false, totalBytesSentOnConnection); return packet; } diff --git a/quic/common/test/TestUtils.cpp b/quic/common/test/TestUtils.cpp index 9510ddeb9..0bfdbda99 100644 --- a/quic/common/test/TestUtils.cpp +++ b/quic/common/test/TestUtils.cpp @@ -522,7 +522,6 @@ OutstandingPacket makeTestingWritePacket( PacketNum desiredPacketSeqNum, size_t desiredSize, uint64_t totalBytesSent, - bool pureAck, TimePoint sentTime) { LongHeader longHeader( LongHeader::Types::ZeroRtt, @@ -532,7 +531,7 @@ OutstandingPacket makeTestingWritePacket( QuicVersion::MVFST); RegularQuicWritePacket packet(std::move(longHeader)); return OutstandingPacket( - packet, sentTime, desiredSize, false, pureAck, totalBytesSent); + packet, sentTime, desiredSize, false, totalBytesSent); } CongestionController::AckEvent makeAck( diff --git a/quic/common/test/TestUtils.h b/quic/common/test/TestUtils.h index 5ab2ea9b7..9cdb6f580 100644 --- a/quic/common/test/TestUtils.h +++ b/quic/common/test/TestUtils.h @@ -214,7 +214,6 @@ OutstandingPacket makeTestingWritePacket( PacketNum desiredPacketSeqNum, size_t desiredSize, uint64_t totalBytesSent, - bool pureAck = false, TimePoint sentTime = Clock::now()); // TODO: The way we setup packet sent, ack, loss in test cases can use some diff --git a/quic/congestion_control/test/BbrBandwidthSamplerTest.cpp b/quic/congestion_control/test/BbrBandwidthSamplerTest.cpp index 6bc2b4bf7..f250e0e7a 100644 --- a/quic/congestion_control/test/BbrBandwidthSamplerTest.cpp +++ b/quic/congestion_control/test/BbrBandwidthSamplerTest.cpp @@ -34,7 +34,7 @@ TEST_F(BbrBandwidthSamplerTest, NoPreviousAckedPacket) { CongestionController::AckEvent ackEvent; ackEvent.ackedBytes = 5000; ackEvent.ackedPackets.push_back(makeAckPacketFromOutstandingPacket( - makeTestingWritePacket(0, 1000, 1000, false))); + makeTestingWritePacket(0, 1000, 1000))); sampler.onPacketAcked(ackEvent, 0); EXPECT_EQ(0, sampler.getBandwidth().units); } @@ -46,7 +46,7 @@ TEST_F(BbrBandwidthSamplerTest, NoPreviousAckedPacketFallback) { CongestionController::AckEvent ackEvent; ackEvent.ackedBytes = 5000; ackEvent.ackedPackets.push_back(makeAckPacketFromOutstandingPacket( - makeTestingWritePacket(0, 1000, 1000, false, sentTime))); + makeTestingWritePacket(0, 1000, 1000, sentTime))); ackEvent.ackTime = sentTime + 50ms; sampler.onPacketAcked(ackEvent, 0); EXPECT_EQ(1000, sampler.getBandwidth().units); @@ -63,7 +63,7 @@ TEST_F(BbrBandwidthSamplerTest, RateCalculation) { auto lastAckedPacketSentTime = ackTime - 200us; auto lastAckedPacketAckTime = ackTime - 100us; for (PacketNum pn = 0; pn < 5; pn++) { - auto packet = makeTestingWritePacket(pn, 1000, 1000 + 1000 * pn, false); + auto packet = makeTestingWritePacket(pn, 1000, 1000 + 1000 * pn); packet.lastAckedPacketInfo.emplace( lastAckedPacketSentTime, lastAckedPacketAckTime, 0, 0); packet.time = ackTime - 50us; @@ -86,7 +86,7 @@ TEST_F(BbrBandwidthSamplerTest, SampleExpiration) { auto lastAckedPacketSentTime = ackTime - 200us; auto lastAckedPacketAckTime = ackTime - 100us; PacketNum pn = 1; - auto packet = makeTestingWritePacket(pn, 1000, 2000, false); + auto packet = makeTestingWritePacket(pn, 1000, 2000); packet.lastAckedPacketInfo.emplace( lastAckedPacketSentTime, lastAckedPacketAckTime, 1000, 1000); packet.time = ackTime - 50us; @@ -96,7 +96,7 @@ TEST_F(BbrBandwidthSamplerTest, SampleExpiration) { pn++; conn_.lossState.totalBytesAcked = 2000; - auto packet2 = makeTestingWritePacket(pn, 500, 2500, false); + auto packet2 = makeTestingWritePacket(pn, 500, 2500); packet2.lastAckedPacketInfo.emplace(packet.time, ackTime, 2000, 1000); auto ackTime2 = ackTime + 150us; CongestionController::AckEvent ackEvent2; @@ -109,7 +109,7 @@ TEST_F(BbrBandwidthSamplerTest, SampleExpiration) { pn++; conn_.lossState.totalBytesAcked = 2500; - auto packet3 = makeTestingWritePacket(pn, 200, 2700, false); + auto packet3 = makeTestingWritePacket(pn, 200, 2700); packet3.lastAckedPacketInfo.emplace(packet2.time, ackTime2, 2500, 2000); auto ackTime3 = ackTime + 250us; CongestionController::AckEvent ackEvent3; @@ -121,7 +121,7 @@ TEST_F(BbrBandwidthSamplerTest, SampleExpiration) { pn++; conn_.lossState.totalBytesAcked = 2700; - auto packet4 = makeTestingWritePacket(pn, 100, 2800, false); + auto packet4 = makeTestingWritePacket(pn, 100, 2800); packet4.lastAckedPacketInfo.emplace(packet3.time, ackTime3, 2700, 2500); auto ackTime4 = ackTime + 350us; CongestionController::AckEvent ackEvent4; @@ -146,7 +146,7 @@ TEST_F(BbrBandwidthSamplerTest, AppLimited) { CongestionController::AckEvent ackEvent; ackEvent.largestAckedPacket = ++conn.lossState.largestSent; auto packet = - makeTestingWritePacket(*ackEvent.largestAckedPacket, 1000, 1000, false); + makeTestingWritePacket(*ackEvent.largestAckedPacket, 1000, 1000); ackEvent.largestAckedPacketSentTime = packet.time; ackEvent.ackedPackets.push_back( makeAckPacketFromOutstandingPacket(std::move(packet))); @@ -175,7 +175,7 @@ TEST_F(BbrBandwidthSamplerTest, AppLimitedOutstandingPacket) { auto lastAckedPacketSentTime = ackTime - 200us; auto lastAckedPacketAckTime = ackTime - 100us; PacketNum pn = 0; - auto packet = makeTestingWritePacket(pn, 1000, 1000 + 1000 * pn, false); + auto packet = makeTestingWritePacket(pn, 1000, 1000 + 1000 * pn); packet.isAppLimited = true; packet.lastAckedPacketInfo.emplace( lastAckedPacketSentTime, lastAckedPacketAckTime, 0, 0); @@ -187,7 +187,7 @@ TEST_F(BbrBandwidthSamplerTest, AppLimitedOutstandingPacket) { auto bandwidth = sampler.getBandwidth(); pn++; - auto packet1 = makeTestingWritePacket(pn, 1000, 1000 + 1000 * pn, false); + auto packet1 = makeTestingWritePacket(pn, 1000, 1000 + 1000 * pn); packet1.isAppLimited = true; packet1.lastAckedPacketInfo.emplace(packet.time, ackTime, 1000, 0); packet1.time = ackTime + 500us; diff --git a/quic/congestion_control/test/BbrTest.cpp b/quic/congestion_control/test/BbrTest.cpp index efb7be3a6..799d4c34c 100644 --- a/quic/congestion_control/test/BbrTest.cpp +++ b/quic/congestion_control/test/BbrTest.cpp @@ -130,7 +130,7 @@ TEST_F(BbrTest, StartupCwnd) { bbr.setRttSampler(std::move(mockRttSampler)); bbr.setBandwidthSampler(std::move(mockBandwidthSampler)); - auto packet = makeTestingWritePacket(0, 3000, 3000, false); + auto packet = makeTestingWritePacket(0, 3000, 3000); bbr.onPacketSent(packet); auto startingCwnd = bbr.getCongestionWindow(); conn.lossState.srtt = 100us; @@ -162,7 +162,7 @@ TEST_F(BbrTest, LeaveStartup) { auto sendAckGrow = [&](bool growFast) { conn.lossState.largestSent = currentLatest; auto packet = makeTestingWritePacket( - conn.lossState.largestSent, 1000, 1000 + totalSent, false); + conn.lossState.largestSent, 1000, 1000 + totalSent); bbr.onPacketSent(packet); totalSent += 1000; EXPECT_CALL(*rawBandwidthSampler, getBandwidth()) @@ -206,7 +206,7 @@ TEST_F(BbrTest, RemoveInflightBytes) { conn.udpSendPacketLen = 1000; BbrCongestionController bbr(conn); auto writableBytesAfterInit = bbr.getWritableBytes(); - bbr.onPacketSent(makeTestingWritePacket(0, 1000, 1000, 0)); + bbr.onPacketSent(makeTestingWritePacket(0, 1000, 1000)); EXPECT_EQ(writableBytesAfterInit - 1000, bbr.getWritableBytes()); bbr.onRemoveBytesFromInflight(1000); EXPECT_EQ(writableBytesAfterInit, bbr.getWritableBytes()); @@ -233,8 +233,7 @@ TEST_F(BbrTest, ProbeRtt) { auto packet = makeTestingWritePacket( conn.lossState.largestSent, conn.udpSendPacketLen, - totalSent + conn.udpSendPacketLen, - false); + totalSent + conn.udpSendPacketLen); bbr.onPacketSent(packet); inflightPackets.push_back(std::make_pair(currentLatest, packet.time)); inflightBytes += conn.udpSendPacketLen; @@ -397,7 +396,7 @@ TEST_F(BbrTest, AckAggregation) { auto sendAckGrow = [&](bool growFast) { conn.lossState.largestSent = currentLatest; auto packet = makeTestingWritePacket( - conn.lossState.largestSent, 1000, 1000 + totalSent, false); + conn.lossState.largestSent, 1000, 1000 + totalSent); bbr.onPacketSent(packet); totalSent += 1000; EXPECT_CALL(*rawBandwidthSampler, getBandwidth()) @@ -438,7 +437,7 @@ TEST_F(BbrTest, AckAggregation) { conn.lossState.largestSent = currentLatest; auto packet = makeTestingWritePacket( - conn.lossState.largestSent, 1000, 1000 + totalSent, false); + conn.lossState.largestSent, 1000, 1000 + totalSent); bbr.onPacketSent(packet); totalSent += 1000; auto ackEvent = makeAck(currentLatest, 1000, Clock::now(), packet.time); @@ -463,8 +462,7 @@ TEST_F(BbrTest, AckAggregation) { auto packet1 = makeTestingWritePacket( conn.lossState.largestSent, currentMaxAckHeight * 2 + 100, - currentMaxAckHeight * 2 + 100 + totalSent, - false); + currentMaxAckHeight * 2 + 100 + totalSent); bbr.onPacketSent(packet1); totalSent += (currentMaxAckHeight * 2 + 100); auto ackEvent2 = makeAck( @@ -483,8 +481,7 @@ TEST_F(BbrTest, AppLimited) { auto rawBandwidthSampler = mockBandwidthSampler.get(); bbr.setBandwidthSampler(std::move(mockBandwidthSampler)); - auto packet = - makeTestingWritePacket(conn.lossState.largestSent, 1000, 1000, false); + auto packet = makeTestingWritePacket(conn.lossState.largestSent, 1000, 1000); bbr.onPacketSent(packet); conn.lossState.largestSent++; EXPECT_CALL(*rawBandwidthSampler, onAppLimited()).Times(1); @@ -507,7 +504,7 @@ TEST_F(BbrTest, AppLimitedIgnored) { EXPECT_CALL(*rawBandwidthSampler, onAppLimited()).Times(1); bbr.setAppLimited(); auto packet = makeTestingWritePacket( - conn.lossState.largestSent++, 1000, 1000 + inflightBytes, 0); + conn.lossState.largestSent++, 1000, 1000 + inflightBytes); inflightBytes += 1000; bbr.onPacketSent(packet); } @@ -529,8 +526,7 @@ TEST_F(BbrTest, ExtendMinRttExpiration) { EXPECT_CALL(*rawBandwidthSampler, onAppLimited()).Times(1); bbr.setAppLimited(); - auto packet = - makeTestingWritePacket(conn.lossState.largestSent, 1000, 1000, 0); + auto packet = makeTestingWritePacket(conn.lossState.largestSent, 1000, 1000); bbr.onPacketSent(packet); EXPECT_CALL(*rawRttSampler, timestampMinRtt(_)).Times(1); bbr.onPacketAckOrLoss( @@ -544,7 +540,7 @@ TEST_F(BbrTest, BytesCounting) { bbr.setBandwidthSampler(std::make_unique(conn)); PacketNum packetNum = 0; - auto packet = makeTestingWritePacket(packetNum, 1200, 1200, false); + auto packet = makeTestingWritePacket(packetNum, 1200, 1200); conn.outstandingPackets.push_back(packet); ReadAckFrame ackFrame; ackFrame.largestAcked = packetNum; diff --git a/quic/congestion_control/test/CopaTest.cpp b/quic/congestion_control/test/CopaTest.cpp index 30b4841ce..4e50ecdb1 100644 --- a/quic/congestion_control/test/CopaTest.cpp +++ b/quic/congestion_control/test/CopaTest.cpp @@ -29,7 +29,7 @@ class CopaTest : public Test { ShortHeader(ProtectionType::KeyPhaseZero, connId, packetData.first)); totalSentBytes += 10; loss.addLostPacket(OutstandingPacket( - std::move(packet), Clock::now(), 10, false, false, totalSentBytes)); + std::move(packet), Clock::now(), 10, false, totalSentBytes)); loss.lostBytes = packetData.second; } loss.lostPackets = lostPackets.size(); @@ -42,7 +42,7 @@ class CopaTest : public Test { RegularQuicWritePacket packet( ShortHeader(ProtectionType::KeyPhaseZero, connId, packetNum)); return OutstandingPacket( - std::move(packet), Clock::now(), size, false, false, totalSent); + std::move(packet), Clock::now(), size, false, totalSent); } CongestionController::AckEvent createAckEvent( diff --git a/quic/congestion_control/test/CubicHystartTest.cpp b/quic/congestion_control/test/CubicHystartTest.cpp index ac1220c50..4a9c381b2 100644 --- a/quic/congestion_control/test/CubicHystartTest.cpp +++ b/quic/congestion_control/test/CubicHystartTest.cpp @@ -83,19 +83,11 @@ TEST_F(CubicHystartTest, AckTrain) { // Packet 0 is sent: auto packetSize = kLowSsthreshInMss * conn.udpSendPacketLen; auto packet0 = makeTestingWritePacket( - 0, - packetSize, - packetSize, - false, - realNow - std::chrono::milliseconds(10)); + 0, packetSize, packetSize, realNow - std::chrono::milliseconds(10)); cubic.onPacketSent(packet0); // Packet 1 is sent: auto packet1 = makeTestingWritePacket( - 1, - packetSize, - packetSize * 2, - false, - realNow - std::chrono::milliseconds(5)); + 1, packetSize, packetSize * 2, realNow - std::chrono::milliseconds(5)); cubic.onPacketSent(packet1); // Packet 0 is acked: cubic.onPacketAckOrLoss( @@ -128,7 +120,7 @@ TEST_F(CubicHystartTest, NoAckTrainNoDelayIncrease) { conn.lossState.srtt = 2us; auto realNow = quic::Clock::now(); // One onPacketAcked will not trigger DelayIncrease - auto packet = makeTestingWritePacket(0, 1000, 1000, false, realNow); + auto packet = makeTestingWritePacket(0, 1000, 1000, realNow); cubic.onPacketSent(packet); cubic.onPacketAckOrLoss( makeAck(0, 1000, realNow + kAckCountingGap + 2us, packet.time), @@ -157,14 +149,14 @@ TEST_F(CubicHystartTest, DelayIncrease) { auto packetsSentTime = Clock::now(); auto firstPacketNum = packetNum++; auto packet0 = makeTestingWritePacket( - packetNum, 1000, totalSent + 1000, false, packetsSentTime); + packetNum, 1000, totalSent + 1000, packetsSentTime); totalSent += 1000; conn.lossState.largestSent = firstPacketNum; cubic.onPacketSent(packet0); auto secondPacketNum = packetNum++; auto packet1 = makeTestingWritePacket( - secondPacketNum, 1000, totalSent + 1000, false, packetsSentTime); + secondPacketNum, 1000, totalSent + 1000, packetsSentTime); totalSent += 1000; // Packet 1 is sent: conn.lossState.largestSent = secondPacketNum; @@ -185,7 +177,7 @@ TEST_F(CubicHystartTest, DelayIncrease) { auto estimatedRttEndTarget = Clock::now(); auto packet2 = makeTestingWritePacket( - packetNum, 1000, totalSent + 1000, false, estimatedRttEndTarget + 1us); + packetNum, 1000, totalSent + 1000, estimatedRttEndTarget + 1us); totalSent += 1000; conn.lossState.largestSent = packetNum; cubic.onPacketSent(packet2); @@ -203,7 +195,7 @@ TEST_F(CubicHystartTest, DelayIncrease) { for (size_t i = 0; i < kAckSampling - 1; i++) { conn.lossState.largestSent = i + packetNum; auto packet = makeTestingWritePacket( - i + packetNum, 1000, totalSent + 1000, false, packetsSentTime); + i + packetNum, 1000, totalSent + 1000, packetsSentTime); cubic.onPacketSent(packet); totalSent += 1000; ackTime += ackTimeIncrease; @@ -217,7 +209,7 @@ TEST_F(CubicHystartTest, DelayIncrease) { // DelayIncrease: conn.lossState.largestSent = packetNum; auto packetEnd = makeTestingWritePacket( - packetNum, 1000, totalSent + 1000, false, packetsSentTime); + packetNum, 1000, totalSent + 1000, packetsSentTime); cubic.onPacketSent(packetEnd); totalSent += 1000; ackTime += ackTimeIncrease; @@ -234,13 +226,13 @@ TEST_F(CubicHystartTest, DelayIncreaseCwndTooSmall) { auto realNow = quic::Clock::now(); uint64_t totalSent = 0; - auto packet0 = makeTestingWritePacket(0, 1, totalSent + 1, false, realNow); + auto packet0 = makeTestingWritePacket(0, 1, totalSent + 1, realNow); // Packet 0 is sent: conn.lossState.largestSent = 0; cubic.onPacketSent(packet0); totalSent += 1; - auto packet1 = makeTestingWritePacket(1, 1, totalSent + 1, false, realNow); + auto packet1 = makeTestingWritePacket(1, 1, totalSent + 1, realNow); conn.lossState.largestSent = 1; cubic.onPacketSent(packet1); totalSent += 1; @@ -255,7 +247,7 @@ TEST_F(CubicHystartTest, DelayIncreaseCwndTooSmall) { ackTime += ackTimeIncrease; cubic.onPacketAckOrLoss(makeAck(1, 1, ackTime, packet1.time), folly::none); - auto packet2 = makeTestingWritePacket(2, 10, 10 + totalSent, false, realNow); + auto packet2 = makeTestingWritePacket(2, 10, 10 + totalSent, realNow); conn.lossState.largestSent = 2; cubic.onPacketSent(packet2); totalSent += 10; @@ -269,8 +261,7 @@ TEST_F(CubicHystartTest, DelayIncreaseCwndTooSmall) { std::vector moreAcks; for (size_t i = 0; i < kAckSampling - 1; i++) { conn.lossState.largestSent = i + 3; - auto packet = - makeTestingWritePacket(i + 3, 1, 1 + totalSent, false, realNow); + auto packet = makeTestingWritePacket(i + 3, 1, 1 + totalSent, realNow); ackTime += ackTimeIncrease; moreAcks.push_back(makeAck(i + 3, 1, ackTime, packet.time)); cubic.onPacketSent(packet); @@ -283,7 +274,7 @@ TEST_F(CubicHystartTest, DelayIncreaseCwndTooSmall) { // DelayIncrease: conn.lossState.largestSent = kAckSampling; auto packetEnd = - makeTestingWritePacket(kAckSampling, 1, 1 + totalSent, false, realNow); + makeTestingWritePacket(kAckSampling, 1, 1 + totalSent, realNow); cubic.onPacketSent(packetEnd); totalSent += 1; ackTime += ackTimeIncrease; diff --git a/quic/congestion_control/test/NewRenoTest.cpp b/quic/congestion_control/test/NewRenoTest.cpp index 59743aa19..5cb027874 100644 --- a/quic/congestion_control/test/NewRenoTest.cpp +++ b/quic/congestion_control/test/NewRenoTest.cpp @@ -25,8 +25,8 @@ CongestionController::LossEvent createLossEvent( for (auto packetData : lostPackets) { RegularQuicWritePacket packet( ShortHeader(ProtectionType::KeyPhaseZero, connId, packetData.first)); - loss.addLostPacket(OutstandingPacket( - std::move(packet), Clock::now(), 10, false, false, 10)); + loss.addLostPacket( + OutstandingPacket(std::move(packet), Clock::now(), 10, false, 10)); loss.lostBytes = packetData.second; } loss.lostPackets = lostPackets.size(); @@ -45,12 +45,7 @@ CongestionController::AckEvent createAckEvent( ack.ackedBytes = ackedSize; ack.ackedPackets.push_back( makeAckPacketFromOutstandingPacket(OutstandingPacket( - std::move(packet), - packetSentTime, - ackedSize, - false, - false, - ackedSize))); + std::move(packet), packetSentTime, ackedSize, false, ackedSize))); return ack; } @@ -59,8 +54,7 @@ createPacket(PacketNum packetNum, uint32_t size, TimePoint sendTime) { auto connId = getTestConnectionId(); RegularQuicWritePacket packet( ShortHeader(ProtectionType::KeyPhaseZero, connId, packetNum)); - return OutstandingPacket( - std::move(packet), sendTime, size, false, false, size); + return OutstandingPacket(std::move(packet), sendTime, size, false, size); } TEST_F(NewRenoTest, TestLoss) { @@ -144,7 +138,6 @@ TEST_F(NewRenoTest, TestSteadyStateAck) { NewReno reno(conn); EXPECT_TRUE(reno.inSlowStart()); - conn.lossState.largestSent = 5; auto originalWritableBytes = reno.getWritableBytes(); PacketNum loss1 = 4; diff --git a/quic/loss/QuicLossFunctions.h b/quic/loss/QuicLossFunctions.h index b6fac1cad..5f53282c7 100644 --- a/quic/loss/QuicLossFunctions.h +++ b/quic/loss/QuicLossFunctions.h @@ -128,8 +128,6 @@ calculateAlarmDuration(const QuicConnectionStateBase& conn) { */ template void setLossDetectionAlarm(QuicConnectionStateBase& conn, Timeout& timeout) { - DCHECK_GE( - conn.outstandingPackets.size(), conn.outstandingPureAckPacketsCount); /* * We might have new data or lost data to send even if we don't have any * outstanding packets. When we get a PTO event, it is possible that only @@ -141,16 +139,6 @@ void setLossDetectionAlarm(QuicConnectionStateBase& conn, Timeout& timeout) { bool hasDataToWrite = hasAckDataToWrite(conn) || (hasNonAckDataToWrite(conn) != WriteDataReason::NO_WRITE); auto totalPacketsOutstanding = conn.outstandingPackets.size(); - if (totalPacketsOutstanding == conn.outstandingPureAckPacketsCount) { - VLOG(10) << __func__ << " unset alarm pure ack only" - << " outstanding=" << totalPacketsOutstanding - << " handshakePackets=" << conn.outstandingHandshakePacketsCount - << " pureAckPackets=" << conn.outstandingPureAckPacketsCount << " " - << nodeToString(conn.nodeType) << " " << conn; - conn.pendingEvents.setLossDetectionAlarm = false; - timeout.cancelLossTimeout(); - return; - } /* * We have this condition to disambiguate the case where we have. * (1) All outstanding packets that are clones that are processed and there @@ -164,14 +152,11 @@ void setLossDetectionAlarm(QuicConnectionStateBase& conn, Timeout& timeout) { * so that we can write this data with the slack packet space for the clones. */ if (!hasDataToWrite && conn.outstandingPacketEvents.empty() && - totalPacketsOutstanding == - (conn.outstandingClonedPacketsCount + - conn.outstandingPureAckPacketsCount)) { + totalPacketsOutstanding == conn.outstandingClonedPacketsCount) { VLOG(10) << __func__ << " unset alarm pure ack or processed packets only" << " outstanding=" << totalPacketsOutstanding << " handshakePackets=" << conn.outstandingHandshakePacketsCount - << " pureAckPackets=" << conn.outstandingPureAckPacketsCount << " " - << conn; + << " " << conn; conn.pendingEvents.setLossDetectionAlarm = false; timeout.cancelLossTimeout(); return; @@ -180,8 +165,7 @@ void setLossDetectionAlarm(QuicConnectionStateBase& conn, Timeout& timeout) { VLOG_IF(10, !timeout.isLossTimeoutScheduled()) << __func__ << " alarm not scheduled" << " outstanding=" << totalPacketsOutstanding - << " handshakePackets=" << conn.outstandingHandshakePacketsCount - << " pureAckPackets=" << conn.outstandingPureAckPacketsCount << " " + << " handshakePackets=" << conn.outstandingHandshakePacketsCount << " " << nodeToString(conn.nodeType) << " " << conn; return; } @@ -193,8 +177,7 @@ void setLossDetectionAlarm(QuicConnectionStateBase& conn, Timeout& timeout) { << " method=" << conn.lossState.currentAlarmMethod << " outstanding=" << totalPacketsOutstanding << " handshakePackets=" << conn.outstandingHandshakePacketsCount - << " pureAckPackets=" << conn.outstandingPureAckPacketsCount << " " - << nodeToString(conn.nodeType) << " " << conn; + << " " << nodeToString(conn.nodeType) << " " << conn; timeout.scheduleLossTimeout(alarmDuration.first); conn.pendingEvents.setLossDetectionAlarm = false; } @@ -241,12 +224,7 @@ folly::Optional detectLossPackets( shouldSetTimer = true; break; } - if (!pkt.pureAck) { - lossEvent.addLostPacket(pkt); - } else { - DCHECK_GT(conn.outstandingPureAckPacketsCount, 0); - --conn.outstandingPureAckPacketsCount; - } + lossEvent.addLostPacket(pkt); if (pkt.associatedEvent) { DCHECK_GT(conn.outstandingClonedPacketsCount, 0); --conn.outstandingClonedPacketsCount; @@ -265,17 +243,15 @@ folly::Optional detectLossPackets( --conn.outstandingHandshakePacketsCount; } VLOG(10) << __func__ << " lost packetNum=" << currentPacketNum - << " pureAck=" << pkt.pureAck << " handshake=" << pkt.isHandshake - << " " << conn; + << " handshake=" << pkt.isHandshake << " " << conn; iter = conn.outstandingPackets.erase(iter); } auto earliest = getFirstOutstandingPacket(conn, pnSpace); for (; earliest != conn.outstandingPackets.end(); earliest = getNextOutstandingPacket(conn, pnSpace, earliest + 1)) { - if (!earliest->pureAck && - (!earliest->associatedEvent || - conn.outstandingPacketEvents.count(*earliest->associatedEvent))) { + if (!earliest->associatedEvent || + conn.outstandingPacketEvents.count(*earliest->associatedEvent)) { break; } } @@ -346,7 +322,6 @@ void onHandshakeAlarm( auto currentPacketNumSpace = packet.packet.header.getPacketNumberSpace(); VLOG(10) << "HandshakeAlarm, removing packetNum=" << currentPacketNum << " packetNumSpace=" << currentPacketNumSpace << " " << conn; - DCHECK(!packet.pureAck); lossEvent.addLostPacket(std::move(packet)); lossVisitor(conn, packet.packet, false, currentPacketNum); DCHECK(conn.outstandingHandshakePacketsCount); @@ -405,14 +380,12 @@ void onLossDetectionAlarm( } else { onPTOAlarm(conn); } - conn.pendingEvents.setLossDetectionAlarm = - (conn.outstandingPackets.size() > conn.outstandingPureAckPacketsCount); + conn.pendingEvents.setLossDetectionAlarm = !conn.outstandingPackets.empty(); VLOG(10) << __func__ << " setLossDetectionAlarm=" << conn.pendingEvents.setLossDetectionAlarm << " outstanding=" << conn.outstandingPackets.size() << " handshakePackets=" << conn.outstandingHandshakePacketsCount - << " pureAckPackets=" << conn.outstandingPureAckPacketsCount << " " - << conn; + << " " << conn; } /* @@ -446,16 +419,14 @@ folly::Optional handleAckForLoss( lossVisitor, ack.ackTime, pnSpace); - conn.pendingEvents.setLossDetectionAlarm = - (conn.outstandingPackets.size() > conn.outstandingPureAckPacketsCount); + conn.pendingEvents.setLossDetectionAlarm = !conn.outstandingPackets.empty(); VLOG(10) << __func__ << " largestAckedInPacket=" << ack.largestAckedPacket.value_or(0) << " setLossDetectionAlarm=" << conn.pendingEvents.setLossDetectionAlarm << " outstanding=" << conn.outstandingPackets.size() << " handshakePackets=" << conn.outstandingHandshakePacketsCount - << " pureAckPackets=" << conn.outstandingPureAckPacketsCount << " " - << conn; + << " " << conn; return lossEvent; } @@ -475,7 +446,6 @@ void markZeroRttPacketsLost( iter->packet.header.getProtectionType() == ProtectionType::ZeroRtt; if (isZeroRttPacket) { auto& pkt = *iter; - DCHECK(!pkt.pureAck); DCHECK(!pkt.isHandshake); auto currentPacketNum = pkt.packet.header.getPacketSequenceNum(); bool processed = pkt.associatedEvent && diff --git a/quic/loss/test/QuicLossFunctionsTest.cpp b/quic/loss/test/QuicLossFunctionsTest.cpp index 27977b65b..adb64f8d8 100644 --- a/quic/loss/test/QuicLossFunctionsTest.cpp +++ b/quic/loss/test/QuicLossFunctionsTest.cpp @@ -54,7 +54,6 @@ class QuicLossFunctionsTest : public TestWithParam { PacketNum sendPacket( QuicConnectionStateBase& conn, TimePoint time, - bool pureAck, folly::Optional associatedEvent, PacketType packetType); @@ -131,7 +130,6 @@ auto testingLossMarkFunc(std::vector& lostPackets) { PacketNum QuicLossFunctionsTest::sendPacket( QuicConnectionStateBase& conn, TimePoint time, - bool pureAck, folly::Optional associatedEvent, PacketType packetType) { folly::Optional header; @@ -182,18 +180,14 @@ PacketNum QuicLossFunctionsTest::sendPacket( encodedSize += packet.body->computeChainDataLength(); } auto outstandingPacket = OutstandingPacket( - packet.packet, time, encodedSize, isHandshake, pureAck, encodedSize); + packet.packet, time, encodedSize, isHandshake, encodedSize); outstandingPacket.associatedEvent = associatedEvent; if (isHandshake) { conn.outstandingHandshakePacketsCount++; conn.lossState.lastHandshakePacketSentTime = time; } - if (pureAck) { - conn.outstandingPureAckPacketsCount++; - } else { - conn.lossState.lastRetransmittablePacketSentTime = time; - } - if (!pureAck && conn.congestionController) { + conn.lossState.lastRetransmittablePacketSentTime = time; + if (conn.congestionController) { conn.congestionController->onPacketSent(outstandingPacket); } if (associatedEvent) { @@ -225,11 +219,11 @@ TEST_F(QuicLossFunctionsTest, AllPacketsProcessed) { auto conn = createConn(); EXPECT_CALL(*transportInfoCb_, onPTO()).Times(0); auto pkt1 = conn->ackStates.appDataAckState.nextPacketNum; - sendPacket(*conn, Clock::now(), false, pkt1, PacketType::OneRtt); + sendPacket(*conn, Clock::now(), pkt1, PacketType::OneRtt); auto pkt2 = conn->ackStates.appDataAckState.nextPacketNum; - sendPacket(*conn, Clock::now(), false, pkt2, PacketType::OneRtt); + sendPacket(*conn, Clock::now(), pkt2, PacketType::OneRtt); auto pkt3 = conn->ackStates.appDataAckState.nextPacketNum; - sendPacket(*conn, Clock::now(), false, pkt3, PacketType::OneRtt); + sendPacket(*conn, Clock::now(), pkt3, PacketType::OneRtt); EXPECT_CALL(timeout, cancelLossTimeout()).Times(1); setLossDetectionAlarm(*conn, timeout); EXPECT_FALSE(conn->pendingEvents.setLossDetectionAlarm); @@ -238,7 +232,7 @@ TEST_F(QuicLossFunctionsTest, AllPacketsProcessed) { TEST_F(QuicLossFunctionsTest, HasDataToWrite) { auto conn = createConn(); // There needs to be at least one outstanding packet. - sendPacket(*conn, Clock::now(), false, folly::none, PacketType::OneRtt); + sendPacket(*conn, Clock::now(), folly::none, PacketType::OneRtt); conn->streamManager->addLoss(1); conn->pendingEvents.setLossDetectionAlarm = true; EXPECT_CALL(timeout, cancelLossTimeout()).Times(1); @@ -247,47 +241,6 @@ TEST_F(QuicLossFunctionsTest, HasDataToWrite) { EXPECT_FALSE(conn->pendingEvents.setLossDetectionAlarm); } -TEST_F(QuicLossFunctionsTest, NoLossTimeoutIfOnlyPureAcksAreOutstanding) { - auto conn = createConn(); - conn->pendingEvents.setLossDetectionAlarm = true; - EXPECT_TRUE(conn->outstandingPackets.empty()); - EXPECT_EQ(0, conn->outstandingPureAckPacketsCount); - - // Empty outstandingPackets: no timer scheduled - EXPECT_CALL(timeout, cancelLossTimeout()).Times(1); - setLossDetectionAlarm(*conn, timeout); - EXPECT_FALSE(conn->pendingEvents.setLossDetectionAlarm); - - TimePoint now = Clock::now(); - // Only pure acks outstanding: no timer scheduled - sendPacket(*conn, now, true, folly::none, PacketType::OneRtt); - EXPECT_TRUE(conn->pendingEvents.setLossDetectionAlarm); - EXPECT_EQ(1, conn->outstandingPackets.size()); - EXPECT_EQ(1, conn->outstandingPureAckPacketsCount); - EXPECT_CALL(timeout, cancelLossTimeout()).Times(1); - setLossDetectionAlarm(*conn, timeout); - EXPECT_FALSE(conn->pendingEvents.setLossDetectionAlarm); - - // Non-pure ack outstanding: timer scheduled - sendPacket(*conn, now, false, folly::none, PacketType::Handshake); - EXPECT_TRUE(conn->pendingEvents.setLossDetectionAlarm); - EXPECT_EQ(2, conn->outstandingPackets.size()); - EXPECT_EQ(1, conn->outstandingPureAckPacketsCount); - EXPECT_CALL(timeout, cancelLossTimeout()).Times(1); - EXPECT_CALL(timeout, scheduleLossTimeout(_)).Times(1); - setLossDetectionAlarm(*conn, timeout); - EXPECT_FALSE(conn->pendingEvents.setLossDetectionAlarm); - - // Non-pure poped from outstanding: no timer scheduled - conn->outstandingPackets.pop_back(); - conn->pendingEvents.setLossDetectionAlarm = true; - EXPECT_EQ(1, conn->outstandingPackets.size()); - EXPECT_EQ(1, conn->outstandingPureAckPacketsCount); - EXPECT_CALL(timeout, cancelLossTimeout()).Times(1); - setLossDetectionAlarm(*conn, timeout); - EXPECT_FALSE(conn->pendingEvents.setLossDetectionAlarm); -} - TEST_F(QuicLossFunctionsTest, TestOnLossDetectionAlarm) { auto conn = createConn(); auto mockCongestionController = std::make_unique(); @@ -296,7 +249,7 @@ TEST_F(QuicLossFunctionsTest, TestOnLossDetectionAlarm) { EXPECT_CALL(*rawCongestionController, onPacketSent(_)) .WillRepeatedly(Return()); - sendPacket(*conn, Clock::now(), false, folly::none, PacketType::OneRtt); + sendPacket(*conn, Clock::now(), folly::none, PacketType::OneRtt); MockClock::mockNow = []() { return TimePoint(123ms); }; std::vector lostPacket; MockClock::mockNow = []() { return TimePoint(23ms); }; @@ -312,7 +265,7 @@ TEST_F(QuicLossFunctionsTest, TestOnLossDetectionAlarm) { MockClock::mockNow = []() { return TimePoint(3ms); }; EXPECT_CALL(*transportInfoCb_, onPTO()); - sendPacket(*conn, TimePoint(), false, folly::none, PacketType::OneRtt); + sendPacket(*conn, TimePoint(), folly::none, PacketType::OneRtt); setLossDetectionAlarm(*conn, timeout); EXPECT_CALL(*rawCongestionController, onPacketAckOrLoss(_, _)).Times(0); onLossDetectionAlarm( @@ -320,7 +273,6 @@ TEST_F(QuicLossFunctionsTest, TestOnLossDetectionAlarm) { EXPECT_EQ(conn->lossState.ptoCount, 2); // PTO doesn't take anything out of outstandingPackets EXPECT_FALSE(conn->outstandingPackets.empty()); - EXPECT_EQ(0, conn->outstandingPureAckPacketsCount); EXPECT_TRUE(conn->pendingEvents.setLossDetectionAlarm); // PTO shouldn't mark loss EXPECT_TRUE(lostPacket.empty()); @@ -336,7 +288,7 @@ TEST_F(QuicLossFunctionsTest, TestOnPTOSkipProcessed) { // By adding an associatedEvent that doesn't exist in the // outstandingPacketEvents, they are all processed and will skip lossVisitor for (auto i = 0; i < 10; i++) { - sendPacket(*conn, TimePoint(), false, i, PacketType::OneRtt); + sendPacket(*conn, TimePoint(), i, PacketType::OneRtt); } EXPECT_EQ(10, conn->outstandingPackets.size()); std::vector lostPackets; @@ -542,11 +494,9 @@ TEST_F(QuicLossFunctionsTest, TestReorderingThreshold) { lostPacket.push_back(packetNum); }; for (int i = 0; i < 6; ++i) { - sendPacket( - *conn, Clock::now(), !(i % 2), folly::none, PacketType::Handshake); + sendPacket(*conn, Clock::now(), folly::none, PacketType::Handshake); } EXPECT_EQ(6, conn->outstandingHandshakePacketsCount); - EXPECT_EQ(3, conn->outstandingPureAckPacketsCount); // Assume some packets are already acked for (auto iter = getFirstOutstandingPacket(*conn, PacketNumberSpace::Handshake) + 2; @@ -556,9 +506,6 @@ TEST_F(QuicLossFunctionsTest, TestReorderingThreshold) { if (iter->isHandshake) { conn->outstandingHandshakePacketsCount--; } - if (iter->pureAck) { - conn->outstandingPureAckPacketsCount--; - } } auto firstHandshakeOpIter = getFirstOutstandingPacket(*conn, PacketNumberSpace::Handshake); @@ -580,7 +527,6 @@ TEST_F(QuicLossFunctionsTest, TestReorderingThreshold) { // Packet 6 is the only thing remaining inflight, it is a handshake pkt EXPECT_EQ(1, conn->outstandingHandshakePacketsCount); - EXPECT_EQ(0, conn->outstandingPureAckPacketsCount); // Packet 6 should remain in packet as the delta is less than threshold EXPECT_EQ(conn->outstandingPackets.size(), 1); @@ -605,7 +551,7 @@ TEST_F(QuicLossFunctionsTest, TestHandleAckForLoss) { RegularQuicWritePacket outstandingRegularPacket(std::move(longHeader)); auto now = Clock::now(); conn->outstandingPackets.emplace_back( - OutstandingPacket(outstandingRegularPacket, now, 0, false, false, 0)); + OutstandingPacket(outstandingRegularPacket, now, 0, false, 0)); bool testLossMarkFuncCalled = false; auto testLossMarkFunc = [&](auto& /* conn */, auto&, bool, PacketNum) { @@ -620,7 +566,6 @@ TEST_F(QuicLossFunctionsTest, TestHandleAckForLoss) { EXPECT_EQ(0, conn->lossState.ptoCount); EXPECT_TRUE(conn->outstandingPackets.empty()); - EXPECT_EQ(0, conn->outstandingPureAckPacketsCount); EXPECT_FALSE(conn->pendingEvents.setLossDetectionAlarm); EXPECT_TRUE(testLossMarkFuncCalled); @@ -642,7 +587,7 @@ TEST_F(QuicLossFunctionsTest, TestHandleAckedPacket) { conn->lossState.handshakeAlarmCount = 5; conn->lossState.reorderingThreshold = 10; - sendPacket(*conn, TimePoint(), false, folly::none, PacketType::OneRtt); + sendPacket(*conn, TimePoint(), folly::none, PacketType::OneRtt); ReadAckFrame ackFrame; ackFrame.largestAcked = conn->lossState.largestSent; @@ -668,7 +613,6 @@ TEST_F(QuicLossFunctionsTest, TestHandleAckedPacket) { EXPECT_EQ(0, conn->lossState.ptoCount); EXPECT_EQ(0, conn->lossState.handshakeAlarmCount); EXPECT_TRUE(conn->outstandingPackets.empty()); - EXPECT_EQ(0, conn->outstandingPureAckPacketsCount); EXPECT_FALSE(conn->pendingEvents.setLossDetectionAlarm); EXPECT_FALSE(testLossMarkFuncCalled); ASSERT_TRUE(conn->outstandingPackets.empty()); @@ -758,8 +702,8 @@ TEST_F(QuicLossFunctionsTest, ReorderingThresholdChecksSamePacketNumberSpace) { }; PacketNum latestSent = 0; for (size_t i = 0; i < conn->lossState.reorderingThreshold + 1; i++) { - latestSent = sendPacket( - *conn, Clock::now(), false, folly::none, PacketType::Handshake); + latestSent = + sendPacket(*conn, Clock::now(), folly::none, PacketType::Handshake); } detectLossPackets( @@ -818,7 +762,7 @@ TEST_F(QuicLossFunctionsTest, TestTimeReordering) { PacketNum largestSent = 0; for (int i = 0; i < 7; ++i) { largestSent = sendPacket( - *conn, TimePoint(i * 100ms), false, folly::none, PacketType::OneRtt); + *conn, TimePoint(i * 100ms), folly::none, PacketType::OneRtt); } // Some packets are already acked conn->lossState.srtt = 400ms; @@ -855,9 +799,9 @@ TEST_F(QuicLossFunctionsTest, LossTimePreemptsCryptoTimer) { 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); + sendPacket(*conn, sendTime, folly::none, PacketType::Handshake); + PacketNum second = + sendPacket(*conn, sendTime + 1ms, folly::none, PacketType::Handshake); auto lossTime = sendTime + 50ms; detectLossPackets( *conn, @@ -904,7 +848,7 @@ TEST_F(QuicLossFunctionsTest, PTONoLongerMarksPacketsToBeRetransmitted) { MockClock::mockNow = [&]() { return startTime; }; std::vector lostPackets; for (auto i = 0; i < kPacketToSendForPTO + 10; i++) { - sendPacket(*conn, startTime, false, folly::none, PacketType::OneRtt); + sendPacket(*conn, startTime, folly::none, PacketType::OneRtt); setLossDetectionAlarm(*conn, timeout); startTime += 1ms; } @@ -936,7 +880,6 @@ TEST_F( auto sentPacketNum = sendPacket( *conn, TimePoint(100ms), - false, folly::none, (i % 2 ? PacketType::OneRtt : PacketType::Handshake)); expectedLargestLostNum = std::max( @@ -980,8 +923,7 @@ TEST_F(QuicLossFunctionsTest, HandshakeAlarmWithOneRttCipher) { conn->oneRttWriteCipher = createNoOpAead(); conn->lossState.currentAlarmMethod = LossState::AlarmMethod::Handshake; std::vector lostPackets; - sendPacket( - *conn, TimePoint(100ms), false, folly::none, PacketType::Handshake); + sendPacket(*conn, TimePoint(100ms), folly::none, PacketType::Handshake); onLossDetectionAlarm( *conn, testingLossMarkFunc(lostPackets)); @@ -1001,45 +943,6 @@ TEST_F(QuicLossFunctionsTest, HandshakeAlarmWithOneRttCipher) { EXPECT_EQ(event->type, kHandshakeAlarm); } -TEST_F(QuicLossFunctionsTest, PureAckSkipsCongestionControl) { - std::vector lostPacket; - auto conn = createConn(); - auto mockCongestionController = std::make_unique(); - auto rawCongestionController = mockCongestionController.get(); - conn->congestionController = std::move(mockCongestionController); - EXPECT_CALL(*rawCongestionController, onPacketSent(_)) - .WillRepeatedly(Return()); - - for (int i = 0; i < 5; ++i) { - bool pureAck = (i == 3); - sendPacket(*conn, TimePoint(), pureAck, folly::none, PacketType::Handshake); - } - - // Verify that bytes for pure ack pkt won't be counted in lost bytes - auto sumNoPureAckBytes = std::accumulate( - conn->outstandingPackets.begin(), - conn->outstandingPackets.end(), - 0, - [](uint64_t bytes, const OutstandingPacket& p) { - if (p.pureAck) { - return bytes; - } - return bytes + p.encodedSize; - }); - - // Ack for packet 9 arrives - auto lossEvent = detectLossPackets( - *conn, - 9, - testingLossMarkFunc(lostPacket), - TimePoint(80ms), - PacketNumberSpace::Handshake); - EXPECT_EQ(5, lossEvent->largestLostPacketNum.value()); - EXPECT_EQ(sumNoPureAckBytes, lossEvent->lostBytes); - EXPECT_EQ(TimePoint(80ms), lossEvent->lossTime); - EXPECT_EQ(conn->outstandingHandshakePacketsCount, 0); -} - TEST_F(QuicLossFunctionsTest, EmptyOutstandingNoTimeout) { auto conn = createConn(); EXPECT_CALL(timeout, cancelLossTimeout()).Times(1); @@ -1053,8 +956,7 @@ TEST_F(QuicLossFunctionsTest, AlarmDurationHandshakeOutstanding) { std::chrono::milliseconds packetSentDelay = 10ms; auto thisMoment = lastPacketSentTime + packetSentDelay; MockClock::mockNow = [=]() { return thisMoment; }; - sendPacket( - *conn, lastPacketSentTime, false, folly::none, PacketType::Handshake); + sendPacket(*conn, lastPacketSentTime, folly::none, PacketType::Handshake); MockClock::mockNow = [=]() { return thisMoment; }; auto duration = calculateAlarmDuration(*conn); @@ -1088,7 +990,7 @@ TEST_F(QuicLossFunctionsTest, AlarmDurationHasLossTime) { conn->lossState.srtt = 200ms; conn->lossState.lrtt = 150ms; - sendPacket(*conn, lastPacketSentTime, false, folly::none, PacketType::OneRtt); + sendPacket(*conn, lastPacketSentTime, folly::none, PacketType::OneRtt); auto duration = calculateAlarmDuration(*conn); EXPECT_EQ(100ms, duration.first); EXPECT_EQ( @@ -1106,7 +1008,7 @@ TEST_F(QuicLossFunctionsTest, AlarmDurationLossTimeIsZero) { conn->lossState.srtt = 200ms; conn->lossState.lrtt = 150ms; - sendPacket(*conn, lastPacketSentTime, false, folly::none, PacketType::OneRtt); + sendPacket(*conn, lastPacketSentTime, folly::none, PacketType::OneRtt); auto duration = calculateAlarmDuration(*conn); EXPECT_EQ(0ms, duration.first); EXPECT_EQ( @@ -1120,7 +1022,7 @@ TEST_F(QuicLossFunctionsTest, AlarmDurationNonHandshakeOutstanding) { conn->lossState.maxAckDelay = 25ms; TimePoint lastPacketSentTime = Clock::now(); MockClock::mockNow = [=]() { return lastPacketSentTime; }; - sendPacket(*conn, lastPacketSentTime, false, folly::none, PacketType::OneRtt); + sendPacket(*conn, lastPacketSentTime, folly::none, PacketType::OneRtt); auto duration = calculateAlarmDuration(*conn); EXPECT_EQ(duration.second, LossState::AlarmMethod::PTO); setLossDetectionAlarm(*conn, timeout); @@ -1149,8 +1051,7 @@ TEST_F(QuicLossFunctionsTest, NoSkipLossVisitor) { // Send 5 packets, so when we ack the last one, we mark the first one loss PacketNum lastSent; for (size_t i = 0; i < 5; i++) { - lastSent = - sendPacket(*conn, Clock::now(), false, folly::none, PacketType::OneRtt); + lastSent = sendPacket(*conn, Clock::now(), folly::none, PacketType::OneRtt); } detectLossPackets( *conn, @@ -1179,7 +1080,7 @@ TEST_F(QuicLossFunctionsTest, SkipLossVisitor) { PacketNum lastSent; for (size_t i = 0; i < 5; i++) { lastSent = conn->ackStates.appDataAckState.nextPacketNum; - sendPacket(*conn, Clock::now(), false, lastSent, PacketType::OneRtt); + sendPacket(*conn, Clock::now(), lastSent, PacketType::OneRtt); } detectLossPackets( *conn, @@ -1209,8 +1110,7 @@ TEST_F(QuicLossFunctionsTest, NoDoubleProcess) { PacketNum lastSent; PacketEvent event = 0; for (size_t i = 0; i < 6; i++) { - lastSent = - sendPacket(*conn, Clock::now(), false, event, PacketType::OneRtt); + lastSent = sendPacket(*conn, Clock::now(), event, PacketType::OneRtt); } EXPECT_EQ(6, conn->outstandingPackets.size()); // Add the PacketEvent to the outstandingPacketEvents set @@ -1231,12 +1131,12 @@ TEST_F(QuicLossFunctionsTest, NoDoubleProcess) { TEST_F(QuicLossFunctionsTest, DetectPacketLossClonedPacketsCounter) { auto conn = createConn(); auto packet1 = conn->ackStates.appDataAckState.nextPacketNum; - sendPacket(*conn, Clock::now(), false, packet1, PacketType::OneRtt); - sendPacket(*conn, Clock::now(), false, folly::none, PacketType::OneRtt); - sendPacket(*conn, Clock::now(), false, folly::none, PacketType::OneRtt); - sendPacket(*conn, Clock::now(), false, folly::none, PacketType::OneRtt); + sendPacket(*conn, Clock::now(), packet1, PacketType::OneRtt); + sendPacket(*conn, Clock::now(), folly::none, PacketType::OneRtt); + sendPacket(*conn, Clock::now(), folly::none, PacketType::OneRtt); + sendPacket(*conn, Clock::now(), folly::none, PacketType::OneRtt); auto ackedPacket = - sendPacket(*conn, Clock::now(), false, folly::none, PacketType::OneRtt); + sendPacket(*conn, Clock::now(), folly::none, PacketType::OneRtt); auto noopLossMarker = [](auto&, auto&, bool, PacketNum) {}; detectLossPackets( *conn, @@ -1357,7 +1257,7 @@ TEST_F(QuicLossFunctionsTest, TotalLossCount) { PacketNum largestSent = 0; for (int i = 0; i < 10; i++) { largestSent = - sendPacket(*conn, Clock::now(), false, folly::none, PacketType::OneRtt); + sendPacket(*conn, Clock::now(), folly::none, PacketType::OneRtt); } EXPECT_EQ(10, conn->outstandingPackets.size()); uint32_t lostPackets = 0; @@ -1390,8 +1290,8 @@ TEST_F(QuicLossFunctionsTest, TestZeroRttRejected) { // By adding an associatedEvent that doesn't exist in the // outstandingPacketEvents, they are all processed and will skip lossVisitor for (auto i = 0; i < 2; i++) { - sendPacket(*conn, TimePoint(), false, folly::none, PacketType::OneRtt); - sendPacket(*conn, TimePoint(), false, folly::none, PacketType::ZeroRtt); + sendPacket(*conn, TimePoint(), folly::none, PacketType::OneRtt); + sendPacket(*conn, TimePoint(), folly::none, PacketType::ZeroRtt); } EXPECT_FALSE(conn->outstandingPackets.empty()); EXPECT_EQ(4, conn->outstandingPackets.size()); @@ -1429,14 +1329,14 @@ TEST_F(QuicLossFunctionsTest, TestZeroRttRejectedWithClones) { folly::Optional lastPacket; for (auto i = 0; i < 2; i++) { lastPacket = - sendPacket(*conn, TimePoint(), false, lastPacket, PacketType::ZeroRtt); + sendPacket(*conn, TimePoint(), lastPacket, PacketType::ZeroRtt); zeroRttPackets.emplace(*lastPacket); } zeroRttPackets.emplace( - sendPacket(*conn, TimePoint(), false, folly::none, PacketType::ZeroRtt)); + sendPacket(*conn, TimePoint(), folly::none, PacketType::ZeroRtt)); for (auto zeroRttPacketNum : zeroRttPackets) { - lastPacket = sendPacket( - *conn, TimePoint(), false, zeroRttPacketNum, PacketType::OneRtt); + lastPacket = + sendPacket(*conn, TimePoint(), zeroRttPacketNum, PacketType::OneRtt); } EXPECT_EQ(6, conn->outstandingPackets.size()); @@ -1477,12 +1377,11 @@ TEST_F(QuicLossFunctionsTest, TimeThreshold) { auto conn = createConn(); conn->lossState.srtt = 10ms; auto referenceTime = Clock::now(); - auto packet1 = sendPacket( - *conn, referenceTime - 10ms, false, folly::none, PacketType::OneRtt); + auto packet1 = + sendPacket(*conn, referenceTime - 10ms, folly::none, PacketType::OneRtt); auto packet2 = sendPacket( *conn, referenceTime + conn->lossState.srtt / 2, - false, folly::none, PacketType::OneRtt); auto lossVisitor = [&](const auto& /*conn*/, @@ -1503,7 +1402,7 @@ TEST_P(QuicLossFunctionsTest, CappedShiftNoCrash) { auto conn = createConn(); conn->lossState.handshakeAlarmCount = std::numeric_limitslossState.handshakeAlarmCount)>::max(); - sendPacket(*conn, Clock::now(), false, folly::none, PacketType::Handshake); + sendPacket(*conn, Clock::now(), folly::none, PacketType::Handshake); ASSERT_GT(conn->outstandingHandshakePacketsCount, 0); calculateAlarmDuration(*conn); @@ -1512,7 +1411,7 @@ TEST_P(QuicLossFunctionsTest, CappedShiftNoCrash) { conn->outstandingPackets.clear(); conn->lossState.ptoCount = std::numeric_limitslossState.ptoCount)>::max(); - sendPacket(*conn, Clock::now(), false, folly::none, PacketType::OneRtt); + sendPacket(*conn, Clock::now(), folly::none, PacketType::OneRtt); calculateAlarmDuration(*conn); } diff --git a/quic/server/test/QuicServerTransportTest.cpp b/quic/server/test/QuicServerTransportTest.cpp index d5d204b39..81abea81c 100644 --- a/quic/server/test/QuicServerTransportTest.cpp +++ b/quic/server/test/QuicServerTransportTest.cpp @@ -829,10 +829,25 @@ TEST_F(QuicServerTransportTest, IdleTimerNotResetWhenDataOutstanding) { server->getNonConstConn().receivedNewPacketBeforeWrite = false; StreamId streamId = server->createBidirectionalStream().value(); - auto expected = IOBuf::copyBuffer("hello"); server->idleTimeout().cancelTimeout(); ASSERT_FALSE(server->idleTimeout().isScheduled()); - server->writeChain(streamId, expected->clone(), false, false); + server->writeChain( + streamId, + IOBuf::copyBuffer("And if the darkness is to keep us apart"), + false, + false); + loopForWrites(); + // It was the first packet + ASSERT_TRUE(server->idleTimeout().isScheduled()); + + // cancel it and write something else. This time idle timer shouldn't set. + server->idleTimeout().cancelTimeout(); + ASSERT_FALSE(server->idleTimeout().isScheduled()); + server->writeChain( + streamId, + IOBuf::copyBuffer("And if the daylight feels like it's a long way off"), + false, + false); loopForWrites(); ASSERT_FALSE(server->idleTimeout().isScheduled()); } @@ -1140,7 +1155,6 @@ TEST_F(QuicServerTransportTest, TestOpenAckStreamFrame) { // Remove any packets that might have been queued. server->getNonConstConn().outstandingPackets.clear(); - server->getNonConstConn().outstandingPureAckPacketsCount = 0; server->getNonConstConn().outstandingHandshakePacketsCount = 0; server->writeChain(streamId, data->clone(), false, false); loopForWrites(); @@ -1725,7 +1739,6 @@ TEST_F(QuicServerTransportTest, TestCloneStopSending) { server->getNonConstConn().streamManager->getStream(streamId); // knock every handshake outstanding packets out server->getNonConstConn().outstandingHandshakePacketsCount = 0; - server->getNonConstConn().outstandingPureAckPacketsCount = 0; server->getNonConstConn().outstandingPackets.clear(); server->getNonConstConn().lossState.initialLossTime.clear(); server->getNonConstConn().lossState.handshakeLossTime.clear(); diff --git a/quic/state/AckHandlers.cpp b/quic/state/AckHandlers.cpp index d9a2e83e7..a163a44a6 100644 --- a/quic/state/AckHandlers.cpp +++ b/quic/state/AckHandlers.cpp @@ -39,8 +39,6 @@ void processAckFrame( const AckVisitor& ackVisitor, const LossVisitor& lossVisitor, const TimePoint& ackReceiveTime) { - DCHECK_GE( - conn.outstandingPackets.size(), conn.outstandingPureAckPacketsCount); // TODO: send error if we get an ack for a packet we've not sent t18721184 CongestionController::AckEvent ack; ack.ackTime = ackReceiveTime; @@ -51,7 +49,6 @@ void processAckFrame( ack.ackedPackets.reserve(kRxPacketsPendingBeforeAckThresh); auto currentPacketIt = getLastOutstandingPacket(conn, pnSpace); uint64_t handshakePacketAcked = 0; - uint64_t pureAckPacketsAcked = 0; uint64_t clonedPacketsAcked = 0; folly::Optional lastAckedPacketSentTime; @@ -109,16 +106,11 @@ void processAckFrame( } VLOG(10) << __func__ << " acked packetNum=" << currentPacketNum << " space=" << currentPacketNumberSpace - << " handshake=" << (int)rPacketIt->isHandshake - << " pureAck=" << (int)rPacketIt->pureAck << " " << conn; + << " handshake=" << (int)rPacketIt->isHandshake << " " << conn; if (rPacketIt->isHandshake) { ++handshakePacketAcked; } - if (!rPacketIt->pureAck) { - ack.ackedBytes += rPacketIt->encodedSize; - } else { - ++pureAckPacketsAcked; - } + ack.ackedBytes += rPacketIt->encodedSize; if (rPacketIt->associatedEvent) { ++clonedPacketsAcked; } @@ -127,7 +119,7 @@ void processAckFrame( ackReceiveTime > rPacketIt->time ? ackReceiveTime : Clock::now(); auto rttSample = std::chrono::duration_cast( ackReceiveTimeOrNow - rPacketIt->time); - if (currentPacketNum == frame.largestAcked && !rPacketIt->pureAck) { + if (currentPacketNum == frame.largestAcked) { updateRtt(conn, rttSample, frame.ackDelay); } if (conn.qLogger) { @@ -190,12 +182,9 @@ void processAckFrame( } DCHECK_GE(conn.outstandingHandshakePacketsCount, handshakePacketAcked); conn.outstandingHandshakePacketsCount -= handshakePacketAcked; - DCHECK_GE(conn.outstandingPureAckPacketsCount, pureAckPacketsAcked); - conn.outstandingPureAckPacketsCount -= pureAckPacketsAcked; DCHECK_GE(conn.outstandingClonedPacketsCount, clonedPacketsAcked); conn.outstandingClonedPacketsCount -= clonedPacketsAcked; auto updatedOustandingPacketsCount = conn.outstandingPackets.size(); - DCHECK_GE(updatedOustandingPacketsCount, conn.outstandingPureAckPacketsCount); DCHECK_GE( updatedOustandingPacketsCount, conn.outstandingHandshakePacketsCount); DCHECK_GE(updatedOustandingPacketsCount, conn.outstandingClonedPacketsCount); diff --git a/quic/state/StateData.h b/quic/state/StateData.h index de38a6bfd..b9984eee2 100644 --- a/quic/state/StateData.h +++ b/quic/state/StateData.h @@ -93,6 +93,7 @@ struct NetworkDataSingle { */ using PacketEvent = PacketNum; +// Data structure to represent outstanding retransmittable packets struct OutstandingPacket { // Structure representing the frames that are outstanding including the header // that was sent. @@ -103,8 +104,6 @@ struct OutstandingPacket { uint32_t encodedSize; // Whether this packet has any data from stream 0 bool isHandshake; - // Whether this packet is pure ack - bool pureAck; // Total sent bytes on this connection including this packet itself when this // packet is sent. uint64_t totalBytesSent; @@ -146,13 +145,11 @@ struct OutstandingPacket { TimePoint timeIn, uint32_t encodedSizeIn, bool isHandshakeIn, - bool pureAckIn, uint64_t totalBytesSentIn) : packet(std::move(packetIn)), time(std::move(timeIn)), encodedSize(encodedSizeIn), isHandshake(isHandshakeIn), - pureAck(pureAckIn), totalBytesSent(totalBytesSentIn) {} }; @@ -521,9 +518,6 @@ struct QuicConnectionStateBase { // TODO: Enforce only AppTraffic packets to be clonable folly::F14FastSet outstandingPacketEvents; - // Number of pure ack packets outstanding. - uint64_t outstandingPureAckPacketsCount{0}; - // Number of handshake packets outstanding. uint64_t outstandingHandshakePacketsCount{0}; diff --git a/quic/state/test/AckHandlersTest.cpp b/quic/state/test/AckHandlersTest.cpp index 792cf728d..04bad9960 100644 --- a/quic/state/test/AckHandlersTest.cpp +++ b/quic/state/test/AckHandlersTest.cpp @@ -27,11 +27,11 @@ namespace test { class AckHandlersTest : public TestWithParam {}; auto testLossHandler(std::vector& lostPackets) -> decltype(auto) { - return [&lostPackets]( - QuicConnectionStateBase&, auto& packet, bool, PacketNum) { - auto packetNum = packet.header.getPacketSequenceNum(); - lostPackets.push_back(packetNum); - }; + return + [&lostPackets](QuicConnectionStateBase&, auto& packet, bool, PacketNum) { + auto packetNum = packet.header.getPacketSequenceNum(); + lostPackets.push_back(packetNum); + }; } TEST_P(AckHandlersTest, TestAckMultipleSequentialBlocks) { @@ -51,7 +51,7 @@ TEST_P(AckHandlersTest, TestAckMultipleSequentialBlocks) { WriteStreamFrame frame(currentStreamId++, 0, 0, true); regularPacket.frames.emplace_back(std::move(frame)); conn.outstandingPackets.emplace_back(OutstandingPacket( - std::move(regularPacket), sentTime, 1, false, false, packetNum)); + std::move(regularPacket), sentTime, 1, false, packetNum)); } ReadAckFrame ackFrame; ackFrame.largestAcked = 101; @@ -122,7 +122,7 @@ TEST_P(AckHandlersTest, TestAckBlocksWithGaps) { WriteStreamFrame frame(currentStreamId++, 0, 0, true); regularPacket.frames.emplace_back(std::move(frame)); conn.outstandingPackets.emplace_back(OutstandingPacket( - std::move(regularPacket), Clock::now(), 1, false, false, packetNum)); + std::move(regularPacket), Clock::now(), 1, false, packetNum)); } ReadAckFrame ackFrame; @@ -223,7 +223,7 @@ TEST_P(AckHandlersTest, TestNonSequentialPacketNumbers) { WriteStreamFrame frame(current++, 0, 0, true); regularPacket.frames.emplace_back(std::move(frame)); conn.outstandingPackets.emplace_back(OutstandingPacket( - std::move(regularPacket), Clock::now(), 1, false, false, packetNum)); + std::move(regularPacket), Clock::now(), 1, false, packetNum)); } for (PacketNum packetNum = 20; packetNum < 40; packetNum += 3) { @@ -232,7 +232,7 @@ TEST_P(AckHandlersTest, TestNonSequentialPacketNumbers) { current += 3; regularPacket.frames.emplace_back(std::move(frame)); conn.outstandingPackets.emplace_back(OutstandingPacket( - std::move(regularPacket), Clock::now(), 1, false, false, packetNum)); + std::move(regularPacket), Clock::now(), 1, false, packetNum)); } ReadAckFrame ackFrame; @@ -311,8 +311,8 @@ TEST_P(AckHandlersTest, AckVisitorForAckTest) { conn.ackStates.appDataAckState.acks.insert(900, 1000); conn.ackStates.appDataAckState.acks.insert(500, 700); firstPacket.frames.emplace_back(std::move(firstAckFrame)); - conn.outstandingPackets.emplace_back(OutstandingPacket( - std::move(firstPacket), Clock::now(), 0, false, false, 0)); + conn.outstandingPackets.emplace_back( + OutstandingPacket(std::move(firstPacket), Clock::now(), 0, false, 0)); auto secondPacket = createNewPacket(101 /* packetNum */, GetParam()); WriteAckFrame secondAckFrame; @@ -321,8 +321,8 @@ TEST_P(AckHandlersTest, AckVisitorForAckTest) { conn.ackStates.appDataAckState.acks.insert(1100, 2000); conn.ackStates.appDataAckState.acks.insert(1002, 1090); secondPacket.frames.emplace_back(std::move(secondAckFrame)); - conn.outstandingPackets.emplace_back(OutstandingPacket( - std::move(secondPacket), Clock::now(), 0, false, false, 0)); + conn.outstandingPackets.emplace_back( + OutstandingPacket(std::move(secondPacket), Clock::now(), 0, false, 0)); ReadAckFrame firstReceivedAck; firstReceivedAck.largestAcked = 100; @@ -385,8 +385,8 @@ TEST_P(AckHandlersTest, NoNewAckedPacket) { conn.lossState.ptoCount = 1; PacketNum packetAfterRtoNum = 10; auto packetAfterRto = createNewPacket(packetAfterRtoNum, GetParam()); - conn.outstandingPackets.emplace_back(OutstandingPacket( - std::move(packetAfterRto), Clock::now(), 0, false, false, 0)); + conn.outstandingPackets.emplace_back( + OutstandingPacket(std::move(packetAfterRto), Clock::now(), 0, false, 0)); ReadAckFrame ackFrame; ackFrame.largestAcked = 5; @@ -430,12 +430,12 @@ TEST_P(AckHandlersTest, AckPacketNumDoesNotExist) { PacketNum packetNum1 = 9; auto regularPacket1 = createNewPacket(packetNum1, GetParam()); conn.outstandingPackets.emplace_back( - std::move(regularPacket1), Clock::now(), 0, false, false, 0); + std::move(regularPacket1), Clock::now(), 0, false, 0); PacketNum packetNum2 = 10; auto regularPacket2 = createNewPacket(packetNum2, GetParam()); conn.outstandingPackets.emplace_back( - std::move(regularPacket2), Clock::now(), 0, false, false, 0); + std::move(regularPacket2), Clock::now(), 0, false, 0); // Ack a packet one higher than the packet so that we don't trigger reordering // threshold. @@ -466,7 +466,6 @@ TEST_P(AckHandlersTest, TestHandshakeCounterUpdate) { Clock::now(), 0, packetNum % 2, - false, packetNum / 2); conn.outstandingHandshakePacketsCount += packetNum % 2; } @@ -507,76 +506,6 @@ TEST_P(AckHandlersTest, PurgeAcks) { expectedTime, *conn.ackStates.initialAckState.largestRecvdPacketTime); } -TEST_P(AckHandlersTest, PureAckBytesCountedTowardsTotalBytesAcked) { - QuicServerConnectionState conn; - ASSERT_EQ(0, conn.lossState.totalBytesAcked); - auto regularPacket = createNewPacket(10, GetParam()); - WriteAckFrame ack; - ack.ackBlocks.insert(2, 5); - conn.outstandingPackets.emplace_back(OutstandingPacket( - std::move(regularPacket), Clock::now(), 2, false, true, 2)); - conn.outstandingPureAckPacketsCount++; - - ReadAckFrame ackFrame; - ackFrame.largestAcked = 12; - ackFrame.ackBlocks.emplace_back(5, 12); - - std::vector lostPackets; - processAckFrame( - conn, - GetParam(), - ackFrame, - [&](const auto&, const auto&, const auto&) {}, - testLossHandler(lostPackets), - Clock::now()); - // Packet should be removed from outstandingPackets without triggering - // onPacketAckOrLoss - EXPECT_TRUE(conn.outstandingPackets.empty()); - EXPECT_GT(conn.lossState.totalBytesAcked, 0); -} - -TEST_P(AckHandlersTest, PureAckBytesSkipsCongestionControl) { - QuicServerConnectionState conn; - auto mockController = std::make_unique(); - auto rawController = mockController.get(); - conn.congestionController = std::move(mockController); - - auto regularPacket = createNewPacket(10, GetParam()); - WriteAckFrame ack; - ack.ackBlocks.insert(2, 5); - conn.outstandingPackets.emplace_back(OutstandingPacket( - std::move(regularPacket), Clock::now(), 2, false, true, 2)); - conn.outstandingPureAckPacketsCount++; - - ReadAckFrame ackFrame; - ackFrame.largestAcked = 12; - ackFrame.ackBlocks.emplace_back(5, 12); - - std::vector lostPackets; - // onPacketAckOrLoss will be called, but the ackedBytes is 0. We only need the - // largestAckedPacket - EXPECT_CALL(*rawController, onPacketAckOrLoss(_, _)) - .Times(1) - .WillOnce(Invoke([&](auto ackEvent, auto) { - EXPECT_EQ(0, ackEvent->ackedBytes); - EXPECT_EQ(10, ackEvent->largestAckedPacket.value()); - EXPECT_FALSE(ackEvent->ackedPackets.empty()); - EXPECT_EQ(1, ackEvent->ackedPackets.size()); - EXPECT_EQ(2, ackEvent->ackedPackets.front().encodedSize); - EXPECT_EQ(2, ackEvent->ackedPackets.front().totalBytesSentThen); - })); - processAckFrame( - conn, - GetParam(), - ackFrame, - [&](const auto&, const auto&, const auto&) {}, - testLossHandler(lostPackets), - Clock::now()); - // Packet should be removed from outstandingPackets without triggering - // onPacketAckOrLoss - EXPECT_TRUE(conn.outstandingPackets.empty()); -} - TEST_P(AckHandlersTest, NoSkipAckVisitor) { QuicServerConnectionState conn; auto mockCongestionController = std::make_unique(); @@ -595,8 +524,8 @@ TEST_P(AckHandlersTest, NoSkipAckVisitor) { // We need to at least have one frame to trigger ackVisitor WriteStreamFrame frame(0, 0, 0, true); regularPacket.frames.emplace_back(std::move(frame)); - conn.outstandingPackets.emplace_back(OutstandingPacket( - std::move(regularPacket), Clock::now(), 1, false, false, 1)); + conn.outstandingPackets.emplace_back( + OutstandingPacket(std::move(regularPacket), Clock::now(), 1, false, 1)); ReadAckFrame ackFrame; ackFrame.largestAcked = 0; ackFrame.ackBlocks.emplace_back(0, 0); @@ -639,7 +568,7 @@ TEST_P(AckHandlersTest, SkipAckVisitor) { WriteStreamFrame frame(0, 0, 0, true); regularPacket.frames.emplace_back(std::move(frame)); OutstandingPacket outstandingPacket( - std::move(regularPacket), Clock::now(), 1, false, false, 1); + std::move(regularPacket), Clock::now(), 1, false, 1); // Give this outstandingPacket an associatedEvent that's not in // outstandingPacketEvents outstandingPacket.associatedEvent = 0; @@ -681,11 +610,11 @@ TEST_P(AckHandlersTest, NoDoubleProcess) { regularPacket2.frames.push_back(frame); OutstandingPacket outstandingPacket1( - std::move(regularPacket1), Clock::now(), 1, false, false, 1); + std::move(regularPacket1), Clock::now(), 1, false, 1); outstandingPacket1.associatedEvent = packetNum1; OutstandingPacket outstandingPacket2( - std::move(regularPacket2), Clock::now(), 1, false, false, 1); + std::move(regularPacket2), Clock::now(), 1, false, 1); // The seconds packet has the same PacketEvent outstandingPacket2.associatedEvent = packetNum1; @@ -743,7 +672,7 @@ TEST_P(AckHandlersTest, ClonedPacketsCounter) { auto regularPacket1 = createNewPacket(packetNum1, GetParam()); regularPacket1.frames.push_back(frame); OutstandingPacket outstandingPacket1( - std::move(regularPacket1), Clock::now(), 1, false, false, 1); + std::move(regularPacket1), Clock::now(), 1, false, 1); outstandingPacket1.associatedEvent = packetNum1; conn.ackStates.appDataAckState.nextPacketNum++; @@ -751,7 +680,7 @@ TEST_P(AckHandlersTest, ClonedPacketsCounter) { auto regularPacket2 = createNewPacket(packetNum2, GetParam()); regularPacket2.frames.push_back(frame); OutstandingPacket outstandingPacket2( - std::move(regularPacket2), Clock::now(), 1, false, false, 1); + std::move(regularPacket2), Clock::now(), 1, false, 1); conn.outstandingPackets.push_back(std::move(outstandingPacket1)); conn.outstandingPackets.push_back(std::move(outstandingPacket2)); @@ -789,8 +718,8 @@ TEST_P(AckHandlersTest, UpdateMaxAckDelay) { PacketNum packetNum = 0; auto regularPacket = createNewPacket(packetNum, GetParam()); auto sentTime = Clock::now(); - conn.outstandingPackets.emplace_back(OutstandingPacket( - std::move(regularPacket), sentTime, 1, false, false, 1)); + conn.outstandingPackets.emplace_back( + OutstandingPacket(std::move(regularPacket), sentTime, 1, false, 1)); ReadAckFrame ackFrame; // ackDelay has no effect on mrtt @@ -843,7 +772,6 @@ TEST_P(AckHandlersTest, AckNotOutstandingButLoss) { Clock::now() - delayUntilLost - 20ms, 1, false, - false, 1); conn.outstandingPackets.push_back(std::move(outstandingPacket)); conn.outstandingClonedPacketsCount++; @@ -895,7 +823,6 @@ TEST_P(AckHandlersTest, UpdatePendingAckStates) { sentTime, 111, false, - false, conn.lossState.totalBytesSent + 111)); conn.lossState.totalBytesSent += 111; @@ -918,65 +845,6 @@ TEST_P(AckHandlersTest, UpdatePendingAckStates) { EXPECT_EQ(111 + 1357, conn.lossState.totalBytesAcked); } -TEST_F(AckHandlersTest, PureAckDoesNotUpdateRtt) { - QuicServerConnectionState conn; - conn.congestionController = nullptr; - PacketNum packetNum = 0; - auto regularPacket = createNewPacket(packetNum, PacketNumberSpace::AppData); - conn.outstandingPackets.emplace_back(OutstandingPacket( - std::move(regularPacket), - Clock::now() - 200ms, - 111, - false /* handshake */, - true /* pureAck */, - 111)); - conn.outstandingPureAckPacketsCount++; - ASSERT_FALSE(conn.outstandingPackets.empty()); - ReadAckFrame ackFrame; - ackFrame.largestAcked = packetNum; - ackFrame.ackDelay = 100us; - ackFrame.ackBlocks.emplace_back(packetNum, packetNum); - processAckFrame( - conn, - PacketNumberSpace::AppData, - ackFrame, - [&](const auto&, const auto&, const auto&) {}, - [&](auto&, auto&, bool, auto) {}, - Clock::now() - 150ms); - EXPECT_EQ(std::chrono::microseconds::max(), conn.lossState.mrtt); - EXPECT_EQ(0us, conn.lossState.srtt); - EXPECT_EQ(0us, conn.lossState.lrtt); - EXPECT_EQ(0us, conn.lossState.rttvar); - EXPECT_TRUE(conn.outstandingPackets.empty()); - - packetNum++; - regularPacket = createNewPacket(packetNum, PacketNumberSpace::AppData); - conn.outstandingPackets.emplace_back(OutstandingPacket( - std::move(regularPacket), - Clock::now() - 100ms, - 111, - false /* handshake */, - false /* pureAck */, - 111)); - ASSERT_FALSE(conn.outstandingPackets.empty()); - ackFrame.largestAcked = packetNum; - ackFrame.ackDelay = 100us; - ackFrame.ackBlocks.clear(); - ackFrame.ackBlocks.emplace_back(packetNum, packetNum); - processAckFrame( - conn, - PacketNumberSpace::AppData, - ackFrame, - [&](const auto&, const auto&, const auto&) {}, - [&](auto&, auto&, bool, auto) {}, - Clock::now()); - EXPECT_NE(std::chrono::microseconds::max(), conn.lossState.mrtt); - EXPECT_NE(0us, conn.lossState.srtt); - EXPECT_NE(0us, conn.lossState.lrtt); - EXPECT_NE(0us, conn.lossState.rttvar); - EXPECT_TRUE(conn.outstandingPackets.empty()); -} - TEST_P(AckHandlersTest, AckEventCreation) { QuicServerConnectionState conn; auto mockCongestionController = std::make_unique(); @@ -997,13 +865,11 @@ TEST_P(AckHandlersTest, AckEventCreation) { largestSentTime, 1, false /* handshake */, - (packetNum % 2) /* pureAck */, packetNum); sentPacket.isAppLimited = (packetNum % 2); conn.outstandingPackets.emplace_back(sentPacket); packetNum++; } - conn.outstandingPureAckPacketsCount = 5; ReadAckFrame ackFrame; ackFrame.largestAcked = 9; @@ -1015,7 +881,7 @@ TEST_P(AckHandlersTest, AckEventCreation) { EXPECT_EQ(ackTime, ack->ackTime); EXPECT_EQ(9, ack->largestAckedPacket.value()); EXPECT_EQ(largestSentTime, ack->largestAckedPacketSentTime); - EXPECT_EQ(5, ack->ackedBytes); + EXPECT_EQ(10, ack->ackedBytes); EXPECT_TRUE(ack->largestAckedPacketAppLimited); EXPECT_EQ( std::chrono::duration_cast( diff --git a/quic/state/test/QuicStateFunctionsTest.cpp b/quic/state/test/QuicStateFunctionsTest.cpp index 0ef828b0b..a7acb77d4 100644 --- a/quic/state/test/QuicStateFunctionsTest.cpp +++ b/quic/state/test/QuicStateFunctionsTest.cpp @@ -482,26 +482,23 @@ TEST_F(QuicStateFunctionsTest, GetOutstandingPackets) { Clock::now(), 135, false, - false, 0); conn.outstandingPackets.emplace_back( makeTestLongPacket(LongHeader::Types::Handshake), Clock::now(), 1217, false, - false, 0); conn.outstandingPackets.emplace_back( - makeTestShortPacket(), Clock::now(), 5556, false, false, 0); + makeTestShortPacket(), Clock::now(), 5556, false, 0); conn.outstandingPackets.emplace_back( makeTestLongPacket(LongHeader::Types::Initial), Clock::now(), 56, false, - false, 0); conn.outstandingPackets.emplace_back( - makeTestShortPacket(), Clock::now(), 6665, false, false, 0); + makeTestShortPacket(), Clock::now(), 6665, false, 0); EXPECT_EQ( 135, getFirstOutstandingPacket(conn, PacketNumberSpace::Initial)->encodedSize); diff --git a/quic/state/test/StateDataTest.cpp b/quic/state/test/StateDataTest.cpp index 39442bb2f..4e3940cd1 100644 --- a/quic/state/test/StateDataTest.cpp +++ b/quic/state/test/StateDataTest.cpp @@ -35,8 +35,7 @@ TEST_F(StateDataTest, SingleLostPacketEvent) { getTestConnectionId(), 100, kVersion)); - OutstandingPacket outstandingPacket( - packet, Clock::now(), 1234, false, false, 1234); + OutstandingPacket outstandingPacket(packet, Clock::now(), 1234, false, 1234); CongestionController::LossEvent loss; loss.addLostPacket(outstandingPacket); EXPECT_EQ(1234, loss.lostBytes); @@ -51,7 +50,7 @@ TEST_F(StateDataTest, MultipleLostPacketsEvent) { 100, kVersion)); OutstandingPacket outstandingPacket1( - packet1, Clock::now(), 1234, false, false, 1234); + packet1, Clock::now(), 1234, false, 1234); RegularQuicWritePacket packet2(LongHeader( LongHeader::Types::Initial, @@ -60,7 +59,7 @@ TEST_F(StateDataTest, MultipleLostPacketsEvent) { 110, kVersion)); OutstandingPacket outstandingPacket2( - packet2, Clock::now(), 1357, false, false, 1357); + packet2, Clock::now(), 1357, false, 1357); CongestionController::LossEvent loss; loss.addLostPacket(outstandingPacket1);