diff --git a/quic/api/QuicPacketScheduler.cpp b/quic/api/QuicPacketScheduler.cpp index ac3306791..697b231c3 100644 --- a/quic/api/QuicPacketScheduler.cpp +++ b/quic/api/QuicPacketScheduler.cpp @@ -918,15 +918,25 @@ SchedulingResult CloningScheduler::scheduleFramesForPacket( header, getAckState(conn_, builderPnSpace).largestAckedByPeer.value_or(0)); } - // If the packet is already a clone that has been processed, we don't clone - // it again. - if (outstandingPacket.maybeClonedPacketIdentifier && - conn_.outstandings.clonedPacketIdentifiers.count( - *outstandingPacket.maybeClonedPacketIdentifier) == 0) { - continue; + // The packet is already a clone + if (outstandingPacket.maybeClonedPacketIdentifier) { + // If packet has CRYPTO frame, don't clone it again even if not processed + // yet, move on to give next packet a chance to be cloned + const auto& frames = outstandingPacket.packet.frames; + if (conn_.transportSettings.cloneAllPacketsWithCryptoFrame && + std::find_if(frames.begin(), frames.end(), [](const auto& frame) { + return frame.type() == QuicWriteFrame::Type::WriteCryptoFrame; + }) != frames.end()) { + continue; + } + // Otherwise, clone until it is processed + if (conn_.outstandings.clonedPacketIdentifiers.count( + *outstandingPacket.maybeClonedPacketIdentifier) == 0) { + continue; + } } - // I think this only fail if udpSendPacketLen somehow shrinks in the middle - // of a connection. + // I think this only fail if udpSendPacketLen somehow shrinks in the + // middle of a connection. if (outstandingPacket.metadata.encodedSize > writableBytes + cipherOverhead_) { continue; @@ -936,13 +946,14 @@ SchedulingResult CloningScheduler::scheduleFramesForPacket( internalBuilder->encodePacketHeader(); PacketRebuilder rebuilder(*internalBuilder, conn_); - // TODO: It's possible we write out a packet that's larger than the packet - // size limit. For example, when the packet sequence number has advanced to - // a point where we need more bytes to encoded it than that of the original - // packet. In that case, if the original packet is already at the packet - // size limit, we will generate a packet larger than the limit. We can - // either ignore the problem, hoping the packet will be able to travel the - // network just fine; Or we can throw away the built packet and send a ping. + // TODO: It's possible we write out a packet that's larger than the + // packet size limit. For example, when the packet sequence number + // has advanced to a point where we need more bytes to encoded it + // than that of the original packet. In that case, if the original + // packet is already at the packet size limit, we will generate a + // packet larger than the limit. We can either ignore the problem, + // hoping the packet will be able to travel the network just fine; + // Or we can throw away the built packet and send a ping. // Rebuilder will write the rest of frames auto rebuildResult = rebuilder.rebuildFromPacket(outstandingPacket); @@ -954,14 +965,15 @@ SchedulingResult CloningScheduler::scheduleFramesForPacket( } else if ( conn_.transportSettings.dataPathType == DataPathType::ContinuousMemory) { - // When we use Inplace packet building and reuse the write buffer, even if - // the packet rebuild has failed, there might be some bytes already - // written into the buffer and the buffer tail pointer has already moved. - // We need to roll back the tail pointer to the position before the packet - // building to exclude those bytes. Otherwise these bytes will be sitting - // in between legit packets inside the buffer and will either cause errors - // further down the write path, or be sent out and then dropped at peer - // when peer fail to parse them. + // When we use Inplace packet building and reuse the write buffer, + // even if the packet rebuild has failed, there might be some + // bytes already written into the buffer and the buffer tail + // pointer has already moved. We need to roll back the tail + // pointer to the position before the packet building to exclude + // those bytes. Otherwise these bytes will be sitting in between + // legit packets inside the buffer and will either cause errors + // further down the write path, or be sent out and then dropped at + // peer when peer fail to parse them. internalBuilder.reset(); CHECK(conn_.bufAccessor && conn_.bufAccessor->ownsBuffer()); conn_.bufAccessor->trimEnd(conn_.bufAccessor->length() - prevSize); diff --git a/quic/api/test/QuicPacketSchedulerTest.cpp b/quic/api/test/QuicPacketSchedulerTest.cpp index 97b24fe64..b7934b1b0 100644 --- a/quic/api/test/QuicPacketSchedulerTest.cpp +++ b/quic/api/test/QuicPacketSchedulerTest.cpp @@ -108,7 +108,7 @@ PacketNum addOutstandingPacket(QuicConnectionStateBase& conn) { namespace quic { namespace test { -class QuicPacketSchedulerTest : public TestWithParam { +class QuicPacketSchedulerTest : public testing::Test { public: QuicVersion version{QuicVersion::MVFST}; }; @@ -602,17 +602,17 @@ TEST_F(QuicPacketSchedulerTest, DoNotCloneProcessedClonedPacket) { FizzClientQuicHandshakeContext::Builder().build()); FrameScheduler noopScheduler("frame", conn); CloningScheduler cloningScheduler(noopScheduler, conn, "CopyCat", 0); - // Add two outstanding packets, but then mark the second one processed by + // Add two outstanding packets, but then mark the first one processed by // adding a ClonedPacketIdentifier that's missing from the // outstandings.clonedPacketIdentifiers set - PacketNum expected = addOutstandingPacket(conn); - // There needs to have retransmittable frame for the rebuilder to work - conn.outstandings.packets.back().packet.frames.push_back( - MaxDataFrame(conn.flowControlState.advertisedMaxOffset)); addOutstandingPacket(conn); conn.outstandings.packets.back().maybeClonedPacketIdentifier = ClonedPacketIdentifier(PacketNumberSpace::AppData, 1); // There needs to have retransmittable frame for the rebuilder to work + conn.outstandings.packets.back().packet.frames.push_back( + MaxDataFrame(conn.flowControlState.advertisedMaxOffset)); + PacketNum expected = addOutstandingPacket(conn); + // There needs to have retransmittable frame for the rebuilder to work conn.outstandings.packets.back().packet.frames.push_back( MaxDataFrame(conn.flowControlState.advertisedMaxOffset)); @@ -631,6 +631,94 @@ TEST_F(QuicPacketSchedulerTest, DoNotCloneProcessedClonedPacket) { EXPECT_EQ(expected, result.clonedPacketIdentifier->packetNumber); } +class CloneAllPacketsWithCryptoFrameTest : public QuicPacketSchedulerTest, + public WithParamInterface {}; +TEST_P( + CloneAllPacketsWithCryptoFrameTest, + TestCloneAllPacketsWithCryptoFrameTrueFalse) { + QuicClientConnectionState conn( + FizzClientQuicHandshakeContext::Builder().build()); + conn.transportSettings.cloneAllPacketsWithCryptoFrame = GetParam(); + FrameScheduler noopScheduler("frame", conn); + CloningScheduler cloningScheduler(noopScheduler, conn, "cryptoClone", 0); + + // First packet has a crypto frame + PacketNum firstPacketNum = addOutstandingPacket(conn); + conn.outstandings.packets.back().packet.frames.push_back( + WriteCryptoFrame(0, 1)); + ClonedPacketIdentifier clonedPacketIdentifier( + PacketNumberSpace::AppData, firstPacketNum); + conn.outstandings.packets.back().maybeClonedPacketIdentifier = + clonedPacketIdentifier; + // It is not processed yet + conn.outstandings.clonedPacketIdentifiers.insert(clonedPacketIdentifier); + // There needs to have retransmittable frame for the rebuilder to work + conn.outstandings.packets.back().packet.frames.push_back( + MaxDataFrame(conn.flowControlState.advertisedMaxOffset)); + + PacketNum secondPacketNum = addOutstandingPacket(conn); + // There needs to have retransmittable frame for the rebuilder to work + conn.outstandings.packets.back().packet.frames.push_back( + MaxDataFrame(conn.flowControlState.advertisedMaxOffset)); + + ShortHeader header( + ProtectionType::KeyPhaseOne, + conn.clientConnectionId.value_or(getTestConnectionId()), + getNextPacketNum(conn, PacketNumberSpace::AppData)); + RegularQuicPacketBuilder builder( + conn.udpSendPacketLen, + std::move(header), + conn.ackStates.initialAckState->largestAckedByPeer.value_or(0)); + auto result = cloningScheduler.scheduleFramesForPacket( + std::move(builder), kDefaultUDPSendPacketLen); + EXPECT_TRUE( + result.clonedPacketIdentifier.has_value() && result.packet.has_value()); + EXPECT_EQ( + conn.transportSettings.cloneAllPacketsWithCryptoFrame ? secondPacketNum + : firstPacketNum, + result.clonedPacketIdentifier->packetNumber); +} + +INSTANTIATE_TEST_SUITE_P( + CloneAllPacketsWithCryptoFrameTest, + CloneAllPacketsWithCryptoFrameTest, + Bool()); + +TEST_F(QuicPacketSchedulerTest, DoNotSkipUnclonedCryptoPacket) { + QuicClientConnectionState conn( + FizzClientQuicHandshakeContext::Builder().build()); + conn.transportSettings.cloneAllPacketsWithCryptoFrame = true; + FrameScheduler noopScheduler("frame", conn); + CloningScheduler cloningScheduler(noopScheduler, conn, "cryptoClone", 0); + + // First packet has a crypto frame + PacketNum firstPacketNum = addOutstandingPacket(conn); + conn.outstandings.packets.back().packet.frames.push_back( + WriteCryptoFrame(0, 1)); + // There needs to have retransmittable frame for the rebuilder to work + conn.outstandings.packets.back().packet.frames.push_back( + MaxDataFrame(conn.flowControlState.advertisedMaxOffset)); + + addOutstandingPacket(conn); + // There needs to have retransmittable frame for the rebuilder to work + conn.outstandings.packets.back().packet.frames.push_back( + MaxDataFrame(conn.flowControlState.advertisedMaxOffset)); + + ShortHeader header( + ProtectionType::KeyPhaseOne, + conn.clientConnectionId.value_or(getTestConnectionId()), + getNextPacketNum(conn, PacketNumberSpace::AppData)); + RegularQuicPacketBuilder builder( + conn.udpSendPacketLen, + std::move(header), + conn.ackStates.initialAckState->largestAckedByPeer.value_or(0)); + auto result = cloningScheduler.scheduleFramesForPacket( + std::move(builder), kDefaultUDPSendPacketLen); + EXPECT_TRUE( + result.clonedPacketIdentifier.has_value() && result.packet.has_value()); + EXPECT_EQ(firstPacketNum, result.clonedPacketIdentifier->packetNumber); +} + TEST_F(QuicPacketSchedulerTest, CloneSchedulerHasHandshakeData) { QuicClientConnectionState conn( FizzClientQuicHandshakeContext::Builder().build()); diff --git a/quic/state/TransportSettings.h b/quic/state/TransportSettings.h index 8a44af2ba..6140feec4 100644 --- a/quic/state/TransportSettings.h +++ b/quic/state/TransportSettings.h @@ -409,6 +409,7 @@ struct TransportSettings { // Whether to trigger packet processing per socket read rather than batch // receiving and then processing. bool networkDataPerSocketRead{false}; + bool cloneAllPacketsWithCryptoFrame{false}; }; } // namespace quic