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);