diff --git a/quic/api/QuicPacketScheduler.cpp b/quic/api/QuicPacketScheduler.cpp index f56eb0604..a97679bb9 100644 --- a/quic/api/QuicPacketScheduler.cpp +++ b/quic/api/QuicPacketScheduler.cpp @@ -557,14 +557,14 @@ SchedulingResult CloningScheduler::scheduleFramesForPacket( internalBuilder = std::make_unique( conn_.udpSendPacketLen - cipherOverhead_, header, - getAckState(conn_, builderPnSpace).largestAckedByPeer); + getAckState(conn_, builderPnSpace).largestAckedByPeer.value_or(0)); } else { CHECK(conn_.bufAccessor && conn_.bufAccessor->ownsBuffer()); internalBuilder = std::make_unique( *conn_.bufAccessor, conn_.udpSendPacketLen - cipherOverhead_, header, - getAckState(conn_, builderPnSpace).largestAckedByPeer); + getAckState(conn_, builderPnSpace).largestAckedByPeer.value_or(0)); } // We shouldn't clone Handshake packet. if (iter->isHandshake) { diff --git a/quic/api/QuicSocket.h b/quic/api/QuicSocket.h index c3960ccbd..9b2208bb7 100644 --- a/quic/api/QuicSocket.h +++ b/quic/api/QuicSocket.h @@ -111,8 +111,8 @@ class QuicSocket { uint64_t totalBytesRetransmitted{0}; uint32_t ptoCount{0}; uint32_t totalPTOCount{0}; - PacketNum largestPacketAckedByPeer{0}; - PacketNum largestPacketSent{0}; + folly::Optional largestPacketAckedByPeer; + folly::Optional largestPacketSent; }; /** diff --git a/quic/api/QuicTransportFunctions.cpp b/quic/api/QuicTransportFunctions.cpp index d1f364a57..24c75810b 100644 --- a/quic/api/QuicTransportFunctions.cpp +++ b/quic/api/QuicTransportFunctions.cpp @@ -207,7 +207,7 @@ DataPathResult continuousMemoryBuildScheduleEncrypt( *connection.bufAccessor, connection.udpSendPacketLen, std::move(header), - getAckState(connection, pnSpace).largestAckedByPeer); + getAckState(connection, pnSpace).largestAckedByPeer.value_or(0)); pktBuilder.setCipherOverhead(cipherOverhead); CHECK(scheduler.hasData()); auto result = @@ -287,7 +287,7 @@ DataPathResult iobufChainBasedBuildScheduleEncrypt( RegularQuicPacketBuilder pktBuilder( connection.udpSendPacketLen, std::move(header), - getAckState(connection, pnSpace).largestAckedByPeer); + getAckState(connection, pnSpace).largestAckedByPeer.value_or(0)); // It's the scheduler's job to invoke encode header pktBuilder.setCipherOverhead(cipherOverhead); auto result = @@ -605,7 +605,8 @@ void updateConnection( } increaseNextPacketNum(conn, packetNumberSpace); - conn.lossState.largestSent = std::max(conn.lossState.largestSent, packetNum); + conn.lossState.largestSent = + std::max(conn.lossState.largestSent.value_or(packetNum), packetNum); // updateConnection may be called multiple times during write. If before or // during any updateConnection, setLossDetectionAlarm is already set, we // shouldn't clear it: @@ -903,7 +904,7 @@ void writeCloseCommon( RegularQuicPacketBuilder packetBuilder( connection.udpSendPacketLen, std::move(header), - getAckState(connection, pnSpace).largestAckedByPeer); + getAckState(connection, pnSpace).largestAckedByPeer.value_or(0)); packetBuilder.encodePacketHeader(); packetBuilder.setCipherOverhead(aead.getCipherOverhead()); size_t written = 0; diff --git a/quic/api/test/QuicPacketSchedulerTest.cpp b/quic/api/test/QuicPacketSchedulerTest.cpp index 923930dae..3b06df72c 100644 --- a/quic/api/test/QuicPacketSchedulerTest.cpp +++ b/quic/api/test/QuicPacketSchedulerTest.cpp @@ -97,7 +97,7 @@ TEST_F(QuicPacketSchedulerTest, CryptoPaddingInitialPacket) { RegularQuicPacketBuilder builder( conn.udpSendPacketLen, std::move(longHeader), - conn.ackStates.initialAckState.largestAckedByPeer); + conn.ackStates.initialAckState.largestAckedByPeer.value_or(0)); FrameScheduler cryptoOnlyScheduler = std::move( FrameScheduler::Builder( @@ -129,7 +129,7 @@ TEST_F(QuicPacketSchedulerTest, PaddingInitialPureAcks) { RegularQuicPacketBuilder builder( conn.udpSendPacketLen, std::move(longHeader), - conn.ackStates.handshakeAckState.largestAckedByPeer); + conn.ackStates.handshakeAckState.largestAckedByPeer.value_or(0)); conn.ackStates.initialAckState.largestRecvdPacketTime = Clock::now(); conn.ackStates.initialAckState.needsToSendAckImmediately = true; conn.ackStates.initialAckState.acks.insert(10); @@ -163,7 +163,7 @@ TEST_F(QuicPacketSchedulerTest, PaddingUpToWrapperSize) { RegularQuicPacketBuilder builder( conn.udpSendPacketLen, std::move(longHeader), - conn.ackStates.handshakeAckState.largestAckedByPeer); + conn.ackStates.handshakeAckState.largestAckedByPeer.value_or(0)); conn.ackStates.initialAckState.largestRecvdPacketTime = Clock::now(); conn.ackStates.initialAckState.needsToSendAckImmediately = true; conn.ackStates.initialAckState.acks.insert(10); @@ -196,7 +196,7 @@ TEST_F(QuicPacketSchedulerTest, CryptoServerInitialPadded) { RegularQuicPacketBuilder builder1( conn.udpSendPacketLen, std::move(longHeader1), - conn.ackStates.initialAckState.largestAckedByPeer); + conn.ackStates.initialAckState.largestAckedByPeer.value_or(0)); FrameScheduler scheduler = std::move( FrameScheduler::Builder( @@ -228,7 +228,7 @@ TEST_F(QuicPacketSchedulerTest, PadTwoInitialPackets) { RegularQuicPacketBuilder builder1( conn.udpSendPacketLen, std::move(longHeader1), - conn.ackStates.initialAckState.largestAckedByPeer); + conn.ackStates.initialAckState.largestAckedByPeer.value_or(0)); FrameScheduler scheduler = std::move( FrameScheduler::Builder( @@ -256,7 +256,7 @@ TEST_F(QuicPacketSchedulerTest, PadTwoInitialPackets) { RegularQuicPacketBuilder builder2( conn.udpSendPacketLen, std::move(longHeader2), - conn.ackStates.initialAckState.largestAckedByPeer); + conn.ackStates.initialAckState.largestAckedByPeer.value_or(0)); writeDataToQuicStream( conn.cryptoState->initialStream, folly::IOBuf::copyBuffer("shlo again")); auto result2 = scheduler.scheduleFramesForPacket( @@ -279,7 +279,7 @@ TEST_F(QuicPacketSchedulerTest, CryptoPaddingRetransmissionClientInitial) { RegularQuicPacketBuilder builder( conn.udpSendPacketLen, std::move(longHeader), - conn.ackStates.initialAckState.largestAckedByPeer); + conn.ackStates.initialAckState.largestAckedByPeer.value_or(0)); FrameScheduler scheduler = std::move( FrameScheduler::Builder( @@ -310,7 +310,7 @@ TEST_F(QuicPacketSchedulerTest, CryptoSchedulerOnlySingleLossFits) { RegularQuicPacketBuilder builder( conn.udpSendPacketLen, std::move(longHeader), - conn.ackStates.handshakeAckState.largestAckedByPeer); + conn.ackStates.handshakeAckState.largestAckedByPeer.value_or(0)); builder.encodePacketHeader(); PacketBuilderWrapper builderWrapper(builder, 13); CryptoStreamScheduler scheduler( @@ -338,7 +338,7 @@ TEST_F(QuicPacketSchedulerTest, CryptoWritePartialLossBuffer) { RegularQuicPacketBuilder builder( 25, std::move(longHeader), - conn.ackStates.initialAckState.largestAckedByPeer); + conn.ackStates.initialAckState.largestAckedByPeer.value_or(0)); FrameScheduler cryptoOnlyScheduler = std::move( FrameScheduler::Builder( @@ -375,7 +375,7 @@ TEST_F(QuicPacketSchedulerTest, StreamFrameSchedulerExists) { RegularQuicPacketBuilder builder( conn.udpSendPacketLen, std::move(shortHeader), - conn.ackStates.appDataAckState.largestAckedByPeer); + conn.ackStates.appDataAckState.largestAckedByPeer.value_or(0)); builder.encodePacketHeader(); auto originalSpace = builder.remainingSpaceInPkt(); conn.streamManager->queueWindowUpdate(stream->id); @@ -397,7 +397,7 @@ TEST_F(QuicPacketSchedulerTest, StreamFrameNoSpace) { RegularQuicPacketBuilder builder( conn.udpSendPacketLen, std::move(shortHeader), - conn.ackStates.appDataAckState.largestAckedByPeer); + conn.ackStates.appDataAckState.largestAckedByPeer.value_or(0)); builder.encodePacketHeader(); PacketBuilderWrapper builderWrapper(builder, 2); auto originalSpace = builder.remainingSpaceInPkt(); @@ -419,7 +419,7 @@ TEST_F(QuicPacketSchedulerTest, StreamFrameSchedulerStreamNotExists) { RegularQuicPacketBuilder builder( conn.udpSendPacketLen, std::move(shortHeader), - conn.ackStates.appDataAckState.largestAckedByPeer); + conn.ackStates.appDataAckState.largestAckedByPeer.value_or(0)); builder.encodePacketHeader(); auto originalSpace = builder.remainingSpaceInPkt(); conn.streamManager->queueWindowUpdate(nonExistentStream); @@ -448,7 +448,7 @@ TEST_F(QuicPacketSchedulerTest, CloningSchedulerTest) { RegularQuicPacketBuilder builder( conn.udpSendPacketLen, std::move(header), - conn.ackStates.appDataAckState.largestAckedByPeer); + conn.ackStates.appDataAckState.largestAckedByPeer.value_or(0)); auto result = cloningScheduler.scheduleFramesForPacket( std::move(builder), kDefaultUDPSendPacketLen); EXPECT_TRUE(result.packetEvent.has_value() && result.packet.has_value()); @@ -479,7 +479,7 @@ TEST_F(QuicPacketSchedulerTest, DoNotCloneProcessedClonedPacket) { RegularQuicPacketBuilder builder( conn.udpSendPacketLen, std::move(header), - conn.ackStates.initialAckState.largestAckedByPeer); + conn.ackStates.initialAckState.largestAckedByPeer.value_or(0)); auto result = cloningScheduler.scheduleFramesForPacket( std::move(builder), kDefaultUDPSendPacketLen); EXPECT_TRUE(result.packetEvent.has_value() && result.packet.has_value()); @@ -524,7 +524,7 @@ TEST_F(QuicPacketSchedulerTest, DoNotCloneHandshake) { RegularQuicPacketBuilder builder( conn.udpSendPacketLen, std::move(header), - conn.ackStates.appDataAckState.largestAckedByPeer); + conn.ackStates.appDataAckState.largestAckedByPeer.value_or(0)); auto result = cloningScheduler.scheduleFramesForPacket( std::move(builder), kDefaultUDPSendPacketLen); EXPECT_TRUE(result.packetEvent.has_value() && result.packet.has_value()); @@ -557,7 +557,7 @@ TEST_F(QuicPacketSchedulerTest, CloneSchedulerUseNormalSchedulerFirst) { RegularQuicPacketBuilder builder( conn.udpSendPacketLen, std::move(header), - conn.ackStates.appDataAckState.largestAckedByPeer); + conn.ackStates.appDataAckState.largestAckedByPeer.value_or(0)); auto result = cloningScheduler.scheduleFramesForPacket( std::move(builder), kDefaultUDPSendPacketLen); EXPECT_EQ(folly::none, result.packetEvent); @@ -606,7 +606,7 @@ TEST_F(QuicPacketSchedulerTest, CloneWillGenerateNewWindowUpdate) { RegularQuicPacketBuilder builder( conn.udpSendPacketLen, std::move(header), - conn.ackStates.appDataAckState.largestAckedByPeer); + conn.ackStates.appDataAckState.largestAckedByPeer.value_or(0)); auto packetResult = cloningScheduler.scheduleFramesForPacket( std::move(builder), conn.udpSendPacketLen); EXPECT_EQ(expectedPacketEvent, *packetResult.packetEvent); @@ -687,7 +687,7 @@ TEST_F(QuicPacketSchedulerTest, CloningSchedulerWithInplaceBuilder) { bufAccessor, conn.udpSendPacketLen, std::move(header), - conn.ackStates.appDataAckState.largestAckedByPeer); + conn.ackStates.appDataAckState.largestAckedByPeer.value_or(0)); auto result = cloningScheduler.scheduleFramesForPacket( std::move(builder), kDefaultUDPSendPacketLen); EXPECT_TRUE(result.packetEvent.has_value() && result.packet.has_value()); @@ -731,7 +731,7 @@ TEST_F(QuicPacketSchedulerTest, CloningSchedulerWithInplaceBuilderFullPacket) { bufAccessor, conn.udpSendPacketLen, std::move(header), - conn.ackStates.appDataAckState.largestAckedByPeer); + conn.ackStates.appDataAckState.largestAckedByPeer.value_or(0)); ASSERT_TRUE(scheduler.hasData()); auto result = scheduler.scheduleFramesForPacket( std::move(builder), conn.udpSendPacketLen); @@ -759,7 +759,7 @@ TEST_F(QuicPacketSchedulerTest, CloningSchedulerWithInplaceBuilderFullPacket) { bufAccessor, conn.udpSendPacketLen, std::move(dupHeader), - conn.ackStates.appDataAckState.largestAckedByPeer); + conn.ackStates.appDataAckState.largestAckedByPeer.value_or(0)); auto cloneResult = cloningScheduler.scheduleFramesForPacket( std::move(internalBuilder), conn.udpSendPacketLen); EXPECT_TRUE( @@ -798,7 +798,7 @@ TEST_F(QuicPacketSchedulerTest, CloneLargerThanOriginalPacket) { RegularQuicPacketBuilder builder( conn.udpSendPacketLen, std::move(header), - conn.ackStates.appDataAckState.largestAckedByPeer); + conn.ackStates.appDataAckState.largestAckedByPeer.value_or(0)); auto packetResult = scheduler.scheduleFramesForPacket( std::move(builder), conn.udpSendPacketLen - cipherOverhead); auto encodedSize = packetResult.packet->body->computeChainDataLength() + @@ -820,7 +820,7 @@ TEST_F(QuicPacketSchedulerTest, CloneLargerThanOriginalPacket) { RegularQuicPacketBuilder throwawayBuilder( conn.udpSendPacketLen, std::move(cloneHeader), - conn.ackStates.appDataAckState.largestAckedByPeer); + conn.ackStates.appDataAckState.largestAckedByPeer.value_or(0)); FrameScheduler noopScheduler("noopScheduler"); CloningScheduler cloningScheduler( noopScheduler, conn, "CopyCat", cipherOverhead); @@ -905,7 +905,7 @@ TEST_F(QuicPacketSchedulerTest, StreamFrameSchedulerAllFit) { RegularQuicPacketBuilder builder( conn.udpSendPacketLen, std::move(shortHeader), - conn.ackStates.appDataAckState.largestAckedByPeer); + conn.ackStates.appDataAckState.largestAckedByPeer.value_or(0)); builder.encodePacketHeader(); auto stream1 = conn.streamManager->createNextBidirectionalStream().value()->id; @@ -944,7 +944,7 @@ TEST_F(QuicPacketSchedulerTest, StreamFrameSchedulerRoundRobin) { RegularQuicPacketBuilder builder( conn.udpSendPacketLen, std::move(shortHeader1), - conn.ackStates.appDataAckState.largestAckedByPeer); + conn.ackStates.appDataAckState.largestAckedByPeer.value_or(0)); builder.encodePacketHeader(); auto stream1 = conn.streamManager->createNextBidirectionalStream().value()->id; @@ -1010,7 +1010,7 @@ TEST_F(QuicPacketSchedulerTest, StreamFrameSchedulerRoundRobinStreamPerPacket) { RegularQuicPacketBuilder builder1( conn.udpSendPacketLen, std::move(shortHeader1), - conn.ackStates.appDataAckState.largestAckedByPeer); + conn.ackStates.appDataAckState.largestAckedByPeer.value_or(0)); auto stream1 = conn.streamManager->createNextBidirectionalStream().value()->id; auto stream2 = @@ -1078,7 +1078,7 @@ TEST_F(QuicPacketSchedulerTest, StreamFrameSchedulerRoundRobinControl) { RegularQuicPacketBuilder builder( conn.udpSendPacketLen, std::move(shortHeader1), - conn.ackStates.appDataAckState.largestAckedByPeer); + conn.ackStates.appDataAckState.largestAckedByPeer.value_or(0)); builder.encodePacketHeader(); auto stream1 = conn.streamManager->createNextBidirectionalStream().value()->id; @@ -1160,7 +1160,7 @@ TEST_F(QuicPacketSchedulerTest, StreamFrameSchedulerOneStream) { RegularQuicPacketBuilder builder( conn.udpSendPacketLen, std::move(shortHeader), - conn.ackStates.appDataAckState.largestAckedByPeer); + conn.ackStates.appDataAckState.largestAckedByPeer.value_or(0)); builder.encodePacketHeader(); auto stream1 = conn.streamManager->createNextBidirectionalStream().value(); writeDataToQuicStream(*stream1, folly::IOBuf::copyBuffer("some data"), false); @@ -1242,7 +1242,7 @@ TEST_F( bufAccessor, conn.udpSendPacketLen, std::move(header), - conn.ackStates.appDataAckState.largestAckedByPeer); + conn.ackStates.appDataAckState.largestAckedByPeer.value_or(0)); auto result = cloningScheduler.scheduleFramesForPacket( std::move(builder), kDefaultUDPSendPacketLen); EXPECT_FALSE(result.packetEvent.has_value()); @@ -1282,7 +1282,7 @@ TEST_F( bufAccessor, conn.udpSendPacketLen, std::move(header), - conn.ackStates.appDataAckState.largestAckedByPeer); + conn.ackStates.appDataAckState.largestAckedByPeer.value_or(0)); auto result = cloningScheduler.scheduleFramesForPacket( std::move(builder), kDefaultUDPSendPacketLen); EXPECT_FALSE(result.packetEvent.has_value()); diff --git a/quic/api/test/QuicTransportFunctionsTest.cpp b/quic/api/test/QuicTransportFunctionsTest.cpp index 4387f8c4a..df5af5c69 100644 --- a/quic/api/test/QuicTransportFunctionsTest.cpp +++ b/quic/api/test/QuicTransportFunctionsTest.cpp @@ -120,7 +120,7 @@ auto buildEmptyPacket( RegularQuicPacketBuilder builder( conn.udpSendPacketLen, std::move(*header), - getAckState(conn, pnSpace).largestAckedByPeer); + getAckState(conn, pnSpace).largestAckedByPeer.value_or(0)); builder.encodePacketHeader(); DCHECK(builder.canBuildPacket()); return std::move(builder).buildPacket(); diff --git a/quic/common/test/TestUtils.cpp b/quic/common/test/TestUtils.cpp index 6f7647d8f..871503038 100644 --- a/quic/common/test/TestUtils.cpp +++ b/quic/common/test/TestUtils.cpp @@ -119,7 +119,7 @@ RegularQuicPacketBuilder::Packet createAckPacket( RegularQuicPacketBuilder builder( dstConn.udpSendPacketLen, std::move(*header), - getAckState(dstConn, pnSpace).largestAckedByPeer); + getAckState(dstConn, pnSpace).largestAckedByPeer.value_or(0)); builder.encodePacketHeader(); if (aead) { builder.setCipherOverhead(aead->getCipherOverhead()); diff --git a/quic/congestion_control/test/BbrBandwidthSamplerTest.cpp b/quic/congestion_control/test/BbrBandwidthSamplerTest.cpp index 886700d37..cd8fc1a1f 100644 --- a/quic/congestion_control/test/BbrBandwidthSamplerTest.cpp +++ b/quic/congestion_control/test/BbrBandwidthSamplerTest.cpp @@ -144,7 +144,8 @@ TEST_F(BbrBandwidthSamplerTest, AppLimited) { sampler.onAppLimited(); EXPECT_TRUE(sampler.isAppLimited()); CongestionController::AckEvent ackEvent; - ackEvent.largestAckedPacket = ++conn.lossState.largestSent; + conn.lossState.largestSent = conn.lossState.largestSent.value_or(0); + ackEvent.largestAckedPacket = ++conn.lossState.largestSent.value(); auto packet = makeTestingWritePacket(*ackEvent.largestAckedPacket, 1000, 1000); ackEvent.largestAckedPacketSentTime = packet.time; diff --git a/quic/congestion_control/test/BbrTest.cpp b/quic/congestion_control/test/BbrTest.cpp index c297abe51..e44ac05c5 100644 --- a/quic/congestion_control/test/BbrTest.cpp +++ b/quic/congestion_control/test/BbrTest.cpp @@ -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); + conn.lossState.largestSent.value(), 1000, 1000 + totalSent); bbr.onPacketSent(packet); totalSent += 1000; EXPECT_CALL(*rawBandwidthSampler, getBandwidth()) @@ -231,7 +231,7 @@ TEST_F(BbrTest, ProbeRtt) { auto sendFunc = [&]() { conn.lossState.largestSent = currentLatest; auto packet = makeTestingWritePacket( - conn.lossState.largestSent, + conn.lossState.largestSent.value(), conn.udpSendPacketLen, totalSent + conn.udpSendPacketLen); bbr.onPacketSent(packet); @@ -396,7 +396,7 @@ TEST_F(BbrTest, AckAggregation) { auto sendAckGrow = [&](bool growFast) { conn.lossState.largestSent = currentLatest; auto packet = makeTestingWritePacket( - conn.lossState.largestSent, 1000, 1000 + totalSent); + conn.lossState.largestSent.value(), 1000, 1000 + totalSent); bbr.onPacketSent(packet); totalSent += 1000; EXPECT_CALL(*rawBandwidthSampler, getBandwidth()) @@ -437,7 +437,7 @@ TEST_F(BbrTest, AckAggregation) { conn.lossState.largestSent = currentLatest; auto packet = makeTestingWritePacket( - conn.lossState.largestSent, 1000, 1000 + totalSent); + conn.lossState.largestSent.value(), 1000, 1000 + totalSent); bbr.onPacketSent(packet); totalSent += 1000; auto ackEvent = makeAck(currentLatest, 1000, Clock::now(), packet.time); @@ -460,7 +460,7 @@ TEST_F(BbrTest, AckAggregation) { // Another round, this time send something larger than currentMaxAckHeight: conn.lossState.largestSent = currentLatest; auto packet1 = makeTestingWritePacket( - conn.lossState.largestSent, + conn.lossState.largestSent.value(), currentMaxAckHeight * 2 + 100, currentMaxAckHeight * 2 + 100 + totalSent); bbr.onPacketSent(packet1); @@ -481,9 +481,10 @@ TEST_F(BbrTest, AppLimited) { auto rawBandwidthSampler = mockBandwidthSampler.get(); bbr.setBandwidthSampler(std::move(mockBandwidthSampler)); - auto packet = makeTestingWritePacket(conn.lossState.largestSent, 1000, 1000); + auto packet = makeTestingWritePacket( + conn.lossState.largestSent.value_or(0), 1000, 1000); bbr.onPacketSent(packet); - conn.lossState.largestSent++; + conn.lossState.largestSent = conn.lossState.largestSent.value_or(0) + 1; EXPECT_CALL(*rawBandwidthSampler, onAppLimited()).Times(1); bbr.setAppLimited(); EXPECT_CALL(*rawBandwidthSampler, isAppLimited()) @@ -504,7 +505,8 @@ TEST_F(BbrTest, AppLimitedIgnored) { EXPECT_CALL(*rawBandwidthSampler, onAppLimited()).Times(1); bbr.setAppLimited(); auto packet = makeTestingWritePacket( - conn.lossState.largestSent++, 1000, 1000 + inflightBytes); + conn.lossState.largestSent.value_or(0), 1000, 1000 + inflightBytes); + conn.lossState.largestSent = conn.lossState.largestSent.value_or(0) + 1; inflightBytes += 1000; bbr.onPacketSent(packet); } @@ -526,11 +528,16 @@ TEST_F(BbrTest, ExtendMinRttExpiration) { EXPECT_CALL(*rawBandwidthSampler, onAppLimited()).Times(1); bbr.setAppLimited(); - auto packet = makeTestingWritePacket(conn.lossState.largestSent, 1000, 1000); + auto packet = makeTestingWritePacket( + conn.lossState.largestSent.value_or(0), 1000, 1000); bbr.onPacketSent(packet); EXPECT_CALL(*rawRttSampler, timestampMinRtt(_)).Times(1); bbr.onPacketAckOrLoss( - makeAck(conn.lossState.largestSent, 1000, Clock::now(), packet.time), + makeAck( + conn.lossState.largestSent.value_or(0), + 1000, + Clock::now(), + packet.time), folly::none); } diff --git a/quic/loss/QuicLossFunctions.cpp b/quic/loss/QuicLossFunctions.cpp index dd6b905a7..4efffd5db 100644 --- a/quic/loss/QuicLossFunctions.cpp +++ b/quic/loss/QuicLossFunctions.cpp @@ -36,7 +36,7 @@ void onPTOAlarm(QuicConnectionStateBase& conn) { QUIC_TRACE( pto_alarm, conn, - conn.lossState.largestSent, + conn.lossState.largestSent.value_or(0), conn.lossState.ptoCount, (uint64_t)conn.outstandingPackets.size()); QUIC_STATS(conn.statsCallback, onPTO); @@ -44,7 +44,7 @@ void onPTOAlarm(QuicConnectionStateBase& conn) { conn.lossState.totalPTOCount++; if (conn.qLogger) { conn.qLogger->addLossAlarm( - conn.lossState.largestSent, + conn.lossState.largestSent.value_or(0), conn.lossState.ptoCount, (uint64_t)conn.outstandingPackets.size(), kPtoAlarm); diff --git a/quic/loss/QuicLossFunctions.h b/quic/loss/QuicLossFunctions.h index cc6c5a43e..b39d4be3e 100644 --- a/quic/loss/QuicLossFunctions.h +++ b/quic/loss/QuicLossFunctions.h @@ -189,7 +189,7 @@ void setLossDetectionAlarm(QuicConnectionStateBase& conn, Timeout& timeout) { template folly::Optional detectLossPackets( QuicConnectionStateBase& conn, - PacketNum largestAcked, + folly::Optional largestAcked, const LossVisitor& lossVisitor, TimePoint lossTime, PacketNumberSpace pnSpace) { @@ -199,7 +199,7 @@ folly::Optional detectLossPackets( conn.transportSettings.timeReorderingThreshDividend / conn.transportSettings.timeReorderingThreshDivisor; VLOG(10) << __func__ << " outstanding=" << conn.outstandingPackets.size() - << " largestAcked=" << largestAcked + << " largestAcked=" << largestAcked.value_or(0) << " delayUntilLost=" << delayUntilLost.count() << "us" << " " << conn; CongestionController::LossEvent lossEvent(lossTime); @@ -209,7 +209,7 @@ folly::Optional detectLossPackets( while (iter != conn.outstandingPackets.end()) { auto& pkt = *iter; auto currentPacketNum = pkt.packet.header.getPacketSequenceNum(); - if (currentPacketNum >= largestAcked) { + if (!largestAcked.has_value() || currentPacketNum >= *largestAcked) { break; } auto currentPacketNumberSpace = pkt.packet.header.getPacketNumberSpace(); @@ -219,7 +219,7 @@ folly::Optional detectLossPackets( } bool lost = (lossTime - pkt.time) > delayUntilLost; lost = lost || - (largestAcked - currentPacketNum) > conn.lossState.reorderingThreshold; + (*largestAcked - currentPacketNum) > conn.lossState.reorderingThreshold; if (!lost) { // We can exit early here because if packet N doesn't meet the // threshold, then packet N + 1 will not either. @@ -306,13 +306,13 @@ void onHandshakeAlarm( QUIC_TRACE( handshake_alarm, conn, - conn.lossState.largestSent, + conn.lossState.largestSent.value_or(0), conn.lossState.handshakeAlarmCount, (uint64_t)conn.outstandingHandshakePacketsCount, (uint64_t)conn.outstandingPackets.size()); if (conn.qLogger) { conn.qLogger->addLossAlarm( - conn.lossState.largestSent, + conn.lossState.largestSent.value_or(0), conn.lossState.handshakeAlarmCount, (uint64_t)conn.outstandingPackets.size(), kHandshakeAlarm); @@ -416,7 +416,9 @@ folly::Optional handleAckForLoss( // doesn't ack anything that's in OP list? conn.lossState.ptoCount = 0; conn.lossState.handshakeAlarmCount = 0; - largestAcked = std::max(largestAcked, *ack.largestAckedPacket); + largestAcked = std::max( + largestAcked.value_or(*ack.largestAckedPacket), + *ack.largestAckedPacket); } auto lossEvent = detectLossPackets( conn, diff --git a/quic/loss/test/QuicLossFunctionsTest.cpp b/quic/loss/test/QuicLossFunctionsTest.cpp index 3667df90e..dcd810efd 100644 --- a/quic/loss/test/QuicLossFunctionsTest.cpp +++ b/quic/loss/test/QuicLossFunctionsTest.cpp @@ -172,7 +172,7 @@ PacketNum QuicLossFunctionsTest::sendPacket( RegularQuicPacketBuilder builder( conn.udpSendPacketLen, std::move(*header), - getAckState(conn, packetNumberSpace).largestAckedByPeer); + getAckState(conn, packetNumberSpace).largestAckedByPeer.value_or(0)); builder.encodePacketHeader(); EXPECT_TRUE(builder.canBuildPacket()); auto packet = std::move(builder).buildPacket(); @@ -216,7 +216,7 @@ PacketNum QuicLossFunctionsTest::sendPacket( conn.lossState.largestSent = getNextPacketNum(conn, packetNumberSpace); increaseNextPacketNum(conn, packetNumberSpace); conn.pendingEvents.setLossDetectionAlarm = true; - return conn.lossState.largestSent; + return conn.lossState.largestSent.value(); } TEST_F(QuicLossFunctionsTest, AllPacketsProcessed) { @@ -713,9 +713,10 @@ TEST_F(QuicLossFunctionsTest, TestHandleAckedPacket) { sendPacket(*conn, TimePoint(), folly::none, PacketType::OneRtt); ReadAckFrame ackFrame; - ackFrame.largestAcked = conn->lossState.largestSent; + ackFrame.largestAcked = conn->lossState.largestSent.value_or(0); ackFrame.ackBlocks.emplace_back( - conn->lossState.largestSent, conn->lossState.largestSent); + conn->lossState.largestSent.value_or(0), + conn->lossState.largestSent.value_or(0)); bool testLossMarkFuncCalled = false; auto testLossMarkFunc = [&](auto& /* conn */, auto&, bool, PacketNum) { diff --git a/quic/server/test/QuicServerTransportTest.cpp b/quic/server/test/QuicServerTransportTest.cpp index c2fa72160..9eeb8b5d9 100644 --- a/quic/server/test/QuicServerTransportTest.cpp +++ b/quic/server/test/QuicServerTransportTest.cpp @@ -1720,7 +1720,8 @@ TEST_F(QuicServerTransportTest, StopSendingLoss) { RegularQuicPacketBuilder builder( server->getConn().udpSendPacketLen, std::move(header), - server->getConn().ackStates.appDataAckState.largestAckedByPeer); + server->getConn().ackStates.appDataAckState.largestAckedByPeer.value_or( + 0)); builder.encodePacketHeader(); StopSendingFrame stopSendingFrame( streamId, GenericApplicationErrorCode::UNKNOWN); @@ -1751,7 +1752,8 @@ TEST_F(QuicServerTransportTest, StopSendingLossAfterStreamClosed) { RegularQuicPacketBuilder builder( server->getConn().udpSendPacketLen, std::move(header), - server->getConn().ackStates.appDataAckState.largestAckedByPeer); + server->getConn().ackStates.appDataAckState.largestAckedByPeer.value_or( + 0)); builder.encodePacketHeader(); StopSendingFrame stopSendingFrame( streamId, GenericApplicationErrorCode::UNKNOWN); diff --git a/quic/state/AckStates.h b/quic/state/AckStates.h index ce88ea7e4..b3b59feca 100644 --- a/quic/state/AckStates.h +++ b/quic/state/AckStates.h @@ -31,8 +31,7 @@ struct AckState { // The receive time of the largest ack packet folly::Optional largestRecvdPacketTime; // Latest packet number acked by peer - // TODO: 0 is a legit PacketNum now, we need to make this optional: - PacketNum largestAckedByPeer{0}; + folly::Optional largestAckedByPeer; // Largest received packet numbers on the connection. folly::Optional largestReceivedPacketNum; // Largest received packet number at the time we sent our last close message. diff --git a/quic/state/StateData.h b/quic/state/StateData.h index 171569c2d..ab2d08dd3 100644 --- a/quic/state/StateData.h +++ b/quic/state/StateData.h @@ -447,9 +447,8 @@ struct LossState { // The time when last handshake packet was sent TimePoint lastHandshakePacketSentTime; // Latest packet number sent - // TODO: 0 is a legit PacketNum now, we need to make this optional: // TODO: this also needs to be 3 numbers now... - PacketNum largestSent{0}; + folly::Optional largestSent; // Reordering threshold used uint32_t reorderingThreshold{kReorderingThreshold}; // Timer for time reordering detection or early retransmit alarm. diff --git a/quic/state/test/AckHandlersTest.cpp b/quic/state/test/AckHandlersTest.cpp index 9b62bff26..1c5e16b48 100644 --- a/quic/state/test/AckHandlersTest.cpp +++ b/quic/state/test/AckHandlersTest.cpp @@ -400,7 +400,7 @@ TEST_P(AckHandlersTest, NoNewAckedPacket) { Clock::now()); EXPECT_TRUE(conn.pendingEvents.setLossDetectionAlarm); EXPECT_EQ(conn.lossState.ptoCount, 1); - EXPECT_EQ(conn.ackStates.appDataAckState.largestAckedByPeer, 0); + EXPECT_TRUE(!conn.ackStates.appDataAckState.largestAckedByPeer.has_value()); } TEST_P(AckHandlersTest, LossByAckedRecovered) {