diff --git a/quic/api/QuicPacketScheduler.cpp b/quic/api/QuicPacketScheduler.cpp index e7cb82435..325413d46 100644 --- a/quic/api/QuicPacketScheduler.cpp +++ b/quic/api/QuicPacketScheduler.cpp @@ -686,7 +686,10 @@ CloningScheduler::CloningScheduler( cipherOverhead_(cipherOverhead) {} bool CloningScheduler::hasData() const { - return frameScheduler_.hasData() || conn_.outstandings.numOutstanding() > 0; + // TODO: I'm not completely convinced d6d.outstandingProbes has been updated + // correctly. + return frameScheduler_.hasData() || + conn_.outstandings.numOutstanding() > conn_.d6d.outstandingProbes; } SchedulingResult CloningScheduler::scheduleFramesForPacket( diff --git a/quic/api/test/QuicPacketSchedulerTest.cpp b/quic/api/test/QuicPacketSchedulerTest.cpp index 25b85c2d3..ca28bc413 100644 --- a/quic/api/test/QuicPacketSchedulerTest.cpp +++ b/quic/api/test/QuicPacketSchedulerTest.cpp @@ -28,6 +28,45 @@ enum PacketBuilderType { Regular, Inplace }; namespace { +SchedulingResult sendD6DProbe( + QuicConnectionStateBase& conn, + uint64_t cipherOverhead, + uint32_t probeSize, + PacketBuilderType builderType) { + auto connId = quic::test::getTestConnectionId(); + D6DProbeScheduler d6dProbeScheduler( + conn, "d6d probe", cipherOverhead, probeSize); + EXPECT_TRUE(d6dProbeScheduler.hasData()); + + ShortHeader shortHeader( + ProtectionType::KeyPhaseZero, + connId, + getNextPacketNum(conn, PacketNumberSpace::AppData)); + if (builderType == PacketBuilderType::Regular) { + RegularQuicPacketBuilder builder( + conn.udpSendPacketLen, + std::move(shortHeader), + conn.ackStates.appDataAckState.largestAckedByPeer.value_or(0)); + auto result = d6dProbeScheduler.scheduleFramesForPacket( + std::move(builder), kDefaultUDPSendPacketLen); + EXPECT_FALSE(d6dProbeScheduler.hasData()); + return result; + } else { + // Just enough to build the probe + auto simpleBufAccessor = std::make_unique(probeSize); + InplaceQuicPacketBuilder builder( + *simpleBufAccessor, + conn.udpSendPacketLen, + std::move(shortHeader), + conn.ackStates.appDataAckState.largestAckedByPeer.value_or(0)); + auto result = d6dProbeScheduler.scheduleFramesForPacket( + std::move(builder), kDefaultUDPSendPacketLen); + EXPECT_FALSE(d6dProbeScheduler.hasData()); + return result; + } + folly::assume_unreachable(); +} + PacketNum addInitialOutstandingPacket(QuicConnectionStateBase& conn) { PacketNum nextPacketNum = getNextPacketNum(conn, PacketNumberSpace::Initial); std::vector zeroConnIdData(quic::kDefaultConnectionIdSize, 0); @@ -482,36 +521,39 @@ TEST_P(QuicPacketSchedulerTest, D6DProbeSchedulerTest) { connId, getNextPacketNum(conn, PacketNumberSpace::AppData)); auto param = GetParam(); - size_t packetSize = 0; - if (param == PacketBuilderType::Regular) { - RegularQuicPacketBuilder builder( - conn.udpSendPacketLen, - std::move(shortHeader), - conn.ackStates.appDataAckState.largestAckedByPeer.value_or(0)); - auto result = d6dProbeScheduler.scheduleFramesForPacket( - std::move(builder), kDefaultUDPSendPacketLen); - ASSERT_TRUE(result.packet.has_value()); - packetSize = result.packet->header->computeChainDataLength() + - result.packet->body->computeChainDataLength() + cipherOverhead; - } else { - // Just enough to build the probe - auto simpleBufAccessor = std::make_unique(probeSize); - InplaceQuicPacketBuilder builder( - *simpleBufAccessor, - conn.udpSendPacketLen, - std::move(shortHeader), - conn.ackStates.appDataAckState.largestAckedByPeer.value_or(0)); - auto result = d6dProbeScheduler.scheduleFramesForPacket( - std::move(builder), kDefaultUDPSendPacketLen); - ASSERT_TRUE(result.packet.has_value()); - packetSize = result.packet->header->computeChainDataLength() + - result.packet->body->computeChainDataLength() + cipherOverhead; - } - - EXPECT_FALSE(d6dProbeScheduler.hasData()); + auto result = sendD6DProbe(conn, cipherOverhead, probeSize, param); + ASSERT_TRUE(result.packet.has_value()); + auto packetSize = result.packet->header->computeChainDataLength() + + result.packet->body->computeChainDataLength() + cipherOverhead; EXPECT_EQ(packetSize, probeSize); } +TEST_P(QuicPacketSchedulerTest, NoCloningWithOnlyD6DProbes) { + QuicClientConnectionState conn( + FizzClientQuicHandshakeContext::Builder().build()); + uint64_t cipherOverhead = 2; + uint32_t probeSize = 1450; + auto param = GetParam(); + auto result = sendD6DProbe(conn, cipherOverhead, probeSize, param); + ASSERT_TRUE(result.packet.has_value()); + auto packetSize = result.packet->header->computeChainDataLength() + + result.packet->body->computeChainDataLength() + cipherOverhead; + updateConnection( + conn, + folly::none, + result.packet->packet, + Clock::now(), + packetSize, + result.packet->body->computeChainDataLength(), + false /* isDSRPacket */); + ASSERT_EQ(1, conn.outstandings.packets.size()); + EXPECT_TRUE(conn.outstandings.packets.back().metadata.isD6DProbe); + FrameScheduler noopScheduler("noopScheduler"); + CloningScheduler cloningScheduler( + noopScheduler, conn, "MarShall Mathers", cipherOverhead); + EXPECT_FALSE(cloningScheduler.hasData()); +} + TEST_F(QuicPacketSchedulerTest, WriteOnlyOutstandingPacketsTest) { QuicClientConnectionState conn( FizzClientQuicHandshakeContext::Builder().build()); diff --git a/quic/state/TransportSettings.h b/quic/state/TransportSettings.h index 927da1d10..227c5513b 100644 --- a/quic/state/TransportSettings.h +++ b/quic/state/TransportSettings.h @@ -53,6 +53,9 @@ struct D6DConfig { * whether it will send the base PMTU transport parameter during handshake. * As a result, d6d is activated for a connection only when *both* client and * server enables d6d. + * + * TODO: Please make sure QuicConnectionStateBase::D6DState::outstandingProbes + * are mutated correctly throughout Mvfst. */ bool enabled{false};