diff --git a/quic/QuicConstants.h b/quic/QuicConstants.h index f7819ecab..b74267b1c 100644 --- a/quic/QuicConstants.h +++ b/quic/QuicConstants.h @@ -194,6 +194,7 @@ enum class FrameType : uint64_t { DATAGRAM = 0x30, DATAGRAM_LEN = 0x31, KNOB = 0x1550, + IMMEDIATE_ACK = 0xAC, ACK_FREQUENCY = 0xAF, // Stream groups. GROUP_STREAM = 0x32, diff --git a/quic/api/QuicPacketScheduler.cpp b/quic/api/QuicPacketScheduler.cpp index 943c72f24..58a7a9e14 100644 --- a/quic/api/QuicPacketScheduler.cpp +++ b/quic/api/QuicPacketScheduler.cpp @@ -186,6 +186,11 @@ FrameScheduler::Builder& FrameScheduler::Builder::datagramFrames() { return *this; } +FrameScheduler::Builder& FrameScheduler::Builder::immediateAckFrames() { + immediateAckFrameScheduler_ = true; + return *this; +} + FrameScheduler FrameScheduler::Builder::build() && { FrameScheduler scheduler(name_, conn_); if (streamFrameScheduler_) { @@ -217,6 +222,10 @@ FrameScheduler FrameScheduler::Builder::build() && { if (datagramFrameScheduler_) { scheduler.datagramFrameScheduler_.emplace(DatagramFrameScheduler(conn_)); } + if (immediateAckFrameScheduler_) { + scheduler.immediateAckFrameScheduler_.emplace( + ImmediateAckFrameScheduler(conn_)); + } return scheduler; } @@ -259,6 +268,13 @@ SchedulingResult FrameScheduler::scheduleFramesForPacket( ackScheduler_->writeNextAcks(builder); } } + // Immediate ACK frames are subject to congestion control but should be sent + // before other frames to maximize their chance of being included in the + // packet since they are time sensitive + if (immediateAckFrameScheduler_ && + immediateAckFrameScheduler_->hasPendingImmediateAckFrame()) { + immediateAckFrameScheduler_->writeImmediateAckFrame(wrapper); + } if (windowUpdateScheduler_ && windowUpdateScheduler_->hasPendingWindowUpdates()) { windowUpdateScheduler_->writeWindowUpdates(wrapper); @@ -317,7 +333,7 @@ void FrameScheduler::writeNextAcks(PacketBuilderInterface& builder) { } bool FrameScheduler::hasData() const { - return (hasPendingAcks()) || hasImmediateData(); + return hasPendingAcks() || hasImmediateData(); } bool FrameScheduler::hasPendingAcks() const { @@ -335,7 +351,9 @@ bool FrameScheduler::hasImmediateData() const { simpleFrameScheduler_->hasPendingSimpleFrames()) || (pingFrameScheduler_ && pingFrameScheduler_->hasPingFrame()) || (datagramFrameScheduler_ && - datagramFrameScheduler_->hasPendingDatagramFrames()); + datagramFrameScheduler_->hasPendingDatagramFrames()) || + (immediateAckFrameScheduler_ && + immediateAckFrameScheduler_->hasPendingImmediateAckFrame()); } folly::StringPiece FrameScheduler::name() const { @@ -779,6 +797,19 @@ bool CryptoStreamScheduler::hasData() const { !cryptoStream_.lossBuffer.empty(); } +ImmediateAckFrameScheduler::ImmediateAckFrameScheduler( + const QuicConnectionStateBase& conn) + : conn_(conn) {} + +bool ImmediateAckFrameScheduler::hasPendingImmediateAckFrame() const { + return conn_.pendingEvents.requestImmediateAck; +} + +bool ImmediateAckFrameScheduler::writeImmediateAckFrame( + PacketBuilderInterface& builder) { + return 0 != writeFrame(ImmediateAckFrame(), builder); +} + CloningScheduler::CloningScheduler( FrameScheduler& scheduler, QuicConnectionStateBase& conn, diff --git a/quic/api/QuicPacketScheduler.h b/quic/api/QuicPacketScheduler.h index 795896633..e7de51e9a 100644 --- a/quic/api/QuicPacketScheduler.h +++ b/quic/api/QuicPacketScheduler.h @@ -242,6 +242,18 @@ class CryptoStreamScheduler { const QuicCryptoStream& cryptoStream_; }; +class ImmediateAckFrameScheduler { + public: + explicit ImmediateAckFrameScheduler(const QuicConnectionStateBase& conn); + + [[nodiscard]] bool hasPendingImmediateAckFrame() const; + + bool writeImmediateAckFrame(PacketBuilderInterface& builder); + + private: + const QuicConnectionStateBase& conn_; +}; + class FrameScheduler : public QuicPacketScheduler { public: ~FrameScheduler() override = default; @@ -262,6 +274,7 @@ class FrameScheduler : public QuicPacketScheduler { Builder& simpleFrames(); Builder& pingFrames(); Builder& datagramFrames(); + Builder& immediateAckFrames(); FrameScheduler build() &&; @@ -281,6 +294,7 @@ class FrameScheduler : public QuicPacketScheduler { bool simpleFrameScheduler_{false}; bool pingFrameScheduler_{false}; bool datagramFrameScheduler_{false}; + bool immediateAckFrameScheduler_{false}; }; FrameScheduler(folly::StringPiece name, QuicConnectionStateBase& conn); @@ -313,6 +327,7 @@ class FrameScheduler : public QuicPacketScheduler { folly::Optional simpleFrameScheduler_; folly::Optional pingFrameScheduler_; folly::Optional datagramFrameScheduler_; + folly::Optional immediateAckFrameScheduler_; folly::StringPiece name_; QuicConnectionStateBase& conn_; }; diff --git a/quic/api/QuicTransportFunctions.cpp b/quic/api/QuicTransportFunctions.cpp index 9c242ff01..1bfc6bd62 100644 --- a/quic/api/QuicTransportFunctions.cpp +++ b/quic/api/QuicTransportFunctions.cpp @@ -151,7 +151,8 @@ WriteQuicDataResult writeQuicDataToSocketImpl( .simpleFrames() .resetFrames() .streamFrames() - .pingFrames(); + .pingFrames() + .immediateAckFrames(); if (!exceptCryptoStream) { probeSchedulerBuilder.cryptoFrames(); } @@ -187,7 +188,8 @@ WriteQuicDataResult writeQuicDataToSocketImpl( .blockedFrames() .simpleFrames() .pingFrames() - .datagramFrames(); + .datagramFrames() + .immediateAckFrames(); if (!exceptCryptoStream) { schedulerBuilder.cryptoFrames(); } @@ -832,6 +834,12 @@ void updateConnection( // do not mark Datagram frames as retransmittable break; } + case QuicWriteFrame::Type::ImmediateAckFrame: { + // do not mark immediate acks as retranmittable. + // turn off the immediate ack pending event. + conn.pendingEvents.requestImmediateAck = false; + break; + } default: retransmittable = true; } diff --git a/quic/api/test/QuicPacketSchedulerTest.cpp b/quic/api/test/QuicPacketSchedulerTest.cpp index 22c5fe87c..5b7785ab9 100644 --- a/quic/api/test/QuicPacketSchedulerTest.cpp +++ b/quic/api/test/QuicPacketSchedulerTest.cpp @@ -1235,6 +1235,35 @@ TEST_F(QuicPacketSchedulerTest, LargestAckToSend) { EXPECT_EQ(folly::none, largestAckToSend(conn.ackStates.appDataAckState)); } +TEST_F(QuicPacketSchedulerTest, NeedsToSendAckWithoutAcksAvailable) { + // This covers the scheduler behavior when an IMMEDIATE_ACK frame is received. + QuicClientConnectionState conn( + FizzClientQuicHandshakeContext::Builder().build()); + AckScheduler initialAckScheduler( + conn, getAckState(conn, PacketNumberSpace::Initial)); + AckScheduler handshakeAckScheduler( + conn, getAckState(conn, PacketNumberSpace::Handshake)); + AckScheduler appDataAckScheduler( + conn, getAckState(conn, PacketNumberSpace::AppData)); + EXPECT_FALSE(initialAckScheduler.hasPendingAcks()); + EXPECT_FALSE(handshakeAckScheduler.hasPendingAcks()); + EXPECT_FALSE(appDataAckScheduler.hasPendingAcks()); + + conn.ackStates.initialAckState.needsToSendAckImmediately = true; + conn.ackStates.handshakeAckState.needsToSendAckImmediately = true; + conn.ackStates.appDataAckState.needsToSendAckImmediately = true; + + conn.ackStates.initialAckState.acks.insert(0, 100); + EXPECT_TRUE(initialAckScheduler.hasPendingAcks()); + + conn.ackStates.handshakeAckState.acks.insert(0, 100); + conn.ackStates.handshakeAckState.largestAckScheduled = 200; + EXPECT_FALSE(handshakeAckScheduler.hasPendingAcks()); + + conn.ackStates.handshakeAckState.largestAckScheduled = folly::none; + EXPECT_TRUE(handshakeAckScheduler.hasPendingAcks()); +} + TEST_F(QuicPacketSchedulerTest, StreamFrameSchedulerAllFit) { QuicClientConnectionState conn( FizzClientQuicHandshakeContext::Builder().build()); @@ -2300,6 +2329,80 @@ TEST_F(QuicPacketSchedulerTest, ShortHeaderPaddingMaxPacketLength) { EXPECT_EQ(packetLength, conn.udpSendPacketLen); } +TEST_F(QuicPacketSchedulerTest, ImmediateAckFrameSchedulerOnRequest) { + QuicClientConnectionState conn( + FizzClientQuicHandshakeContext::Builder().build()); + conn.pendingEvents.requestImmediateAck = true; + auto connId = getTestConnectionId(); + + LongHeader longHeader( + LongHeader::Types::Initial, + getTestConnectionId(1), + connId, + getNextPacketNum(conn, PacketNumberSpace::Initial), + QuicVersion::MVFST); + increaseNextPacketNum(conn, PacketNumberSpace::Initial); + + RegularQuicPacketBuilder builder( + conn.udpSendPacketLen, + std::move(longHeader), + conn.ackStates.initialAckState.largestAckedByPeer.value_or(0)); + + FrameScheduler immediateAckOnlyScheduler = + std::move( + FrameScheduler::Builder( + conn, + EncryptionLevel::Initial, + LongHeader::typeToPacketNumberSpace(LongHeader::Types::Initial), + "ImmediateAckOnlyScheduler") + .immediateAckFrames()) + .build(); + + auto result = immediateAckOnlyScheduler.scheduleFramesForPacket( + std::move(builder), conn.udpSendPacketLen); + auto packetLength = result.packet->header->computeChainDataLength() + + result.packet->body->computeChainDataLength(); + EXPECT_EQ(conn.udpSendPacketLen, packetLength); +} + +TEST_F(QuicPacketSchedulerTest, ImmediateAckFrameSchedulerNotRequested) { + QuicClientConnectionState conn( + FizzClientQuicHandshakeContext::Builder().build()); + conn.pendingEvents.requestImmediateAck = false; + auto connId = getTestConnectionId(); + + LongHeader longHeader( + LongHeader::Types::Initial, + getTestConnectionId(1), + connId, + getNextPacketNum(conn, PacketNumberSpace::Initial), + QuicVersion::MVFST); + increaseNextPacketNum(conn, PacketNumberSpace::Initial); + + RegularQuicPacketBuilder builder( + conn.udpSendPacketLen, + std::move(longHeader), + conn.ackStates.initialAckState.largestAckedByPeer.value_or(0)); + + FrameScheduler immediateAckOnlyScheduler = + std::move( + FrameScheduler::Builder( + conn, + EncryptionLevel::Initial, + LongHeader::typeToPacketNumberSpace(LongHeader::Types::Initial), + "ImmediateAckOnlyScheduler") + .immediateAckFrames()) + .build(); + + auto result = immediateAckOnlyScheduler.scheduleFramesForPacket( + std::move(builder), conn.udpSendPacketLen); + auto packetLength = result.packet->header->computeChainDataLength() + + result.packet->body->computeChainDataLength(); + // The immediate ACK scheduler was not triggered. This packet has no frames + // and it shouldn't get padded. + EXPECT_LT(packetLength, conn.udpSendPacketLen); +} + INSTANTIATE_TEST_SUITE_P( QuicPacketSchedulerTests, QuicPacketSchedulerTest, diff --git a/quic/client/QuicClientTransport.cpp b/quic/client/QuicClientTransport.cpp index 5b1c002a8..7c6e57160 100644 --- a/quic/client/QuicClientTransport.cpp +++ b/quic/client/QuicClientTransport.cpp @@ -614,6 +614,21 @@ void QuicClientTransport::processPacketData( handleDatagram(*conn_, frame, receiveTimePoint); break; } + case QuicFrame::Type::ImmediateAckFrame: { + if (!conn_->transportSettings.minAckDelay.hasValue()) { + // We do not accept IMMEDIATE_ACK frames. This is a protocol + // violation. + throw QuicTransportException( + "Received IMMEDIATE_ACK frame without announcing min_ack_delay", + TransportErrorCode::PROTOCOL_VIOLATION, + FrameType::IMMEDIATE_ACK); + } + // Send an ACK from any packet number space. + conn_->ackStates.initialAckState.needsToSendAckImmediately = true; + conn_->ackStates.handshakeAckState.needsToSendAckImmediately = true; + conn_->ackStates.appDataAckState.needsToSendAckImmediately = true; + break; + } default: break; } diff --git a/quic/codec/Decode.cpp b/quic/codec/Decode.cpp index 8bf7b2950..cb36f6ad2 100644 --- a/quic/codec/Decode.cpp +++ b/quic/codec/Decode.cpp @@ -136,6 +136,10 @@ AckFrequencyFrame decodeAckFrequencyFrame(folly::io::Cursor& cursor) { return frame; } +ImmediateAckFrame decodeImmediateAckFrame(folly::io::Cursor&) { + return ImmediateAckFrame(); +} + ReadAckFrame decodeAckFrame( folly::io::Cursor& cursor, const PacketHeader& header, @@ -836,6 +840,8 @@ QuicFrame parseFrame( return QuicFrame(decodeKnobFrame(cursor)); case FrameType::ACK_FREQUENCY: return QuicFrame(decodeAckFrequencyFrame(cursor)); + case FrameType::IMMEDIATE_ACK: + return QuicFrame(decodeImmediateAckFrame(cursor)); } } catch (const std::exception& e) { error = true; diff --git a/quic/codec/Decode.h b/quic/codec/Decode.h index abede7493..670d25a17 100644 --- a/quic/codec/Decode.h +++ b/quic/codec/Decode.h @@ -93,6 +93,8 @@ KnobFrame decodeKnobFrame(folly::io::Cursor& cursor); AckFrequencyFrame decodeAckFrequencyFrame(folly::io::Cursor& cursor); +ImmediateAckFrame decodeImmediateAckFrame(folly::io::Cursor& cursor); + DataBlockedFrame decodeDataBlockedFrame(folly::io::Cursor& cursor); StreamDataBlockedFrame decodeStreamDataBlockedFrame(folly::io::Cursor& cursor); diff --git a/quic/codec/QuicWriteCodec.cpp b/quic/codec/QuicWriteCodec.cpp index 5c1376768..a09600bd6 100644 --- a/quic/codec/QuicWriteCodec.cpp +++ b/quic/codec/QuicWriteCodec.cpp @@ -726,6 +726,17 @@ size_t writeFrame(QuicWriteFrame&& frame, PacketBuilderInterface& builder) { // no space left in packet return size_t(0); } + case QuicWriteFrame::Type::ImmediateAckFrame: { + const ImmediateAckFrame& immediateAckFrame = *frame.asImmediateAckFrame(); + QuicInteger intFrameType(static_cast(FrameType::IMMEDIATE_ACK)); + if (packetSpaceCheck(spaceLeft, intFrameType.getSize())) { + builder.write(intFrameType); + builder.appendFrame(immediateAckFrame); + return intFrameType.getSize(); + } + // no space left in packet + return size_t(0); + } default: { // TODO add support for: RETIRE_CONNECTION_ID and NEW_TOKEN frames auto errorStr = folly::to( diff --git a/quic/codec/Types.cpp b/quic/codec/Types.cpp index b796d6756..96448b099 100644 --- a/quic/codec/Types.cpp +++ b/quic/codec/Types.cpp @@ -435,6 +435,8 @@ std::string toString(FrameType frame) { return "KNOB"; case FrameType::ACK_FREQUENCY: return "ACK_FREQUENCY"; + case FrameType::IMMEDIATE_ACK: + return "IMMEDIATE_ACK"; case FrameType::GROUP_STREAM: case FrameType::GROUP_STREAM_FIN: case FrameType::GROUP_STREAM_LEN: diff --git a/quic/codec/Types.h b/quic/codec/Types.h index 7628460be..d1110c647 100644 --- a/quic/codec/Types.h +++ b/quic/codec/Types.h @@ -125,6 +125,14 @@ struct AckFrequencyFrame { } }; +struct ImmediateAckFrame { + ImmediateAckFrame() = default; + + bool operator==(const ImmediateAckFrame& /*rhs*/) const { + return true; + } +}; + /** * AckBlock represents a series of continuous packet sequences from * [startPacket, endPacket] @@ -760,7 +768,8 @@ DECLARE_VARIANT_TYPE(QuicSimpleFrame, QUIC_SIMPLE_FRAME) F(QuicSimpleFrame, __VA_ARGS__) \ F(PingFrame, __VA_ARGS__) \ F(NoopFrame, __VA_ARGS__) \ - F(DatagramFrame, __VA_ARGS__) + F(DatagramFrame, __VA_ARGS__) \ + F(ImmediateAckFrame, __VA_ARGS__) DECLARE_VARIANT_TYPE(QuicFrame, QUIC_FRAME) @@ -779,7 +788,8 @@ DECLARE_VARIANT_TYPE(QuicFrame, QUIC_FRAME) F(QuicSimpleFrame, __VA_ARGS__) \ F(PingFrame, __VA_ARGS__) \ F(NoopFrame, __VA_ARGS__) \ - F(DatagramFrame, __VA_ARGS__) + F(DatagramFrame, __VA_ARGS__) \ + F(ImmediateAckFrame, __VA_ARGS__) // Types of frames which are written. DECLARE_VARIANT_TYPE(QuicWriteFrame, QUIC_WRITE_FRAME) diff --git a/quic/logging/BaseQLogger.cpp b/quic/logging/BaseQLogger.cpp index 2495dfad8..850006e42 100644 --- a/quic/logging/BaseQLogger.cpp +++ b/quic/logging/BaseQLogger.cpp @@ -193,6 +193,10 @@ std::unique_ptr BaseQLogger::createPacketEvent( std::make_unique(frame.length)); break; } + case QuicFrame::Type::ImmediateAckFrame: { + event->frames.push_back(std::make_unique()); + break; + } } } if (numPaddingFrames > 0) { @@ -299,7 +303,12 @@ std::unique_ptr BaseQLogger::createPacketEvent( // TODO break; } - default: { + case QuicWriteFrame::Type::ImmediateAckFrame: { + event->frames.push_back(std::make_unique()); + break; + } + case QuicWriteFrame::Type::PingFrame: { + event->frames.push_back(std::make_unique()); break; } } diff --git a/quic/logging/QLoggerConstants.cpp b/quic/logging/QLoggerConstants.cpp index f518afb91..28408077f 100644 --- a/quic/logging/QLoggerConstants.cpp +++ b/quic/logging/QLoggerConstants.cpp @@ -79,6 +79,8 @@ folly::StringPiece toQlogString(FrameType frame) { return "knob"; case FrameType::ACK_FREQUENCY: return "ack_frequency"; + case FrameType::IMMEDIATE_ACK: + return "immediate_ack"; case FrameType::GROUP_STREAM: case FrameType::GROUP_STREAM_FIN: case FrameType::GROUP_STREAM_LEN: diff --git a/quic/logging/QLoggerTypes.cpp b/quic/logging/QLoggerTypes.cpp index e7c9f7af3..cd593be78 100644 --- a/quic/logging/QLoggerTypes.cpp +++ b/quic/logging/QLoggerTypes.cpp @@ -127,6 +127,12 @@ folly::dynamic AckFrequencyFrameLog::toDynamic() const { return d; } +folly::dynamic ImmediateAckFrameLog::toDynamic() const { + folly::dynamic d = folly::dynamic::object(); + d["frame_type"] = toQlogString(FrameType::IMMEDIATE_ACK); + return d; +} + folly::dynamic StreamDataBlockedFrameLog::toDynamic() const { folly::dynamic d = folly::dynamic::object(); d["frame_type"] = toQlogString(FrameType::STREAM_DATA_BLOCKED); diff --git a/quic/logging/QLoggerTypes.h b/quic/logging/QLoggerTypes.h index ae73c96cb..90480ab2e 100644 --- a/quic/logging/QLoggerTypes.h +++ b/quic/logging/QLoggerTypes.h @@ -172,6 +172,13 @@ class AckFrequencyFrameLog : public QLogFrame { FOLLY_NODISCARD folly::dynamic toDynamic() const override; }; +class ImmediateAckFrameLog : public QLogFrame { + public: + ImmediateAckFrameLog() = default; + ~ImmediateAckFrameLog() override = default; + FOLLY_NODISCARD folly::dynamic toDynamic() const override; +}; + class StreamDataBlockedFrameLog : public QLogFrame { public: StreamId streamId; diff --git a/quic/server/state/ServerStateMachine.cpp b/quic/server/state/ServerStateMachine.cpp index d496fad6d..6be9dab42 100644 --- a/quic/server/state/ServerStateMachine.cpp +++ b/quic/server/state/ServerStateMachine.cpp @@ -121,6 +121,7 @@ bool isUnprotectedPacketFrameInvalid(const QuicFrame& quicFrame) { case QuicFrame::Type::ReadNewTokenFrame: case QuicFrame::Type::DatagramFrame: case QuicFrame::Type::NoopFrame: + case QuicFrame::Type::ImmediateAckFrame: case QuicFrame::Type::QuicSimpleFrame: return true; } @@ -148,6 +149,7 @@ bool isZeroRttPacketFrameInvalid(const QuicFrame& quicFrame) { case QuicFrame::Type::ReadAckFrame: case QuicFrame::Type::ReadCryptoFrame: case QuicFrame::Type::ReadNewTokenFrame: + case QuicFrame::Type::ImmediateAckFrame: return true; case QuicFrame::Type::PingFrame: case QuicFrame::Type::ConnectionCloseFrame: @@ -1264,6 +1266,21 @@ void onServerReadDataFromOpen( handleDatagram(conn, frame, readData.networkData.receiveTimePoint); break; } + case QuicFrame::Type::ImmediateAckFrame: { + if (!conn.transportSettings.minAckDelay.hasValue()) { + // We do not accept IMMEDIATE_ACK frames. This is a protocol + // violation. + throw QuicTransportException( + "Received IMMEDIATE_ACK frame without announcing min_ack_delay", + TransportErrorCode::PROTOCOL_VIOLATION, + FrameType::IMMEDIATE_ACK); + } + // Send an ACK from any packet number space. + conn.ackStates.initialAckState.needsToSendAckImmediately = true; + conn.ackStates.handshakeAckState.needsToSendAckImmediately = true; + conn.ackStates.appDataAckState.needsToSendAckImmediately = true; + break; + } default: { break; } diff --git a/quic/server/test/QuicServerTransportTest.cpp b/quic/server/test/QuicServerTransportTest.cpp index 854b7e7d3..25e28cb9b 100644 --- a/quic/server/test/QuicServerTransportTest.cpp +++ b/quic/server/test/QuicServerTransportTest.cpp @@ -2858,6 +2858,65 @@ TEST_F(QuicServerTransportTest, PingIsTreatedAsRetransmittable) { EXPECT_TRUE(server->getConn().pendingEvents.scheduleAckTimeout); } +TEST_F(QuicServerTransportTest, ImmediateAckValid) { + // Verify that an incoming IMMEDIATE_ACK frame flags all + // packet number spaces to generate ACKs immediately. + ImmediateAckFrame immediateAckFrame; + // We support receiving IMMEDIATE_ACK + server->getNonConstConn().transportSettings.minAckDelay = 1ms; + + auto packetNum = clientNextAppDataPacketNum++; + ShortHeader header( + ProtectionType::KeyPhaseZero, + *server->getConn().serverConnectionId, + packetNum); + RegularQuicPacketBuilder builder( + server->getConn().udpSendPacketLen, + std::move(header), + 0 /* largestAcked */); + builder.encodePacketHeader(); + writeFrame(immediateAckFrame, builder); + auto packet = std::move(builder).buildPacket(); + ASSERT_NO_THROW(deliverData(packetToBuf(packet))); + // An ACK has been scheduled for AppData number space. + EXPECT_TRUE(server->getConn() + .ackStates.appDataAckState.largestAckScheduled.hasValue()); + EXPECT_EQ( + server->getConn().ackStates.appDataAckState.largestAckScheduled.value_or( + packetNum + 1), + packetNum); +} + +TEST_F(QuicServerTransportTest, ImmediateAckProtocolViolation) { + // Verify that an incoming IMMEDIATE_ACK frame flags all + // packet number spaces to generate ACKs immediately. + ImmediateAckFrame immediateAckFrame; + // We do not support IMMEDIATE_ACK frames + server->getNonConstConn().transportSettings.minAckDelay.clear(); + + auto packetNum = clientNextAppDataPacketNum++; + ShortHeader header( + ProtectionType::KeyPhaseZero, + *server->getConn().serverConnectionId, + packetNum); + RegularQuicPacketBuilder builder( + server->getConn().udpSendPacketLen, + std::move(header), + 0 /* largestAcked */); + builder.encodePacketHeader(); + writeFrame(immediateAckFrame, builder); + auto packet = std::move(builder).buildPacket(); + // This should throw a protocol violation error + ASSERT_THROW(deliverData(packetToBuf(packet)), std::runtime_error); + // Verify that none of the ack states have changed + EXPECT_FALSE( + server->getConn().ackStates.initialAckState.needsToSendAckImmediately); + EXPECT_FALSE( + server->getConn().ackStates.handshakeAckState.needsToSendAckImmediately); + EXPECT_FALSE( + server->getConn().ackStates.appDataAckState.needsToSendAckImmediately); +} + TEST_F(QuicServerTransportTest, ReceiveDatagramFrameAndDiscard) { ShortHeader header( ProtectionType::KeyPhaseZero, diff --git a/quic/state/AckStates.h b/quic/state/AckStates.h index 7fe86fdd7..54ff988b4 100644 --- a/quic/state/AckStates.h +++ b/quic/state/AckStates.h @@ -36,8 +36,10 @@ struct AckState { folly::Optional tolerance; folly::Optional ackFrequencySequenceNumber; // Flag indicating that if we need to send ack immediately. This will be set - // to true if we got packets with retransmittable data and haven't sent the + // to true in either of the following cases: + // - we got packets with retransmittable data and haven't sent the // ack for the first time. + // - the peer has requested it through an immediate ack frame. bool needsToSendAckImmediately{false}; // Count of oustanding packets received with retransmittable data. uint8_t numRxPacketsRecvd{0}; diff --git a/quic/state/SimpleFrameFunctions.cpp b/quic/state/SimpleFrameFunctions.cpp index edf02148e..f25c2dfe0 100644 --- a/quic/state/SimpleFrameFunctions.cpp +++ b/quic/state/SimpleFrameFunctions.cpp @@ -250,7 +250,12 @@ bool updateSimpleFrameOnPacketReceived( } case QuicSimpleFrame::Type::AckFrequencyFrame: { if (!conn.transportSettings.minAckDelay.hasValue()) { - return true; + // We do not accept ACK_FREQUENCY frames. This is a protocol + // violation. + throw QuicTransportException( + "Received ACK_FREQUENCY frame without announcing min_ack_delay", + TransportErrorCode::PROTOCOL_VIOLATION, + FrameType::ACK_FREQUENCY); } const auto ackFrequencyFrame = frame.asAckFrequencyFrame(); auto& ackState = conn.ackStates.appDataAckState; diff --git a/quic/state/StateData.h b/quic/state/StateData.h index 86aa172c2..71330c0d0 100644 --- a/quic/state/StateData.h +++ b/quic/state/StateData.h @@ -495,6 +495,9 @@ struct QuicConnectionStateBase : public folly::DelayedDestruction { // Do we need to send data blocked frame when connection is blocked. bool sendDataBlocked{false}; + + // Send an immediate ack frame (requesting an ack) + bool requestImmediateAck{false}; }; PendingEvents pendingEvents; @@ -549,7 +552,8 @@ struct QuicConnectionStateBase : public folly::DelayedDestruction { // Value of the negotiated ack delay exponent. uint64_t peerAckDelayExponent{kDefaultAckDelayExponent}; - // The value of the peer's min_ack_delay, for creating ACK_FREQUENCY frames. + // The value of the peer's min_ack_delay, for creating ACK_FREQUENCY and + // IMMEDIATE_ACK frames. folly::Optional peerMinAckDelay; // Idle timeout advertised by the peer. Initially sets it to the maximum value diff --git a/quic/state/TransportSettings.h b/quic/state/TransportSettings.h index fdc211e49..fd02168af 100644 --- a/quic/state/TransportSettings.h +++ b/quic/state/TransportSettings.h @@ -216,6 +216,9 @@ struct TransportSettings { kDefaultRxPacketsBeforeAckInitThreshold}; uint16_t rxPacketsBeforeAckBeforeInit{kDefaultRxPacketsBeforeAckBeforeInit}; uint16_t rxPacketsBeforeAckAfterInit{kDefaultRxPacketsBeforeAckAfterInit}; + // The minimum amount of time in microseconds by which an ack can be delayed + // Setting a value here also indicates to the peer that it can send + // ACK_FREQUENCY and IMMEDIATE_ACK frames folly::Optional minAckDelay; // Limits the amount of data that should be buffered in a QuicSocket. // If the amount of data in the buffer equals or exceeds this amount, then