From 4acc7dc355b66f8c07c8f773b8bc345aafcaade0 Mon Sep 17 00:00:00 2001 From: Luca Niccolini Date: Sun, 13 Jun 2021 21:12:07 -0700 Subject: [PATCH] Implement one Datagram Frame per packet, and discard policy on rx/tx paths Reviewed By: mjoras Differential Revision: D28167755 fbshipit-source-id: b589376800742dfe167f1efe193f0fe059b18ab4 --- quic/api/QuicPacketScheduler.cpp | 3 ++ quic/api/QuicTransportBase.cpp | 22 +++++--- quic/api/test/QuicPacketSchedulerTest.cpp | 48 +++++++++++++++++ .../client/test/QuicClientTransportTest.cpp | 52 +++++++++++++++++++ quic/state/DatagramHandlers.cpp | 19 ++++--- quic/state/TransportSettings.h | 3 ++ 6 files changed, 133 insertions(+), 14 deletions(-) diff --git a/quic/api/QuicPacketScheduler.cpp b/quic/api/QuicPacketScheduler.cpp index 4bc95747d..0ff367355 100644 --- a/quic/api/QuicPacketScheduler.cpp +++ b/quic/api/QuicPacketScheduler.cpp @@ -640,6 +640,9 @@ bool DatagramFrameScheduler::writeDatagramFrames( payload = datagramFrame.data.move(); break; } + if (conn_.transportSettings.datagramConfig.framePerPacket) { + break; + } } return sent; } diff --git a/quic/api/QuicTransportBase.cpp b/quic/api/QuicTransportBase.cpp index e179ddb5e..2928f9404 100644 --- a/quic/api/QuicTransportBase.cpp +++ b/quic/api/QuicTransportBase.cpp @@ -2712,16 +2712,22 @@ uint16_t QuicTransportBase::getDatagramSizeLimit() const { folly::Expected QuicTransportBase::writeDatagram( Buf buf) { - if (conn_->datagramState.writeBuffer.size() >= - conn_->datagramState.maxWriteBufferSize || - // TODO(lniccolini) update max datagram frame size - // https://github.com/quicwg/datagram/issues/3 - // For now, max_datagram_size > 0 means the peer supports datagram frames - conn_->datagramState.maxWriteFrameSize == 0) { - // TODO(lniccolini) use different return codes to signal the application - // exactly why the datagram got dropped + // TODO(lniccolini) update max datagram frame size + // https://github.com/quicwg/datagram/issues/3 + // For now, max_datagram_size > 0 means the peer supports datagram frames + if (conn_->datagramState.maxWriteFrameSize == 0) { return folly::makeUnexpected(LocalErrorCode::INVALID_WRITE_DATA); } + if (conn_->datagramState.writeBuffer.size() >= + conn_->datagramState.maxWriteBufferSize) { + if (!conn_->transportSettings.datagramConfig.sendDropOldDataFirst) { + // TODO(lniccolini) use different return codes to signal the application + // exactly why the datagram got dropped + return folly::makeUnexpected(LocalErrorCode::INVALID_WRITE_DATA); + } else { + conn_->datagramState.writeBuffer.pop_front(); + } + } conn_->datagramState.writeBuffer.emplace_back(std::move(buf)); return folly::unit; } diff --git a/quic/api/test/QuicPacketSchedulerTest.cpp b/quic/api/test/QuicPacketSchedulerTest.cpp index 09ac9fbd1..b6c057b44 100644 --- a/quic/api/test/QuicPacketSchedulerTest.cpp +++ b/quic/api/test/QuicPacketSchedulerTest.cpp @@ -1842,6 +1842,54 @@ TEST_F(QuicPacketSchedulerTest, NoFINWriteWhenBufMetaWrittenFIN) { EXPECT_FALSE(scheduler2.hasPendingData()); } +TEST_F(QuicPacketSchedulerTest, DatagramFrameSchedulerMultipleFramesPerPacket) { + QuicClientConnectionState conn( + FizzClientQuicHandshakeContext::Builder().build()); + conn.datagramState.maxReadFrameSize = std::numeric_limits::max(); + conn.datagramState.maxReadBufferSize = 10; + conn.transportSettings.datagramConfig.framePerPacket = false; + DatagramFrameScheduler scheduler(conn); + // Add datagrams + conn.datagramState.writeBuffer.emplace_back( + folly::IOBuf::createChain(conn.udpSendPacketLen / 3, 4096)); + conn.datagramState.writeBuffer.emplace_back( + folly::IOBuf::createChain(conn.udpSendPacketLen / 3, 4096)); + NiceMock builder2; + EXPECT_CALL(builder2, remainingSpaceInPkt()).WillRepeatedly(Return(4096)); + EXPECT_CALL(builder2, appendFrame(_)).WillRepeatedly(Invoke([&](auto f) { + builder2.frames_.push_back(f); + })); + // Call scheduler + auto& frames = builder2.frames_; + scheduler.writeDatagramFrames(builder2); + ASSERT_EQ(frames.size(), 2); +} + +TEST_F(QuicPacketSchedulerTest, DatagramFrameSchedulerOneFramePerPacket) { + QuicClientConnectionState conn( + FizzClientQuicHandshakeContext::Builder().build()); + conn.datagramState.maxReadFrameSize = std::numeric_limits::max(); + conn.datagramState.maxReadBufferSize = 10; + conn.transportSettings.datagramConfig.framePerPacket = true; + DatagramFrameScheduler scheduler(conn); + // Add datagrams + conn.datagramState.writeBuffer.emplace_back( + folly::IOBuf::createChain(conn.udpSendPacketLen / 3, 4096)); + conn.datagramState.writeBuffer.emplace_back( + folly::IOBuf::createChain(conn.udpSendPacketLen / 3, 4096)); + NiceMock builder2; + EXPECT_CALL(builder2, remainingSpaceInPkt()).WillRepeatedly(Return(4096)); + EXPECT_CALL(builder2, appendFrame(_)).WillRepeatedly(Invoke([&](auto f) { + builder2.frames_.push_back(f); + })); + // Call scheduler + auto& frames = builder2.frames_; + scheduler.writeDatagramFrames(builder2); + ASSERT_EQ(frames.size(), 1); + scheduler.writeDatagramFrames(builder2); + ASSERT_EQ(frames.size(), 2); +} + INSTANTIATE_TEST_CASE_P( QuicPacketSchedulerTests, QuicPacketSchedulerTest, diff --git a/quic/fizz/client/test/QuicClientTransportTest.cpp b/quic/fizz/client/test/QuicClientTransportTest.cpp index 1576aae4d..673914206 100644 --- a/quic/fizz/client/test/QuicClientTransportTest.cpp +++ b/quic/fizz/client/test/QuicClientTransportTest.cpp @@ -5374,6 +5374,58 @@ TEST_F(QuicClientTransportAfterStartTest, ReceiveDatagramFrameAndStore) { conn.datagramState.maxReadBufferSize); } +TEST_F( + QuicClientTransportAfterStartTest, + ReceiveDatagramFrameAndDiscardOldDataFirst) { + auto& conn = client->getNonConstConn(); + conn.datagramState.maxReadFrameSize = std::numeric_limits::max(); + conn.datagramState.maxReadBufferSize = 1; + auto& transportSettings = client->getNonConstConn().transportSettings; + transportSettings.datagramConfig.recvDropOldDataFirst = true; + + // Enqueue first datagram + ShortHeader header1( + ProtectionType::KeyPhaseZero, *originalConnId, appDataPacketNum++); + RegularQuicPacketBuilder builder1( + client->getConn().udpSendPacketLen, + std::move(header1), + 0 /* largestAcked */); + builder1.encodePacketHeader(); + StringPiece datagramPayload1 = "first"; + DatagramFrame datagramFrame1( + datagramPayload1.size(), IOBuf::copyBuffer(datagramPayload1)); + writeFrame(datagramFrame1, builder1); + auto packet1 = packetToBuf(std::move(builder1).buildPacket()); + deliverData(packet1->coalesce()); + ASSERT_EQ( + client->getConn().datagramState.readBuffer.size(), + conn.datagramState.maxReadBufferSize); + // Enqueue second datagram + ShortHeader header2( + ProtectionType::KeyPhaseZero, *originalConnId, appDataPacketNum++); + RegularQuicPacketBuilder builder2( + client->getConn().udpSendPacketLen, + std::move(header2), + 0 /* largestAcked */); + builder2.encodePacketHeader(); + StringPiece datagramPayload2 = "second"; + DatagramFrame datagramFrame2( + datagramPayload2.size(), IOBuf::copyBuffer(datagramPayload2)); + writeFrame(datagramFrame2, builder2); + auto packet2 = packetToBuf(std::move(builder2).buildPacket()); + deliverData(packet2->coalesce()); + ASSERT_EQ( + client->getConn().datagramState.readBuffer.size(), + conn.datagramState.maxReadBufferSize); + + auto payload = client->getConn() + .datagramState.readBuffer[0] + .front() + ->clone() + ->moveToFbString(); + ASSERT_EQ(payload, "second"); +} + TEST_F(QuicClientTransportAfterStartTest, OneCloseFramePerRtt) { auto streamId = client->createBidirectionalStream().value(); auto& conn = client->getNonConstConn(); diff --git a/quic/state/DatagramHandlers.cpp b/quic/state/DatagramHandlers.cpp index 250686f7f..f9289ea84 100644 --- a/quic/state/DatagramHandlers.cpp +++ b/quic/state/DatagramHandlers.cpp @@ -11,15 +11,22 @@ namespace quic { void handleDatagram(QuicConnectionStateBase& conn, DatagramFrame& frame) { - if (conn.datagramState.readBuffer.size() >= - conn.datagramState.maxReadBufferSize || - // TODO(lniccolini) update max datagram frame size - // https://github.com/quicwg/datagram/issues/3 - // For now, max_datagram_size > 0 means the peer supports datagram frames - conn.datagramState.maxReadFrameSize == 0) { + // TODO(lniccolini) update max datagram frame size + // https://github.com/quicwg/datagram/issues/3 + // For now, max_datagram_size > 0 means the peer supports datagram frames + if (conn.datagramState.maxReadFrameSize == 0) { frame.data.move(); return; } + if (conn.datagramState.readBuffer.size() >= + conn.datagramState.maxReadBufferSize) { + if (!conn.transportSettings.datagramConfig.recvDropOldDataFirst) { + frame.data.move(); + return; + } else { + conn.datagramState.readBuffer.pop_front(); + } + } conn.datagramState.readBuffer.emplace_back(std::move(frame.data)); } diff --git a/quic/state/TransportSettings.h b/quic/state/TransportSettings.h index fa4019413..cbc50e44f 100644 --- a/quic/state/TransportSettings.h +++ b/quic/state/TransportSettings.h @@ -112,6 +112,9 @@ struct D6DConfig { struct DatagramConfig { bool enabled{false}; + bool framePerPacket{true}; + bool recvDropOldDataFirst{false}; + bool sendDropOldDataFirst{false}; }; // JSON-serialized transport knobs