diff --git a/quic/QuicConstants.h b/quic/QuicConstants.h index 0cdf63a13..168642086 100644 --- a/quic/QuicConstants.h +++ b/quic/QuicConstants.h @@ -285,6 +285,7 @@ enum class QuicVersion : uint32_t { MVFST_EXPERIMENTAL = 0xfaceb00e, // Experimental alias for MVFST MVFST_ALIAS = 0xfaceb010, MVFST_INVALID = 0xfaceb00f, + MVFST_EXPERIMENTAL2 = 0xfaceb011, // Experimental alias for MVFST }; using QuicVersionType = std::underlying_type::type; diff --git a/quic/api/QuicPacketScheduler.cpp b/quic/api/QuicPacketScheduler.cpp index 5ee7ce2b8..176ec7bb6 100644 --- a/quic/api/QuicPacketScheduler.cpp +++ b/quic/api/QuicPacketScheduler.cpp @@ -312,11 +312,19 @@ SchedulingResult FrameScheduler::scheduleFramesForPacket( return SchedulingResult(folly::none, std::move(builder).buildPacket()); } +void FrameScheduler::writeNextAcks(PacketBuilderInterface& builder) { + ackScheduler_->writeNextAcks(builder); +} + bool FrameScheduler::hasData() const { return (ackScheduler_ && ackScheduler_->hasPendingAcks()) || hasImmediateData(); } +bool FrameScheduler::hasPendingAcks() const { + return ackScheduler_ && ackScheduler_->hasPendingAcks(); +} + bool FrameScheduler::hasImmediateData() const { return (cryptoStreamScheduler_ && cryptoStreamScheduler_->hasData()) || (streamFrameScheduler_ && streamFrameScheduler_->hasPendingData()) || @@ -794,7 +802,12 @@ SchedulingResult CloningScheduler::scheduleFramesForPacket( // The writableBytes in this function shouldn't be limited by cwnd, since // we only use CloningScheduler for the cases that we want to bypass cwnd for // now. - if (frameScheduler_.hasData()) { + bool hasData = frameScheduler_.hasData(); + if (conn_.version.has_value() && + conn_.version.value() == QuicVersion::MVFST_EXPERIMENTAL2) { + hasData = frameScheduler_.hasImmediateData(); + } + if (hasData) { // Note that there is a possibility that we end up writing nothing here. But // if frameScheduler_ hasData() to write, we shouldn't invoke the cloning // path if the write fails. @@ -874,6 +887,14 @@ SchedulingResult CloningScheduler::scheduleFramesForPacket( // Rebuilder will write the rest of frames auto rebuildResult = rebuilder.rebuildFromPacket(outstandingPacket); if (rebuildResult) { + if (conn_.version.has_value() && + conn_.version.value() == QuicVersion::MVFST_EXPERIMENTAL2) { + // Check if we have any acks pending and write those into the cloned + // packet as well. + if (frameScheduler_.hasPendingAcks()) { + frameScheduler_.writeNextAcks(*internalBuilder); + } + } return SchedulingResult( std::move(rebuildResult), std::move(*internalBuilder).buildPacket()); } else if ( diff --git a/quic/api/QuicPacketScheduler.h b/quic/api/QuicPacketScheduler.h index 5828b8e9d..795896633 100644 --- a/quic/api/QuicPacketScheduler.h +++ b/quic/api/QuicPacketScheduler.h @@ -292,11 +292,17 @@ class FrameScheduler : public QuicPacketScheduler { // If any scheduler, including AckScheduler, has pending data to send FOLLY_NODISCARD bool hasData() const override; + // If AckScheduler has any pending acks to write. + FOLLY_NODISCARD bool hasPendingAcks() const; + // If any of the non-Ack scheduler has pending data to send FOLLY_NODISCARD virtual bool hasImmediateData() const; FOLLY_NODISCARD folly::StringPiece name() const override; + // Writes outstanding acks. + void writeNextAcks(PacketBuilderInterface& builder); + private: folly::Optional streamFrameScheduler_; folly::Optional ackScheduler_; @@ -313,11 +319,11 @@ class FrameScheduler : public QuicPacketScheduler { /** * A packet scheduler wrapping a normal FrameScheduler with the ability to clone - * exiting packets that are still outstanding. A CloningScheduler first trie to - * write new farmes with new data into a packet. If that fails due to the lack + * exiting packets that are still outstanding. A CloningScheduler first tries to + * write new frames with new data into a packet. If that fails due to the lack * of new data, it falls back to cloning one inflight packet from a connection's * oustanding packets if there is at least one outstanding packet that's smaller - * than the writableBytes limit, and isn't a Handshake packet. + * than the writableBytes limit. */ class CloningScheduler : public QuicPacketScheduler { public: diff --git a/quic/api/test/Mocks.h b/quic/api/test/Mocks.h index a14b0493f..beedacbb1 100644 --- a/quic/api/test/Mocks.h +++ b/quic/api/test/Mocks.h @@ -35,6 +35,7 @@ class MockFrameScheduler : public FrameScheduler { } MOCK_METHOD((bool), hasData, (), (const)); + MOCK_METHOD((bool), hasImmediateData, (), (const)); MOCK_METHOD( SchedulingResult, _scheduleFramesForPacket, diff --git a/quic/api/test/QuicPacketSchedulerTest.cpp b/quic/api/test/QuicPacketSchedulerTest.cpp index fe7b97ea9..d77463fb1 100644 --- a/quic/api/test/QuicPacketSchedulerTest.cpp +++ b/quic/api/test/QuicPacketSchedulerTest.cpp @@ -726,6 +726,94 @@ TEST_F(QuicPacketSchedulerTest, CloneSchedulerHasHandshakeData) { EXPECT_TRUE(cloningScheduler.hasData()); } +/** + * This test case covers the following scenario: + 1) conn sent out a handshake packet that did not get acked yet + 2) conn received some handshake data that needs to be acked + 3) imitate that we're emitting a PTO packet (that is generated via cloning + scheduler) + 4) emitted cloned packet MUST have both cloned crypto data AND ack + frame(s) + + There was a bug that would result in mvfst emit a "empty" PTO packet with + acks; this is the test case to cover that scenario. + */ +TEST_F(QuicPacketSchedulerTest, CloneSchedulerHasHandshakeDataAndAcks) { + QuicServerConnectionState conn( + FizzServerQuicHandshakeContext::Builder().build()); + conn.version = QuicVersion::MVFST_EXPERIMENTAL2; + + FrameScheduler noopScheduler = std::move(FrameScheduler::Builder( + conn, + EncryptionLevel::Handshake, + PacketNumberSpace::Handshake, + "testScheduler") + .ackFrames()) + .build(); + addHandshakeOutstandingPacket(conn); + + // Add some crypto data for the outstanding packet to make it look legit. + // This is so cloning scheduler can actually copy something. + getCryptoStream(*conn.cryptoState, EncryptionLevel::Handshake) + ->retransmissionBuffer.emplace( + 0, + std::make_unique( + folly::IOBuf::copyBuffer("test"), 0, false)); + conn.outstandings.packets.back().packet.frames.push_back( + WriteCryptoFrame(0, 4)); + + // Make it look like we received some acks from the peer. + conn.ackStates.handshakeAckState.acks.insert(10); + conn.ackStates.handshakeAckState.largestRecvdPacketTime = Clock::now(); + + // Create cloning scheduler. + CloningScheduler cloningScheduler(noopScheduler, conn, "CopyCat", 0); + EXPECT_TRUE(cloningScheduler.hasData()); + + // Get the packet builder going for the clone packet. + PacketNum nextPacketNum = + getNextPacketNum(conn, PacketNumberSpace::Handshake); + std::vector zeroConnIdData(quic::kDefaultConnectionIdSize, 0); + ConnectionId srcConnId(zeroConnIdData); + LongHeader header( + LongHeader::Types::Handshake, + srcConnId, + conn.clientConnectionId.value_or(quic::test::getTestConnectionId()), + nextPacketNum, + QuicVersion::QUIC_DRAFT); + RegularQuicPacketBuilder builder( + conn.udpSendPacketLen, + std::move(header), + conn.ackStates.appDataAckState.largestAckedByPeer.value_or(0)); + + // Clone the packet. + auto result = cloningScheduler.scheduleFramesForPacket( + std::move(builder), kDefaultUDPSendPacketLen); + EXPECT_TRUE(result.packetEvent.has_value()); + EXPECT_TRUE(result.packet.has_value()); + + // Cloned packet has to have at least one ack frame AND the crypto data. + bool hasAckFrame = false; + bool hasCryptoFrame = false; + for (auto iter = result.packet->packet.frames.cbegin(); + iter != result.packet->packet.frames.cend(); + iter++) { + const QuicWriteFrame& frame = *iter; + switch (frame.type()) { + case QuicWriteFrame::Type::WriteAckFrame: + hasAckFrame = true; + break; + case QuicWriteFrame::Type::WriteCryptoFrame: + hasCryptoFrame = true; + break; + default: + break; + } + } + EXPECT_TRUE(hasAckFrame); + EXPECT_TRUE(hasCryptoFrame); +} + TEST_F(QuicPacketSchedulerTest, CloneSchedulerHasInitialData) { QuicClientConnectionState conn( FizzClientQuicHandshakeContext::Builder().build()); @@ -779,13 +867,16 @@ TEST_F(QuicPacketSchedulerTest, DoNotCloneHandshake) { TEST_F(QuicPacketSchedulerTest, CloneSchedulerUseNormalSchedulerFirst) { QuicClientConnectionState conn( FizzClientQuicHandshakeContext::Builder().build()); + conn.version = QuicVersion::MVFST_EXPERIMENTAL2; NiceMock mockScheduler(&conn); CloningScheduler cloningScheduler(mockScheduler, conn, "Mocker", 0); ShortHeader header( ProtectionType::KeyPhaseOne, conn.clientConnectionId.value_or(getTestConnectionId()), getNextPacketNum(conn, PacketNumberSpace::AppData)); - EXPECT_CALL(mockScheduler, hasData()).Times(1).WillOnce(Return(true)); + EXPECT_CALL(mockScheduler, hasImmediateData()) + .Times(1) + .WillOnce(Return(true)); EXPECT_CALL(mockScheduler, _scheduleFramesForPacket(_, _)) .Times(1) diff --git a/quic/codec/Types.cpp b/quic/codec/Types.cpp index afbbe5ded..3560bc5ef 100644 --- a/quic/codec/Types.cpp +++ b/quic/codec/Types.cpp @@ -451,6 +451,8 @@ std::string toString(QuicVersion version) { return "MVFST_ALIAS"; case QuicVersion::MVFST_INVALID: return "MVFST_INVALID"; + case QuicVersion::MVFST_EXPERIMENTAL2: + return "MVFST_EXPERIMENTAL2"; } LOG(WARNING) << "toString has unhandled version type"; return "UNKNOWN"; diff --git a/quic/server/QuicServer.h b/quic/server/QuicServer.h index a53561e24..74875067b 100644 --- a/quic/server/QuicServer.h +++ b/quic/server/QuicServer.h @@ -398,6 +398,7 @@ class QuicServer : public QuicServerWorker::WorkerCallback, std::vector supportedVersions_{ {QuicVersion::MVFST, QuicVersion::MVFST_EXPERIMENTAL, + QuicVersion::MVFST_EXPERIMENTAL2, QuicVersion::MVFST_ALIAS, QuicVersion::QUIC_V1, QuicVersion::QUIC_DRAFT}}; diff --git a/quic/server/state/ServerStateMachine.h b/quic/server/state/ServerStateMachine.h index 954dec9c5..1a1d82ef5 100644 --- a/quic/server/state/ServerStateMachine.h +++ b/quic/server/state/ServerStateMachine.h @@ -157,6 +157,7 @@ struct QuicServerConnectionState : public QuicConnectionStateBase { supportedVersions = std::vector{ {QuicVersion::MVFST, QuicVersion::MVFST_EXPERIMENTAL, + QuicVersion::MVFST_EXPERIMENTAL2, QuicVersion::MVFST_ALIAS, QuicVersion::QUIC_V1, QuicVersion::QUIC_DRAFT}};