diff --git a/quic/api/QuicTransportBase.cpp b/quic/api/QuicTransportBase.cpp index 5c74cab6c..2698c1030 100644 --- a/quic/api/QuicTransportBase.cpp +++ b/quic/api/QuicTransportBase.cpp @@ -34,6 +34,7 @@ QuicTransportBase::QuicTransportBase( pathValidationTimeout_(this), idleTimeout_(this), drainTimeout_(this), + pingTimeout_(this), readLooper_(new FunctionLooper( evb, [this](bool /* ignored */) { invokeReadDataAndCallbacks(); }, @@ -316,6 +317,10 @@ void QuicTransportBase::closeImpl( if (idleTimeout_.isScheduled()) { idleTimeout_.cancelTimeout(); } + if (pingTimeout_.isScheduled()) { + pingTimeout_.cancelTimeout(); + } + VLOG(10) << "Stopping read looper due to immediate close " << *this; readLooper_->stop(); peekLooper_->stop(); @@ -1397,6 +1402,22 @@ folly:: } } +void QuicTransportBase::handlePingCallback() { + if (!conn_->pendingEvents.cancelPingTimeout) { + return; // nothing to cancel + } + if (!pingTimeout_.isScheduled()) { + // set cancelpingTimeOut to false, delayed acks + conn_->pendingEvents.cancelPingTimeout = false; + return; // nothing to do, as timeout has already fired + } + pingTimeout_.cancelTimeout(); + if (pingCallback_ != nullptr) { + runOnEvbAsync([](auto self) { self->pingCallback_->pingAcknowledged(); }); + } + conn_->pendingEvents.cancelPingTimeout = false; +} + void QuicTransportBase::processCallbacksAfterNetworkData() { if (UNLIKELY(closeState_ != CloseState::OPEN)) { return; @@ -1414,6 +1435,10 @@ void QuicTransportBase::processCallbacksAfterNetworkData() { } } conn_->streamManager->clearNewPeerStreams(); + + // Handle pingCallbacks + handlePingCallback(); + // TODO: we're currently assuming that canceling write callbacks will not // cause reset of random streams. Maybe get rid of that assumption later. for (auto pendingResetIt = conn_->pendingEvents.resets.begin(); @@ -2006,8 +2031,20 @@ void QuicTransportBase::checkForClosedStream() { } void QuicTransportBase::sendPing( - PingCallback* /*callback*/, - std::chrono::milliseconds /*pingTimeout*/) {} + PingCallback* callback, + std::chrono::milliseconds pingTimeout) { + /* Step 0: Connection should not be closed */ + if (closeState_ == CloseState::CLOSED) { + return; + } + + // Step 1: Send a simple ping frame + quic::sendSimpleFrame(*conn_, PingFrame()); + updateWriteLooper(true); + + // Step 2: Schedule the timeout on event base + schedulePingTimeout(callback, pingTimeout); +} void QuicTransportBase::lossTimeoutExpired() noexcept { CHECK_NE(closeState_, CloseState::CLOSED); @@ -2045,6 +2082,14 @@ void QuicTransportBase::ackTimeoutExpired() noexcept { pacedWriteDataToSocket(false); } +void QuicTransportBase::pingTimeoutExpired() noexcept { + // If timeout expired just call the call back Provided + if (pingCallback_ == nullptr) { + return; + } + runOnEvbAsync([](auto self) { self->pingCallback_->pingTimeout(); }); +} + void QuicTransportBase::pathValidationTimeoutExpired() noexcept { CHECK(conn_->outstandingPathValidation); @@ -2110,6 +2155,19 @@ void QuicTransportBase::scheduleAckTimeout() { } } +void QuicTransportBase::schedulePingTimeout( + PingCallback* pingCb, + std::chrono::milliseconds timeout) { + // if a ping timeout is already scheduled, nothing to do, return + if (pingTimeout_.isScheduled()) { + return; + } + + pingCallback_ = pingCb; + auto& wheelTimer = getEventBase()->timer(); + wheelTimer.scheduleTimeout(&pingTimeout_, timeout); +} + void QuicTransportBase::schedulePathValidationTimeout() { if (closeState_ == CloseState::CLOSED) { return; diff --git a/quic/api/QuicTransportBase.h b/quic/api/QuicTransportBase.h index 22f311a5e..7df86e197 100644 --- a/quic/api/QuicTransportBase.h +++ b/quic/api/QuicTransportBase.h @@ -367,6 +367,26 @@ class QuicTransportBase : public QuicSocket { QuicTransportBase* transport_; }; + class PingTimeout : public folly::HHWheelTimer::Callback { + public: + ~PingTimeout() override = default; + + explicit PingTimeout(QuicTransportBase* transport) + : transport_(transport) {} + + void timeoutExpired() noexcept override { + transport_->pingTimeoutExpired(); + } + + void callbackCanceled() noexcept override { + // ignore, as this happens only when event base dies + return; + } + + private: + QuicTransportBase* transport_; + }; + class PathValidationTimeout : public folly::HHWheelTimer::Callback { public: ~PathValidationTimeout() override = default; @@ -460,6 +480,7 @@ class QuicTransportBase : public QuicSocket { void updateReadLooper(); void updatePeekLooper(); void updateWriteLooper(bool thisIteration); + void handlePingCallback(); void runOnEvbAsync( folly::Function)> func); @@ -521,10 +542,14 @@ class QuicTransportBase : public QuicSocket { void pathValidationTimeoutExpired() noexcept; void idleTimeoutExpired(bool drain) noexcept; void drainTimeoutExpired() noexcept; + void pingTimeoutExpired() noexcept; void setIdleTimer(); void scheduleAckTimeout(); void schedulePathValidationTimeout(); + void schedulePingTimeout( + PingCallback* callback, + std::chrono::milliseconds pingTimeout); std::atomic evb_; std::unique_ptr socket_; @@ -570,6 +595,7 @@ class QuicTransportBase : public QuicSocket { deliveryCallbacks_; std::unordered_map dataExpiredCallbacks_; std::unordered_map dataRejectedCallbacks_; + PingCallback* pingCallback_; WriteCallback* connWriteCallback_{nullptr}; std::map pendingWriteCallbacks_; @@ -581,6 +607,7 @@ class QuicTransportBase : public QuicSocket { PathValidationTimeout pathValidationTimeout_; IdleTimeout idleTimeout_; DrainTimeout drainTimeout_; + PingTimeout pingTimeout_; FunctionLooper::Ptr readLooper_; FunctionLooper::Ptr peekLooper_; FunctionLooper::Ptr writeLooper_; diff --git a/quic/api/test/QuicPacketSchedulerTest.cpp b/quic/api/test/QuicPacketSchedulerTest.cpp index adae47e6c..ba433893c 100644 --- a/quic/api/test/QuicPacketSchedulerTest.cpp +++ b/quic/api/test/QuicPacketSchedulerTest.cpp @@ -369,7 +369,7 @@ TEST_F(QuicPacketSchedulerTest, WriteOnlyOutstandingPacketsTest) { // Write those framses with a regular builder writeFrame(connCloseFrame, regularBuilder); writeFrame(QuicSimpleFrame(maxStreamFrame), regularBuilder); - writeFrame(pingFrame, regularBuilder); + writeFrame(QuicSimpleFrame(pingFrame), regularBuilder); writeAckFrame(ackMeta, regularBuilder); auto result = cloningScheduler.scheduleFramesForPacket( @@ -395,7 +395,7 @@ TEST_F(QuicPacketSchedulerTest, WriteOnlyOutstandingPacketsTest) { /* the next four frames should not be written */ present |= frame.asConnectionCloseFrame() ? true : false; present |= frame.asQuicSimpleFrame() ? true : false; - present |= frame.asPingFrame() ? true : false; + present |= frame.asQuicSimpleFrame() ? true : false; present |= frame.asWriteAckFrame() ? true : false; ASSERT_FALSE(present); } diff --git a/quic/api/test/QuicTransportBaseTest.cpp b/quic/api/test/QuicTransportBaseTest.cpp index f53a6f037..2931c7cc6 100644 --- a/quic/api/test/QuicTransportBaseTest.cpp +++ b/quic/api/test/QuicTransportBaseTest.cpp @@ -11,6 +11,7 @@ #include #include +#include #include #include #include @@ -227,7 +228,7 @@ class TestQuicTransport transportClosed = true; } - void invokeAckTimeout() { + void AckTimeout() { ackTimeoutExpired(); } @@ -239,6 +240,31 @@ class TestQuicTransport idleTimeout_.timeoutExpired(); } + void invokeAckTimeout() { + ackTimeout_.timeoutExpired(); + } + + void invokeSendPing( + quic::QuicSocket::PingCallback* cb, + std::chrono::milliseconds interval) { + sendPing(cb, interval); + } + + void invokeCancelPingTimeout() { + pingTimeout_.cancelTimeout(); + } + + void invokeHandlePingCallback() { + handlePingCallback(); + } + + bool isPingTimeoutScheduled() { + if (pingTimeout_.isScheduled()) { + return true; + } + return false; + } + auto& writeLooper() { return writeLooper_; } @@ -2428,5 +2454,29 @@ TEST_F(QuicTransportImplTest, CloseFromCancelDeliveryCallbacksForStream) { transport->cancelDeliveryCallbacksForStream(stream1); } +TEST_F(QuicTransportImplTest, SuccessfulPing) { + auto conn = transport->transportConn; + std::chrono::milliseconds interval(10); + transport->invokeSendPing(nullptr, interval); + EXPECT_EQ(transport->isPingTimeoutScheduled(), true); + EXPECT_EQ(conn->pendingEvents.cancelPingTimeout, false); + conn->pendingEvents.cancelPingTimeout = true; + transport->invokeHandlePingCallback(); + EXPECT_EQ(transport->isPingTimeoutScheduled(), false); + EXPECT_EQ(conn->pendingEvents.cancelPingTimeout, false); +} + +TEST_F(QuicTransportImplTest, FailedPing) { + auto conn = transport->transportConn; + std::chrono::milliseconds interval(10); + transport->invokeSendPing(nullptr, interval); + EXPECT_EQ(transport->isPingTimeoutScheduled(), true); + EXPECT_EQ(conn->pendingEvents.cancelPingTimeout, false); + conn->pendingEvents.cancelPingTimeout = true; + transport->invokeCancelPingTimeout(); + transport->invokeHandlePingCallback(); + EXPECT_EQ(conn->pendingEvents.cancelPingTimeout, false); +} + } // namespace test } // namespace quic diff --git a/quic/client/QuicClientTransport.cpp b/quic/client/QuicClientTransport.cpp index 7937751f4..07dae4dd7 100644 --- a/quic/client/QuicClientTransport.cpp +++ b/quic/client/QuicClientTransport.cpp @@ -20,6 +20,7 @@ #include #include #include +#include namespace fsp = folly::portability::sockets; @@ -351,6 +352,12 @@ void QuicClientTransport::processPacketData( *cryptoStream, frame.offset, frame.len); break; } + case QuicWriteFrame::Type::QuicSimpleFrame_E: { + const quic::QuicSimpleFrame simpleFrame = + *packetFrame.asQuicSimpleFrame(); + updateSimpleFrameOnAck(*conn_, simpleFrame); + break; + } default: // ignore other frames. break; @@ -502,10 +509,6 @@ void QuicClientTransport::processPacketData( *conn_, simpleFrame, packetNum, false); break; } - case QuicFrame::Type::PingFrame_E: { - pktHasRetransmittableData = true; - break; - } default: break; } diff --git a/quic/client/test/QuicClientTransportTest.cpp b/quic/client/test/QuicClientTransportTest.cpp index 85c22c6fb..008d216b7 100644 --- a/quic/client/test/QuicClientTransportTest.cpp +++ b/quic/client/test/QuicClientTransportTest.cpp @@ -4478,7 +4478,7 @@ TEST_F(QuicClientTransportAfterStartTest, PingIsRetransmittable) { client->getConn().udpSendPacketLen, std::move(header), 0 /* largestAcked */); - writeFrame(pingFrame, builder); + writeFrame(QuicSimpleFrame(pingFrame), builder); auto packet = packetToBuf(std::move(builder).buildPacket()); deliverData(packet->coalesce()); EXPECT_TRUE(client->getConn().pendingEvents.scheduleAckTimeout); diff --git a/quic/codec/Decode.cpp b/quic/codec/Decode.cpp index d47fb76fe..1a2400406 100644 --- a/quic/codec/Decode.cpp +++ b/quic/codec/Decode.cpp @@ -60,7 +60,7 @@ PaddingFrame decodePaddingFrame(folly::io::Cursor&) { return PaddingFrame(); } -PingFrame decodePingFrame(folly::io::Cursor& /* cursor */) { +PingFrame decodePingFrame(folly::io::Cursor&) { return PingFrame(); } diff --git a/quic/codec/QuicWriteCodec.cpp b/quic/codec/QuicWriteCodec.cpp index 2b1d1dfa8..523197bc2 100644 --- a/quic/codec/QuicWriteCodec.cpp +++ b/quic/codec/QuicWriteCodec.cpp @@ -318,6 +318,17 @@ 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)); @@ -495,17 +506,6 @@ size_t writeFrame(QuicWriteFrame&& frame, PacketBuilderInterface& builder) { } return size_t(0); } - case QuicWriteFrame::Type::PingFrame_E: { - PingFrame& pingFrame = *frame.asPingFrame(); - QuicInteger intFrameType(static_cast(FrameType::PING)); - if (packetSpaceCheck(spaceLeft, intFrameType.getSize())) { - builder.write(intFrameType); - builder.appendFrame(std::move(pingFrame)); - return intFrameType.getSize(); - } - // no space left in packet - return size_t(0); - } case QuicWriteFrame::Type::RstStreamFrame_E: { RstStreamFrame& rstStreamFrame = *frame.asRstStreamFrame(); QuicInteger intFrameType(static_cast(FrameType::RST_STREAM)); diff --git a/quic/codec/Types.h b/quic/codec/Types.h index 89164af4b..1e02a4900 100644 --- a/quic/codec/Types.h +++ b/quic/codec/Types.h @@ -564,15 +564,16 @@ struct StatelessReset { : token(std::move(tokenIn)) {} }; -#define QUIC_SIMPLE_FRAME(F, ...) \ - F(StopSendingFrame, __VA_ARGS__) \ - F(MinStreamDataFrame, __VA_ARGS__) \ - F(ExpiredStreamDataFrame, __VA_ARGS__) \ - F(PathChallengeFrame, __VA_ARGS__) \ - F(PathResponseFrame, __VA_ARGS__) \ - F(NewConnectionIdFrame, __VA_ARGS__) \ - F(MaxStreamsFrame, __VA_ARGS__) \ - F(RetireConnectionIdFrame, __VA_ARGS__) +#define QUIC_SIMPLE_FRAME(F, ...) \ + F(StopSendingFrame, __VA_ARGS__) \ + F(MinStreamDataFrame, __VA_ARGS__) \ + F(ExpiredStreamDataFrame, __VA_ARGS__) \ + F(PathChallengeFrame, __VA_ARGS__) \ + F(PathResponseFrame, __VA_ARGS__) \ + F(NewConnectionIdFrame, __VA_ARGS__) \ + F(MaxStreamsFrame, __VA_ARGS__) \ + F(RetireConnectionIdFrame, __VA_ARGS__) \ + F(PingFrame, __VA_ARGS__) DECLARE_VARIANT_TYPE(QuicSimpleFrame, QUIC_SIMPLE_FRAME) @@ -583,7 +584,6 @@ DECLARE_VARIANT_TYPE(QuicSimpleFrame, QUIC_SIMPLE_FRAME) F(ApplicationCloseFrame, __VA_ARGS__) \ F(MaxDataFrame, __VA_ARGS__) \ F(MaxStreamDataFrame, __VA_ARGS__) \ - F(PingFrame, __VA_ARGS__) \ F(DataBlockedFrame, __VA_ARGS__) \ F(StreamDataBlockedFrame, __VA_ARGS__) \ F(StreamsBlockedFrame, __VA_ARGS__) \ @@ -603,7 +603,6 @@ DECLARE_VARIANT_TYPE(QuicFrame, QUIC_FRAME) F(ApplicationCloseFrame, __VA_ARGS__) \ F(MaxDataFrame, __VA_ARGS__) \ F(MaxStreamDataFrame, __VA_ARGS__) \ - F(PingFrame, __VA_ARGS__) \ F(DataBlockedFrame, __VA_ARGS__) \ F(StreamDataBlockedFrame, __VA_ARGS__) \ F(StreamsBlockedFrame, __VA_ARGS__) \ @@ -837,7 +836,8 @@ struct VersionNegotiationPacket { struct RegularPacket { PacketHeader header; - explicit RegularPacket(PacketHeader&& headerIn) : header(std::move(headerIn)) {} + explicit RegularPacket(PacketHeader&& headerIn) + : header(std::move(headerIn)) {} }; /** diff --git a/quic/codec/test/QuicPacketRebuilderTest.cpp b/quic/codec/test/QuicPacketRebuilderTest.cpp index 4472021a7..fd5b19554 100644 --- a/quic/codec/test/QuicPacketRebuilderTest.cpp +++ b/quic/codec/test/QuicPacketRebuilderTest.cpp @@ -62,7 +62,6 @@ TEST_F(QuicPacketRebuilderTest, RebuildPacket) { "The sun is in the sky.", FrameType::ACK); MaxStreamsFrame maxStreamsFrame(4321, true); - PingFrame pingFrame; IntervalSet ackBlocks; ackBlocks.insert(10, 100); ackBlocks.insert(200, 1000); @@ -78,10 +77,11 @@ TEST_F(QuicPacketRebuilderTest, RebuildPacket) { uint64_t cryptoOffset = 0; auto cryptoBuf = folly::IOBuf::copyBuffer("NewSessionTicket"); + PingFrame pingFrame{}; // Write them with a regular builder writeFrame(connCloseFrame, regularBuilder1); writeFrame(QuicSimpleFrame(maxStreamsFrame), regularBuilder1); - writeFrame(pingFrame, regularBuilder1); + writeFrame(QuicSimpleFrame(pingFrame), regularBuilder1); writeAckFrame(ackMeta, regularBuilder1); writeStreamFrameHeader( regularBuilder1, @@ -135,14 +135,22 @@ TEST_F(QuicPacketRebuilderTest, RebuildPacket) { } case QuicWriteFrame::Type::QuicSimpleFrame_E: { const QuicSimpleFrame& simpleFrame = *frame.asQuicSimpleFrame(); - const MaxStreamsFrame* maxStreamFrame = simpleFrame.asMaxStreamsFrame(); - EXPECT_NE(maxStreamFrame, nullptr); - EXPECT_EQ(4321, maxStreamFrame->maxStreams); - break; - } - case QuicWriteFrame::Type::PingFrame_E: { - const PingFrame& ping = *frame.asPingFrame(); - EXPECT_EQ(PingFrame(), ping); + switch (simpleFrame.type()) { + case QuicSimpleFrame::Type::MaxStreamsFrame_E: { + const MaxStreamsFrame* maxStreamFrame = + simpleFrame.asMaxStreamsFrame(); + EXPECT_NE(maxStreamFrame, nullptr); + 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 */ + } break; } case QuicWriteFrame::Type::WriteAckFrame_E: { @@ -356,7 +364,6 @@ TEST_F(QuicPacketRebuilderTest, CannotRebuild) { "The sun is in the sky.", FrameType::ACK); StreamsBlockedFrame maxStreamIdFrame(0x1024, true); - PingFrame pingFrame; IntervalSet ackBlocks; ackBlocks.insert(10, 100); ackBlocks.insert(200, 1000); @@ -367,11 +374,11 @@ TEST_F(QuicPacketRebuilderTest, CannotRebuild) { auto streamId = stream->id; auto buf = folly::IOBuf::copyBuffer("You can't deny you are looking for the sunset"); - + PingFrame pingFrame; // Write them with a regular builder writeFrame(connCloseFrame, regularBuilder1); writeFrame(maxStreamIdFrame, regularBuilder1); - writeFrame(pingFrame, regularBuilder1); + writeFrame(QuicSimpleFrame(pingFrame), regularBuilder1); writeAckFrame(ackMeta, regularBuilder1); writeStreamFrameHeader( regularBuilder1, @@ -407,7 +414,7 @@ TEST_F(QuicPacketRebuilderTest, CloneCounter) { RegularQuicPacketBuilder regularBuilder( kDefaultUDPSendPacketLen, std::move(shortHeader1), 0 /* largestAcked */); PingFrame pingFrame; - writeFrame(pingFrame, regularBuilder); + writeFrame(QuicSimpleFrame(pingFrame), regularBuilder); auto packet = std::move(regularBuilder).buildPacket(); auto outstandingPacket = makeDummyOutstandingPacket(packet.packet, 1000); QuicServerConnectionState conn; diff --git a/quic/codec/test/QuicWriteCodecTest.cpp b/quic/codec/test/QuicWriteCodecTest.cpp index 65f704048..2258475d8 100644 --- a/quic/codec/test/QuicWriteCodecTest.cpp +++ b/quic/codec/test/QuicWriteCodecTest.cpp @@ -1115,17 +1115,19 @@ TEST_F(QuicWriteCodecTest, DecodeAppCloseLarge) { TEST_F(QuicWriteCodecTest, WritePing) { MockQuicPacketBuilder pktBuilder; setupCommonExpects(pktBuilder); - auto pingBytesWritten = writeFrame(PingFrame(), pktBuilder); + auto pingBytesWritten = writeFrame(QuicSimpleFrame(PingFrame()), pktBuilder); auto builtOut = std::move(pktBuilder).buildPacket(); auto regularPacket = builtOut.first; EXPECT_EQ(1, pingBytesWritten); - EXPECT_NE(regularPacket.frames[0].asPingFrame(), nullptr); + auto simpleFrame = regularPacket.frames[0].asQuicSimpleFrame(); + EXPECT_NE(simpleFrame->asPingFrame(), nullptr); auto wireBuf = std::move(builtOut.second); folly::io::Cursor cursor(wireBuf.get()); QuicFrame decodedFrame = parseQuicFrame(cursor); - EXPECT_NE(decodedFrame.asPingFrame(), nullptr); + auto decodedSimpleFrame = decodedFrame.asQuicSimpleFrame(); + EXPECT_NE(decodedSimpleFrame->asPingFrame(), nullptr); // At last, verify there is nothing left in the wire format bytes: EXPECT_TRUE(cursor.isAtEnd()); @@ -1135,7 +1137,7 @@ TEST_F(QuicWriteCodecTest, NoSpaceForPing) { MockQuicPacketBuilder pktBuilder; pktBuilder.remaining_ = 0; setupCommonExpects(pktBuilder); - EXPECT_EQ(0, writeFrame(PingFrame(), pktBuilder)); + EXPECT_EQ(0, writeFrame(QuicSimpleFrame(PingFrame()), pktBuilder)); } TEST_F(QuicWriteCodecTest, WritePadding) { diff --git a/quic/logging/BaseQLogger.cpp b/quic/logging/BaseQLogger.cpp index e5dbbbf10..e560b1a2b 100644 --- a/quic/logging/BaseQLogger.cpp +++ b/quic/logging/BaseQLogger.cpp @@ -5,6 +5,10 @@ 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( @@ -123,10 +127,6 @@ std::unique_ptr BaseQLogger::createPacketEvent( frame.streamId, frame.maximumData)); break; } - case QuicFrame::Type::PingFrame_E: { - event->frames.push_back(std::make_unique()); - break; - } case QuicFrame::Type::DataBlockedFrame_E: { const auto& frame = *quicFrame.asDataBlockedFrame(); event->frames.push_back( @@ -244,9 +244,6 @@ std::unique_ptr BaseQLogger::createPacketEvent( frame.streamLimit, frame.isForBidirectional)); break; } - case QuicWriteFrame::Type::PingFrame_E: - event->frames.push_back(std::make_unique()); - break; case QuicWriteFrame::Type::DataBlockedFrame_E: { const DataBlockedFrame& frame = *quicFrame.asDataBlockedFrame(); event->frames.push_back( diff --git a/quic/server/state/ServerStateMachine.cpp b/quic/server/state/ServerStateMachine.cpp index b8d978852..fe797b0e3 100644 --- a/quic/server/state/ServerStateMachine.cpp +++ b/quic/server/state/ServerStateMachine.cpp @@ -16,6 +16,7 @@ #include #include #include +#include namespace quic { using namespace std::chrono_literals; @@ -800,6 +801,12 @@ void onServerReadDataFromOpen( commonAckVisitorForAckFrame(ackState, frame); break; } + case QuicWriteFrame::Type::QuicSimpleFrame_E: { + const QuicSimpleFrame& frame = + *packetFrame.asQuicSimpleFrame(); + updateSimpleFrameOnAck(conn, frame); + break; + } default: { break; } @@ -964,11 +971,6 @@ void onServerReadDataFromOpen( conn, simpleFrame, packetNum, readData.peer != conn.peerAddress); break; } - case QuicFrame::Type::PingFrame_E: { - pktHasRetransmittableData = true; - isNonProbingPacket = true; - break; - } default: { break; } diff --git a/quic/server/test/QuicServerTransportTest.cpp b/quic/server/test/QuicServerTransportTest.cpp index 3a515013f..24f7b902c 100644 --- a/quic/server/test/QuicServerTransportTest.cpp +++ b/quic/server/test/QuicServerTransportTest.cpp @@ -2779,7 +2779,7 @@ TEST_F(QuicServerTransportTest, PingIsRetransmittable) { server->getConn().udpSendPacketLen, std::move(header), 0 /* largestAcked */); - writeFrame(pingFrame, builder); + writeFrame(QuicSimpleFrame(pingFrame), builder); auto packet = std::move(builder).buildPacket(); deliverData(packetToBuf(packet)); EXPECT_TRUE(server->getConn().pendingEvents.scheduleAckTimeout); diff --git a/quic/state/SimpleFrameFunctions.cpp b/quic/state/SimpleFrameFunctions.cpp index 90e42c278..b211b8e7b 100644 --- a/quic/state/SimpleFrameFunctions.cpp +++ b/quic/state/SimpleFrameFunctions.cpp @@ -17,15 +17,25 @@ void sendSimpleFrame(QuicConnectionStateBase& conn, QuicSimpleFrame frame) { } void updateSimpleFrameOnAck( - QuicConnectionStateBase& /*conn*/, - const QuicSimpleFrame& /*frame*/) { + QuicConnectionStateBase& conn, + const QuicSimpleFrame& frame) { // TODO implement. + 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)) { @@ -87,6 +97,9 @@ 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)) { @@ -138,6 +151,9 @@ 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); diff --git a/quic/state/StateData.h b/quic/state/StateData.h index 9585c5979..cfbcd0ae5 100644 --- a/quic/state/StateData.h +++ b/quic/state/StateData.h @@ -598,6 +598,8 @@ struct QuicConnectionStateBase { // Number of probing packets to send after PTO uint8_t numProbePackets{0}; + + bool cancelPingTimeout{false}; }; PendingEvents pendingEvents;