diff --git a/quic/QuicConstants.cpp b/quic/QuicConstants.cpp index da36f631d..f44706250 100644 --- a/quic/QuicConstants.cpp +++ b/quic/QuicConstants.cpp @@ -98,6 +98,8 @@ folly::StringPiece writeDataReasonString(WriteDataReason reason) { return "Reset"; case WriteDataReason::PATHCHALLENGE: return "PathChallenge"; + case WriteDataReason::PING: + return "Ping"; case WriteDataReason::NO_WRITE: return "NoWrite"; } diff --git a/quic/QuicConstants.h b/quic/QuicConstants.h index 8b890bcc8..9031ffc8a 100644 --- a/quic/QuicConstants.h +++ b/quic/QuicConstants.h @@ -462,6 +462,7 @@ enum class WriteDataReason { SIMPLE, RESET, PATHCHALLENGE, + PING, }; enum class NoWriteReason { diff --git a/quic/api/QuicPacketScheduler.cpp b/quic/api/QuicPacketScheduler.cpp index 87101639d..7d913d50a 100644 --- a/quic/api/QuicPacketScheduler.cpp +++ b/quic/api/QuicPacketScheduler.cpp @@ -81,6 +81,11 @@ FrameScheduler::Builder& FrameScheduler::Builder::simpleFrames() { return *this; } +FrameScheduler::Builder& FrameScheduler::Builder::pingFrames() { + pingFrameScheduler_ = true; + return *this; +} + FrameScheduler FrameScheduler::Builder::build() && { FrameScheduler scheduler(std::move(name_)); if (retransmissionScheduler_) { @@ -109,6 +114,9 @@ FrameScheduler FrameScheduler::Builder::build() && { if (simpleFrameScheduler_) { scheduler.simpleFrameScheduler_.emplace(SimpleFrameScheduler(conn_)); } + if (pingFrameScheduler_) { + scheduler.pingFrameScheduler_.emplace(PingFrameScheduler(conn_)); + } return scheduler; } @@ -164,6 +172,9 @@ SchedulingResult FrameScheduler::scheduleFramesForPacket( simpleFrameScheduler_->hasPendingSimpleFrames()) { simpleFrameScheduler_->writeSimpleFrames(wrapper); } + if (pingFrameScheduler_ && pingFrameScheduler_->hasPingFrame()) { + pingFrameScheduler_->writePing(wrapper); + } if (retransmissionScheduler_ && retransmissionScheduler_->hasPendingData()) { retransmissionScheduler_->writeRetransmissionStreams(wrapper); } @@ -201,7 +212,8 @@ bool FrameScheduler::hasImmediateData() const { windowUpdateScheduler_->hasPendingWindowUpdates()) || (blockedScheduler_ && blockedScheduler_->hasPendingBlockedFrames()) || (simpleFrameScheduler_ && - simpleFrameScheduler_->hasPendingSimpleFrames()); + simpleFrameScheduler_->hasPendingSimpleFrames()) || + (pingFrameScheduler_ && pingFrameScheduler_->hasPingFrame()); } std::string FrameScheduler::name() const { @@ -406,6 +418,17 @@ bool SimpleFrameScheduler::writeSimpleFrames(PacketBuilderInterface& builder) { return framesWritten; } +PingFrameScheduler::PingFrameScheduler(const QuicConnectionStateBase& conn) + : conn_(conn) {} + +bool PingFrameScheduler::hasPingFrame() const { + return conn_.pendingEvents.sendPing; +} + +bool PingFrameScheduler::writePing(PacketBuilderInterface& builder) { + return 0 != writeFrame(PingFrame(), builder); +} + WindowUpdateScheduler::WindowUpdateScheduler( const QuicConnectionStateBase& conn) : conn_(conn) {} diff --git a/quic/api/QuicPacketScheduler.h b/quic/api/QuicPacketScheduler.h index 37f883dd5..7a79b6039 100644 --- a/quic/api/QuicPacketScheduler.h +++ b/quic/api/QuicPacketScheduler.h @@ -194,8 +194,7 @@ class AckScheduler { AckScheduler(const QuicConnectionStateBase& conn, const AckState& ackState); template - folly::Optional writeNextAcks( - PacketBuilderInterface& builder); + folly::Optional writeNextAcks(PacketBuilderInterface& builder); bool hasPendingAcks() const; @@ -243,6 +242,18 @@ class SimpleFrameScheduler { const QuicConnectionStateBase& conn_; }; +class PingFrameScheduler { + public: + explicit PingFrameScheduler(const QuicConnectionStateBase& conn); + + bool hasPingFrame() const; + + bool writePing(PacketBuilderInterface& builder); + + private: + const QuicConnectionStateBase& conn_; +}; + class WindowUpdateScheduler { public: explicit WindowUpdateScheduler(const QuicConnectionStateBase& conn); @@ -309,6 +320,7 @@ class FrameScheduler : public QuicPacketScheduler { Builder& blockedFrames(); Builder& cryptoFrames(); Builder& simpleFrames(); + Builder& pingFrames(); FrameScheduler build() &&; @@ -327,6 +339,7 @@ class FrameScheduler : public QuicPacketScheduler { bool blockedScheduler_{false}; bool cryptoStreamScheduler_{false}; bool simpleFrameScheduler_{false}; + bool pingFrameScheduler_{false}; }; explicit FrameScheduler(std::string name); @@ -352,6 +365,7 @@ class FrameScheduler : public QuicPacketScheduler { folly::Optional blockedScheduler_; folly::Optional cryptoStreamScheduler_; folly::Optional simpleFrameScheduler_; + folly::Optional pingFrameScheduler_; std::string name_; }; diff --git a/quic/api/QuicTransportBase.cpp b/quic/api/QuicTransportBase.cpp index 6de3e2f3c..017bcb307 100644 --- a/quic/api/QuicTransportBase.cpp +++ b/quic/api/QuicTransportBase.cpp @@ -2097,7 +2097,7 @@ void QuicTransportBase::sendPing( } // Step 1: Send a simple ping frame - quic::sendSimpleFrame(*conn_, PingFrame()); + conn_->pendingEvents.sendPing = true; updateWriteLooper(true); // Step 2: Schedule the timeout on event base diff --git a/quic/api/QuicTransportFunctions.cpp b/quic/api/QuicTransportFunctions.cpp index c181bc2a7..40e59bfea 100644 --- a/quic/api/QuicTransportFunctions.cpp +++ b/quic/api/QuicTransportFunctions.cpp @@ -122,7 +122,8 @@ uint64_t writeQuicDataToSocketImpl( .simpleFrames() .resetFrames() .streamFrames() - .streamRetransmissions(); + .streamRetransmissions() + .pingFrames(); if (!exceptCryptoStream) { probeSchedulerBuilder.cryptoFrames(); } @@ -133,6 +134,7 @@ uint64_t writeQuicDataToSocketImpl( srcConnId, dstConnId, builder, + EncryptionLevel::AppData, PacketNumberSpace::AppData, probeScheduler, std::min( @@ -155,7 +157,8 @@ uint64_t writeQuicDataToSocketImpl( .resetFrames() .windowUpdateFrames() .blockedFrames() - .simpleFrames(); + .simpleFrames() + .pingFrames(); if (!exceptCryptoStream) { schedulerBuilder.cryptoFrames(); } @@ -458,6 +461,7 @@ void updateConnection( auto packetNum = packet.header.getPacketSequenceNum(); bool retransmittable = false; // AckFrame and PaddingFrame are not retx-able. bool isHandshake = false; + bool isPing = false; uint32_t connWindowUpdateSent = 0; uint32_t ackFrameCounter = 0; auto packetNumberSpace = packet.header.getPacketNumberSpace(); @@ -581,6 +585,10 @@ void updateConnection( conn.streamManager->removeBlocked(streamBlockedFrame.streamId); break; } + case QuicWriteFrame::Type::PingFrame_E: + conn.pendingEvents.sendPing = false; + isPing = true; + break; case QuicWriteFrame::Type::QuicSimpleFrame_E: { const QuicSimpleFrame& simpleFrame = *frame.asQuicSimpleFrame(); retransmittable = true; @@ -616,7 +624,7 @@ void updateConnection( } conn.lossState.totalBytesSent += encodedSize; - if (!retransmittable) { + if (!retransmittable && !isPing) { DCHECK(!packetEvent); return; } @@ -794,6 +802,7 @@ uint64_t writeCryptoAndAckDataToSocket( srcConnId, dstConnId, builder, + encryptionLevel, LongHeader::typeToPacketNumberSpace(packetType), scheduler, std::min( @@ -1230,6 +1239,7 @@ uint64_t writeProbingDataToSocket( const ConnectionId& srcConnId, const ConnectionId& dstConnId, const HeaderBuilder& builder, + EncryptionLevel encryptionLevel, PacketNumberSpace pnSpace, FrameScheduler scheduler, uint8_t probesToSend, @@ -1257,16 +1267,12 @@ uint64_t writeProbingDataToSocket( token); if (probesToSend && !written) { // Fall back to send a ping: - // TODO: Now that Probes can be used for handshake packets. We need to make - // sure we only send Ping here, no other Simple frames. - sendSimpleFrame(connection, PingFrame()); - auto pingScheduler = std::move(FrameScheduler::Builder( - connection, - EncryptionLevel::AppData, - PacketNumberSpace::AppData, - "PingScheduler") - .simpleFrames()) - .build(); + connection.pendingEvents.sendPing = true; + auto pingScheduler = + std::move(FrameScheduler::Builder( + connection, encryptionLevel, pnSpace, "PingScheduler") + .pingFrames()) + .build(); written += writeConnectionDataToSocket( sock, connection, @@ -1362,6 +1368,9 @@ WriteDataReason hasNonAckDataToWrite(const QuicConnectionStateBase& conn) { if ((conn.pendingEvents.pathChallenge != folly::none)) { return WriteDataReason::PATHCHALLENGE; } + if (conn.pendingEvents.sendPing) { + return WriteDataReason::PING; + } return WriteDataReason::NO_WRITE; } diff --git a/quic/api/QuicTransportFunctions.h b/quic/api/QuicTransportFunctions.h index 27db8d307..c2ac1529d 100644 --- a/quic/api/QuicTransportFunctions.h +++ b/quic/api/QuicTransportFunctions.h @@ -273,6 +273,7 @@ uint64_t writeProbingDataToSocket( const ConnectionId& srcConnId, const ConnectionId& dstConnId, const HeaderBuilder& builder, + EncryptionLevel encryptionLevel, PacketNumberSpace pnSpace, FrameScheduler scheduler, uint8_t probesToSend, diff --git a/quic/api/test/QuicPacketSchedulerTest.cpp b/quic/api/test/QuicPacketSchedulerTest.cpp index e23c6abd3..81020e6b1 100644 --- a/quic/api/test/QuicPacketSchedulerTest.cpp +++ b/quic/api/test/QuicPacketSchedulerTest.cpp @@ -492,7 +492,7 @@ TEST_F(QuicPacketSchedulerTest, WriteOnlyOutstandingPacketsTest) { // Write those framses with a regular builder writeFrame(connCloseFrame, regularBuilder); writeFrame(QuicSimpleFrame(maxStreamFrame), regularBuilder); - writeFrame(QuicSimpleFrame(pingFrame), regularBuilder); + writeFrame(pingFrame, regularBuilder); writeAckFrame(ackMeta, regularBuilder); auto result = cloningScheduler.scheduleFramesForPacket( diff --git a/quic/api/test/QuicTransportFunctionsTest.cpp b/quic/api/test/QuicTransportFunctionsTest.cpp index 8abf150a7..df8c5acec 100644 --- a/quic/api/test/QuicTransportFunctionsTest.cpp +++ b/quic/api/test/QuicTransportFunctionsTest.cpp @@ -46,6 +46,7 @@ uint64_t writeProbingDataToSocketForTest( *conn.clientConnectionId, *conn.serverConnectionId, ShortHeaderBuilder(), + EncryptionLevel::AppData, PacketNumberSpace::AppData, scheduler, probesToSend, @@ -75,6 +76,7 @@ void writeCryptoDataProbesToSocketForTest( *conn.clientConnectionId, *conn.serverConnectionId, LongHeaderBuilder(type), + protectionTypeToEncryptionLevel(longHeaderTypeToProtectionType(type)), LongHeader::typeToPacketNumberSpace(type), scheduler, probesToSend, @@ -178,6 +180,17 @@ class QuicTransportFunctionsTest : public Test { std::unique_ptr transportInfoCb_; }; +TEST_F(QuicTransportFunctionsTest, PingPacketGoesToOPList) { + auto conn = createConn(); + auto packet = buildEmptyPacket(*conn, PacketNumberSpace::AppData); + packet.packet.frames.push_back(PingFrame()); + EXPECT_EQ(0, conn->outstandings.packets.size()); + updateConnection(*conn, folly::none, packet.packet, Clock::now(), 50); + EXPECT_EQ(1, conn->outstandings.packets.size()); + // But it won't set loss detection alarm + EXPECT_FALSE(conn->pendingEvents.setLossDetectionAlarm); +} + TEST_F(QuicTransportFunctionsTest, TestUpdateConnection) { auto conn = createConn(); auto mockCongestionController = @@ -1604,15 +1617,10 @@ TEST_F(QuicTransportFunctionsTest, ProbingNotFallbackToPingWhenNoQuota) { TEST_F(QuicTransportFunctionsTest, ProbingFallbackToPing) { auto conn = createConn(); - auto mockCongestionController = - std::make_unique>(); - auto rawCongestionController = mockCongestionController.get(); - conn->congestionController = std::move(mockCongestionController); EventBase evb; auto socket = std::make_unique>(&evb); auto rawSocket = socket.get(); - EXPECT_CALL(*rawCongestionController, onPacketSent(_)).Times(1); EXPECT_CALL(*rawSocket, write(_, _)) .Times(1) .WillOnce(Invoke([&](const SocketAddress&, @@ -1629,17 +1637,8 @@ TEST_F(QuicTransportFunctionsTest, ProbingFallbackToPing) { *aead, *headerCipher, getVersion(*conn))); + // Ping is the only non-retransmittable packet that will go into OP list EXPECT_EQ(1, conn->outstandings.packets.size()); - EXPECT_EQ(1, conn->outstandings.packets[0].packet.frames.size()); - EXPECT_EQ( - QuicWriteFrame::Type::QuicSimpleFrame_E, - conn->outstandings.packets[0].packet.frames[0].type()); - EXPECT_EQ( - QuicSimpleFrame::Type::PingFrame_E, - conn->outstandings.packets[0] - .packet.frames[0] - .asQuicSimpleFrame() - ->type()); } TEST_F(QuicTransportFunctionsTest, TestCryptoWritingIsHandshakeInOutstanding) { diff --git a/quic/client/QuicClientTransport.cpp b/quic/client/QuicClientTransport.cpp index 523c79458..7459b3b2b 100644 --- a/quic/client/QuicClientTransport.cpp +++ b/quic/client/QuicClientTransport.cpp @@ -220,8 +220,7 @@ void QuicClientTransport::processPacketData( auto isAck = quicFrame.asReadAckFrame(); auto isClose = quicFrame.asConnectionCloseFrame(); auto isCrypto = quicFrame.asReadCryptoFrame(); - auto isSimple = quicFrame.asQuicSimpleFrame(); - auto isPing = isSimple ? isSimple->asPingFrame() : nullptr; + auto isPing = quicFrame.asPingFrame(); // TODO: add path challenge and response if (!isPadding && !isAck && !isClose && !isCrypto && !isPing) { throw QuicTransportException( @@ -350,12 +349,10 @@ void QuicClientTransport::processPacketData( *cryptoStream, frame.offset, frame.len); break; } - case QuicWriteFrame::Type::QuicSimpleFrame_E: { - const quic::QuicSimpleFrame simpleFrame = - *packetFrame.asQuicSimpleFrame(); - updateSimpleFrameOnAck(*conn_, simpleFrame); + case QuicWriteFrame::Type::PingFrame_E: + conn_->pendingEvents.cancelPingTimeout = true; break; - } + case QuicWriteFrame::Type::QuicSimpleFrame_E: default: // ignore other frames. break; @@ -482,9 +479,12 @@ void QuicClientTransport::processPacketData( "Peer closed", TransportErrorCode::NO_ERROR); break; } - case QuicFrame::Type::PaddingFrame_E: { + case QuicFrame::Type::PingFrame_E: + // Ping isn't retransmittable. But we would like to ack them early. + pktHasRetransmittableData = true; + break; + case QuicFrame::Type::PaddingFrame_E: break; - } case QuicFrame::Type::QuicSimpleFrame_E: { QuicSimpleFrame& simpleFrame = *quicFrame.asQuicSimpleFrame(); pktHasRetransmittableData = true; diff --git a/quic/codec/QuicPacketRebuilder.cpp b/quic/codec/QuicPacketRebuilder.cpp index 43ef75d05..ca17c164e 100644 --- a/quic/codec/QuicPacketRebuilder.cpp +++ b/quic/codec/QuicPacketRebuilder.cpp @@ -159,6 +159,11 @@ folly::Optional PacketRebuilder::rebuildFromPacket( writeSuccess = writeFrame(paddingFrame, builder_) != 0; break; } + case QuicWriteFrame::Type::PingFrame_E: { + const PingFrame& pingFrame = *frame.asPingFrame(); + writeSuccess = writeFrame(pingFrame, builder_) != 0; + break; + } case QuicWriteFrame::Type::QuicSimpleFrame_E: { const QuicSimpleFrame& simpleFrame = *frame.asQuicSimpleFrame(); auto updatedSimpleFrame = @@ -185,7 +190,7 @@ folly::Optional PacketRebuilder::rebuildFromPacket( } } // We shouldn't clone if: - // (1) we only end up cloning acks and paddings. + // (1) we only end up cloning only acks, ping, or paddings. // (2) we should write window update, but didn't, and wrote nothing else. if (!notPureAck || (shouldWriteWindowUpdate && !windowUpdateWritten && !writeSuccess)) { diff --git a/quic/codec/QuicWriteCodec.cpp b/quic/codec/QuicWriteCodec.cpp index 458e1d5d1..c001c459d 100644 --- a/quic/codec/QuicWriteCodec.cpp +++ b/quic/codec/QuicWriteCodec.cpp @@ -328,17 +328,6 @@ size_t writeSimpleFrame( uint64_t spaceLeft = builder.remainingSpaceInPkt(); switch (frame.type()) { - case QuicSimpleFrame::Type::PingFrame_E: { - const PingFrame& pingFrame = *frame.asPingFrame(); - QuicInteger intFrameType(static_cast(FrameType::PING)); - if (packetSpaceCheck(spaceLeft, intFrameType.getSize())) { - builder.write(intFrameType); - builder.appendFrame(QuicSimpleFrame(pingFrame)); - return intFrameType.getSize(); - } - // no space left in packet - return size_t(0); - } case QuicSimpleFrame::Type::StopSendingFrame_E: { const StopSendingFrame& stopSendingFrame = *frame.asStopSendingFrame(); QuicInteger intFrameType(static_cast(FrameType::STOP_SENDING)); @@ -667,6 +656,17 @@ size_t writeFrame(QuicWriteFrame&& frame, PacketBuilderInterface& builder) { // no space left in packet return size_t(0); } + case QuicWriteFrame::Type::PingFrame_E: { + const PingFrame& pingFrame = *frame.asPingFrame(); + QuicInteger intFrameType(static_cast(FrameType::PING)); + if (packetSpaceCheck(spaceLeft, intFrameType.getSize())) { + builder.write(intFrameType); + builder.appendFrame(pingFrame); + return intFrameType.getSize(); + } + // no space left in packet + return size_t(0); + } case QuicWriteFrame::Type::QuicSimpleFrame_E: { return writeSimpleFrame(std::move(*frame.asQuicSimpleFrame()), builder); } diff --git a/quic/codec/Types.h b/quic/codec/Types.h index 6b4d1d5cc..ea7e8c32f 100644 --- a/quic/codec/Types.h +++ b/quic/codec/Types.h @@ -598,7 +598,6 @@ struct RetryToken { F(NewConnectionIdFrame, __VA_ARGS__) \ F(MaxStreamsFrame, __VA_ARGS__) \ F(RetireConnectionIdFrame, __VA_ARGS__) \ - F(PingFrame, __VA_ARGS__) \ F(HandshakeDoneFrame, __VA_ARGS__) DECLARE_VARIANT_TYPE(QuicSimpleFrame, QUIC_SIMPLE_FRAME) @@ -617,6 +616,7 @@ DECLARE_VARIANT_TYPE(QuicSimpleFrame, QUIC_SIMPLE_FRAME) F(ReadCryptoFrame, __VA_ARGS__) \ F(ReadNewTokenFrame, __VA_ARGS__) \ F(QuicSimpleFrame, __VA_ARGS__) \ + F(PingFrame, __VA_ARGS__) \ F(NoopFrame, __VA_ARGS__) DECLARE_VARIANT_TYPE(QuicFrame, QUIC_FRAME) @@ -634,6 +634,7 @@ DECLARE_VARIANT_TYPE(QuicFrame, QUIC_FRAME) F(WriteStreamFrame, __VA_ARGS__) \ F(WriteCryptoFrame, __VA_ARGS__) \ F(QuicSimpleFrame, __VA_ARGS__) \ + F(PingFrame, __VA_ARGS__) \ F(NoopFrame, __VA_ARGS__) // Types of frames which are written. diff --git a/quic/codec/test/QuicPacketBuilderTest.cpp b/quic/codec/test/QuicPacketBuilderTest.cpp index 288b745da..1e20c815e 100644 --- a/quic/codec/test/QuicPacketBuilderTest.cpp +++ b/quic/codec/test/QuicPacketBuilderTest.cpp @@ -540,7 +540,7 @@ TEST_P(QuicPacketBuilderTest, PadUpLongHeaderPacket) { largestAcked, kDefaultUDPSendPacketLen); builder->encodePacketHeader(); - writeFrame(QuicSimpleFrame(PingFrame()), *builder); + writeFrame(PingFrame(), *builder); EXPECT_TRUE(builder->canBuildPacket()); auto builtOut = std::move(*builder).buildPacket(); auto resultPacket = builtOut.packet; diff --git a/quic/codec/test/QuicPacketRebuilderTest.cpp b/quic/codec/test/QuicPacketRebuilderTest.cpp index 48f99dea0..f57644676 100644 --- a/quic/codec/test/QuicPacketRebuilderTest.cpp +++ b/quic/codec/test/QuicPacketRebuilderTest.cpp @@ -79,7 +79,7 @@ TEST_F(QuicPacketRebuilderTest, RebuildPacket) { // Write them with a regular builder writeFrame(connCloseFrame, regularBuilder1); writeFrame(QuicSimpleFrame(maxStreamsFrame), regularBuilder1); - writeFrame(QuicSimpleFrame(pingFrame), regularBuilder1); + writeFrame(pingFrame, regularBuilder1); writeAckFrame(ackMeta, regularBuilder1); writeStreamFrameHeader( regularBuilder1, @@ -138,6 +138,9 @@ TEST_F(QuicPacketRebuilderTest, RebuildPacket) { EXPECT_EQ(FrameType::ACK, closeFrame.closingFrameType); break; } + case QuicWriteFrame::Type::PingFrame_E: + EXPECT_NE(frame.asPingFrame(), nullptr); + break; case QuicWriteFrame::Type::QuicSimpleFrame_E: { const QuicSimpleFrame& simpleFrame = *frame.asQuicSimpleFrame(); switch (simpleFrame.type()) { @@ -148,11 +151,6 @@ TEST_F(QuicPacketRebuilderTest, RebuildPacket) { EXPECT_EQ(4321, maxStreamFrame->maxStreams); break; } - case QuicSimpleFrame::Type::PingFrame_E: { - const PingFrame* simplePingFrame = simpleFrame.asPingFrame(); - EXPECT_NE(simplePingFrame, nullptr); - break; - } default: EXPECT_TRUE(false); /* fail if this happens */ } @@ -394,7 +392,7 @@ TEST_F(QuicPacketRebuilderTest, CannotRebuild) { // Write them with a regular builder writeFrame(connCloseFrame, regularBuilder1); writeFrame(maxStreamIdFrame, regularBuilder1); - writeFrame(QuicSimpleFrame(pingFrame), regularBuilder1); + writeFrame(pingFrame, regularBuilder1); writeAckFrame(ackMeta, regularBuilder1); writeStreamFrameHeader( regularBuilder1, @@ -435,8 +433,8 @@ TEST_F(QuicPacketRebuilderTest, CloneCounter) { RegularQuicPacketBuilder regularBuilder( kDefaultUDPSendPacketLen, std::move(shortHeader1), 0 /* largestAcked */); regularBuilder.encodePacketHeader(); - PingFrame pingFrame; - writeFrame(QuicSimpleFrame(pingFrame), regularBuilder); + MaxDataFrame maxDataFrame(31415926); + writeFrame(maxDataFrame, regularBuilder); auto packet = std::move(regularBuilder).buildPacket(); auto outstandingPacket = makeDummyOutstandingPacket(packet.packet, 1000); QuicServerConnectionState conn; @@ -451,6 +449,29 @@ TEST_F(QuicPacketRebuilderTest, CloneCounter) { EXPECT_EQ(1, conn.outstandings.clonedPacketsCount); } +TEST_F(QuicPacketRebuilderTest, PurePingWontRebuild) { + ShortHeader shortHeader1( + ProtectionType::KeyPhaseZero, getTestConnectionId(), 0); + RegularQuicPacketBuilder regularBuilder( + kDefaultUDPSendPacketLen, std::move(shortHeader1), 0); + regularBuilder.encodePacketHeader(); + PingFrame pingFrame; + writeFrame(pingFrame, regularBuilder); + auto packet = std::move(regularBuilder).buildPacket(); + auto outstandingPacket = makeDummyOutstandingPacket(packet.packet, 50); + EXPECT_EQ(1, outstandingPacket.packet.frames.size()); + QuicServerConnectionState conn; + ShortHeader shortHeader2( + ProtectionType::KeyPhaseZero, getTestConnectionId(), 0); + RegularQuicPacketBuilder regularBuilder2( + kDefaultUDPSendPacketLen, std::move(shortHeader2), 0); + regularBuilder2.encodePacketHeader(); + PacketRebuilder rebuilder(regularBuilder2, conn); + EXPECT_EQ(folly::none, rebuilder.rebuildFromPacket(outstandingPacket)); + EXPECT_FALSE(outstandingPacket.associatedEvent.has_value()); + EXPECT_EQ(0, conn.outstandings.clonedPacketsCount); +} + TEST_F(QuicPacketRebuilderTest, LastStreamFrameSkipLen) { QuicServerConnectionState conn; conn.streamManager->setMaxLocalBidirectionalStreams(100); diff --git a/quic/codec/test/QuicWriteCodecTest.cpp b/quic/codec/test/QuicWriteCodecTest.cpp index 5032c1255..6c0fce5b4 100644 --- a/quic/codec/test/QuicWriteCodecTest.cpp +++ b/quic/codec/test/QuicWriteCodecTest.cpp @@ -1293,20 +1293,20 @@ TEST_F(QuicWriteCodecTest, DecodeAppCloseLarge) { TEST_F(QuicWriteCodecTest, WritePing) { MockQuicPacketBuilder pktBuilder; setupCommonExpects(pktBuilder); - auto pingBytesWritten = writeFrame(QuicSimpleFrame(PingFrame()), pktBuilder); + auto pingBytesWritten = writeFrame(PingFrame(), pktBuilder); auto builtOut = std::move(pktBuilder).buildTestPacket(); auto regularPacket = builtOut.first; EXPECT_EQ(1, pingBytesWritten); - auto simpleFrame = regularPacket.frames[0].asQuicSimpleFrame(); - EXPECT_NE(simpleFrame->asPingFrame(), nullptr); + auto pingFrame = regularPacket.frames[0].asPingFrame(); + EXPECT_NE(pingFrame, nullptr); auto wireBuf = std::move(builtOut.second); BufQueue queue; queue.append(wireBuf->clone()); QuicFrame decodedFrame = parseQuicFrame(queue); - auto decodedSimpleFrame = decodedFrame.asQuicSimpleFrame(); - EXPECT_NE(decodedSimpleFrame->asPingFrame(), nullptr); + auto decodedPingFrame = decodedFrame.asPingFrame(); + EXPECT_NE(decodedPingFrame, nullptr); // At last, verify there is nothing left in the wire format bytes: EXPECT_EQ(queue.chainLength(), 0); @@ -1316,7 +1316,7 @@ TEST_F(QuicWriteCodecTest, NoSpaceForPing) { MockQuicPacketBuilder pktBuilder; pktBuilder.remaining_ = 0; setupCommonExpects(pktBuilder); - EXPECT_EQ(0, writeFrame(QuicSimpleFrame(PingFrame()), pktBuilder)); + EXPECT_EQ(0, writeFrame(PingFrame(), pktBuilder)); } TEST_F(QuicWriteCodecTest, WritePadding) { diff --git a/quic/fizz/client/test/QuicClientTransportTest.cpp b/quic/fizz/client/test/QuicClientTransportTest.cpp index f9f75151d..3bf347f10 100644 --- a/quic/fizz/client/test/QuicClientTransportTest.cpp +++ b/quic/fizz/client/test/QuicClientTransportTest.cpp @@ -4630,7 +4630,7 @@ TEST_F(QuicClientTransportVersionAndRetryTest, UnencryptedPing) { kDefaultUDPSendPacketLen, std::move(header), 0 /* largestAcked */); builder.encodePacketHeader(); DCHECK(builder.canBuildPacket()); - writeFrame(QuicWriteFrame(PingFrame()), builder); + writeFrame(PingFrame(), builder); auto packet = packetToBufCleartext( std::move(builder).buildPacket(), getInitialCipher(), @@ -5174,7 +5174,7 @@ TEST_F(QuicClientTransportAfterStartTest, SetCongestionControlBbr) { EXPECT_TRUE(isConnectionPaced(client->getConn())); } -TEST_F(QuicClientTransportAfterStartTest, PingIsRetransmittable) { +TEST_F(QuicClientTransportAfterStartTest, PingIsTreatedAsRetransmittable) { PingFrame pingFrame; ShortHeader header( ProtectionType::KeyPhaseZero, *originalConnId, appDataPacketNum++); @@ -5183,12 +5183,10 @@ TEST_F(QuicClientTransportAfterStartTest, PingIsRetransmittable) { std::move(header), 0 /* largestAcked */); builder.encodePacketHeader(); - writeFrame(QuicSimpleFrame(pingFrame), builder); + writeFrame(pingFrame, builder); auto packet = packetToBuf(std::move(builder).buildPacket()); deliverData(packet->coalesce()); EXPECT_TRUE(client->getConn().pendingEvents.scheduleAckTimeout); - EXPECT_FALSE(getAckState(client->getConn(), PacketNumberSpace::AppData) - .needsToSendAckImmediately); } TEST_F(QuicClientTransportAfterStartTest, OneCloseFramePerRtt) { diff --git a/quic/logging/BaseQLogger.cpp b/quic/logging/BaseQLogger.cpp index 0e695eb95..6b7292c73 100644 --- a/quic/logging/BaseQLogger.cpp +++ b/quic/logging/BaseQLogger.cpp @@ -7,10 +7,6 @@ void addQuicSimpleFrameToEvent( quic::QLogPacketEvent* event, const quic::QuicSimpleFrame& simpleFrame) { switch (simpleFrame.type()) { - case quic::QuicSimpleFrame::Type::PingFrame_E: { - event->frames.push_back(std::make_unique()); - break; - } case quic::QuicSimpleFrame::Type::StopSendingFrame_E: { const quic::StopSendingFrame& frame = *simpleFrame.asStopSendingFrame(); event->frames.push_back(std::make_unique( @@ -167,6 +163,9 @@ std::unique_ptr BaseQLogger::createPacketEvent( event->frames.push_back(std::make_unique()); break; } + case QuicFrame::Type::PingFrame_E: + event->frames.push_back(std::make_unique()); + break; case QuicFrame::Type::QuicSimpleFrame_E: { const auto& simpleFrame = *quicFrame.asQuicSimpleFrame(); addQuicSimpleFrameToEvent(event.get(), simpleFrame); diff --git a/quic/server/state/ServerStateMachine.cpp b/quic/server/state/ServerStateMachine.cpp index fec0078d2..a10c0234f 100644 --- a/quic/server/state/ServerStateMachine.cpp +++ b/quic/server/state/ServerStateMachine.cpp @@ -702,8 +702,7 @@ void onServerReadDataFromOpen( auto isAck = quicFrame.asReadAckFrame(); auto isClose = quicFrame.asConnectionCloseFrame(); auto isCrypto = quicFrame.asReadCryptoFrame(); - auto isSimple = quicFrame.asQuicSimpleFrame(); - auto isPing = isSimple ? isSimple->asPingFrame() : nullptr; + auto isPing = quicFrame.asPingFrame(); // TODO: add path challenge and response if (!isPadding && !isAck && !isClose && !isCrypto && !isPing) { QUIC_STATS( @@ -841,6 +840,9 @@ void onServerReadDataFromOpen( commonAckVisitorForAckFrame(ackState, frame); break; } + case QuicWriteFrame::Type::PingFrame_E: + conn.pendingEvents.cancelPingTimeout = true; + return; case QuicWriteFrame::Type::QuicSimpleFrame_E: { const QuicSimpleFrame& frame = *packetFrame.asQuicSimpleFrame(); @@ -850,8 +852,6 @@ void onServerReadDataFromOpen( // Call handshakeConfirmed outside of the packet // processing loop to avoid a re-entrancy. handshakeConfirmedThisLoop = true; - } else { - updateSimpleFrameOnAck(conn, frame); } break; } @@ -990,9 +990,14 @@ void onServerReadDataFromOpen( "Peer closed", TransportErrorCode::NO_ERROR); break; } - case QuicFrame::Type::PaddingFrame_E: { + case QuicFrame::Type::PingFrame_E: + isNonProbingPacket = true; + // Ping isn't retransmittable data. But we would like to ack them + // early. + pktHasRetransmittableData = true; + break; + case QuicFrame::Type::PaddingFrame_E: break; - } case QuicFrame::Type::QuicSimpleFrame_E: { pktHasRetransmittableData = true; QuicSimpleFrame& simpleFrame = *quicFrame.asQuicSimpleFrame(); diff --git a/quic/server/test/QuicServerTransportTest.cpp b/quic/server/test/QuicServerTransportTest.cpp index b55b3b6a4..8e0aca88c 100644 --- a/quic/server/test/QuicServerTransportTest.cpp +++ b/quic/server/test/QuicServerTransportTest.cpp @@ -3193,7 +3193,7 @@ TEST_F( EXPECT_TRUE(server->getConn().migrationState.lastCongestionAndRtt); } -TEST_F(QuicServerTransportTest, PingIsRetransmittable) { +TEST_F(QuicServerTransportTest, PingIsTreatedAsRetransmittable) { PingFrame pingFrame; ShortHeader header( ProtectionType::KeyPhaseZero, @@ -3204,12 +3204,10 @@ TEST_F(QuicServerTransportTest, PingIsRetransmittable) { std::move(header), 0 /* largestAcked */); builder.encodePacketHeader(); - writeFrame(QuicSimpleFrame(pingFrame), builder); + writeFrame(pingFrame, builder); auto packet = std::move(builder).buildPacket(); deliverData(packetToBuf(packet)); EXPECT_TRUE(server->getConn().pendingEvents.scheduleAckTimeout); - EXPECT_FALSE(getAckState(server->getConn(), PacketNumberSpace::AppData) - .needsToSendAckImmediately); } TEST_F(QuicServerTransportTest, RecvNewConnectionIdValid) { diff --git a/quic/state/SimpleFrameFunctions.cpp b/quic/state/SimpleFrameFunctions.cpp index e86e5fc2d..71158d6a0 100644 --- a/quic/state/SimpleFrameFunctions.cpp +++ b/quic/state/SimpleFrameFunctions.cpp @@ -18,25 +18,10 @@ void sendSimpleFrame(QuicConnectionStateBase& conn, QuicSimpleFrame frame) { conn.pendingEvents.frames.emplace_back(std::move(frame)); } -void updateSimpleFrameOnAck( - QuicConnectionStateBase& conn, - const QuicSimpleFrame& frame) { - switch (frame.type()) { - case QuicSimpleFrame::Type::PingFrame_E: { - conn.pendingEvents.cancelPingTimeout = true; - break; - } - default: - break; - } -} - folly::Optional updateSimpleFrameOnPacketClone( QuicConnectionStateBase& conn, const QuicSimpleFrame& frame) { switch (frame.type()) { - case QuicSimpleFrame::Type::PingFrame_E: - return QuicSimpleFrame(frame); case QuicSimpleFrame::Type::StopSendingFrame_E: if (!conn.streamManager->streamExists( frame.asStopSendingFrame()->streamId)) { @@ -102,9 +87,6 @@ void updateSimpleFrameOnPacketLoss( QuicConnectionStateBase& conn, const QuicSimpleFrame& frame) { switch (frame.type()) { - case QuicSimpleFrame::Type::PingFrame_E: { - break; - } case QuicSimpleFrame::Type::StopSendingFrame_E: { const StopSendingFrame& stopSendingFrame = *frame.asStopSendingFrame(); if (conn.streamManager->streamExists(stopSendingFrame.streamId)) { @@ -160,9 +142,6 @@ bool updateSimpleFrameOnPacketReceived( PacketNum packetNum, bool fromChangedPeerAddress) { switch (frame.type()) { - case QuicSimpleFrame::Type::PingFrame_E: { - return true; - } case QuicSimpleFrame::Type::StopSendingFrame_E: { const StopSendingFrame& stopSending = *frame.asStopSendingFrame(); auto stream = conn.streamManager->getStream(stopSending.streamId); @@ -291,8 +270,7 @@ bool updateSimpleFrameOnPacketReceived( return true; } case QuicSimpleFrame::Type::RetireConnectionIdFrame_E: { - // TODO junqiw - return false; + return true; } case QuicSimpleFrame::Type::HandshakeDoneFrame_E: { if (conn.nodeType == QuicNodeType::Server) { diff --git a/quic/state/SimpleFrameFunctions.h b/quic/state/SimpleFrameFunctions.h index b17ee3f32..45026e358 100644 --- a/quic/state/SimpleFrameFunctions.h +++ b/quic/state/SimpleFrameFunctions.h @@ -19,13 +19,6 @@ namespace quic { */ void sendSimpleFrame(QuicConnectionStateBase& conn, QuicSimpleFrame frame); -/* - * Update the connection state on ack of the given simple frame. - */ -void updateSimpleFrameOnAck( - QuicConnectionStateBase& conn, - const QuicSimpleFrame& frame); - /* * Update connection state and the frame on clone of the given simple frame. * Returns the updated simple frame. diff --git a/quic/state/StateData.h b/quic/state/StateData.h index b65101520..d949d9eca 100644 --- a/quic/state/StateData.h +++ b/quic/state/StateData.h @@ -664,6 +664,9 @@ struct QuicConnectionStateBase : public folly::DelayedDestruction { // close transport when the next packet number reaches kMaxPacketNum bool closeTransport{false}; + + // To send a ping frame + bool sendPing{false}; }; PendingEvents pendingEvents;