From 0e6a998e059a2d0ab90b6711a34dae7faa1e3a0d Mon Sep 17 00:00:00 2001 From: Yang Chi Date: Tue, 1 Jun 2021 13:15:40 -0700 Subject: [PATCH] Fix QUIC EOF sending when DSR is used Summary: This is a bug fix. When a stream finished sending all it's BufMetas, it can send EOF from frontend QUIC host. In that case, it will use currentWriteOffset value which is wrong. This diff changes it so that (1) frontend can only write FIN only frame if writeBufMeta's offset is <= finalWriteOffset; (2) When it writes such frame it will use finalWriteOffset as the offset value in the frame. Reviewed By: lnicco Differential Revision: D28727029 fbshipit-source-id: 8f963c2e2d66f921f8a9704ed4671ec4f7c04df7 --- quic/api/QuicPacketScheduler.cpp | 8 ++- quic/api/QuicTransportFunctions.cpp | 1 + quic/api/test/QuicPacketSchedulerTest.cpp | 70 ++++++++++++++++++++++- quic/state/StreamData.h | 3 +- 4 files changed, 79 insertions(+), 3 deletions(-) diff --git a/quic/api/QuicPacketScheduler.cpp b/quic/api/QuicPacketScheduler.cpp index 5480aedfe..4bc95747d 100644 --- a/quic/api/QuicPacketScheduler.cpp +++ b/quic/api/QuicPacketScheduler.cpp @@ -499,10 +499,16 @@ bool StreamFrameScheduler::writeStreamFrame( // send. bool canWriteFin = stream.finalWriteOffset.has_value() && bufferLen <= flowControlLen && stream.writeBufMeta.length == 0; + auto writeOffset = stream.currentWriteOffset; + if (canWriteFin && stream.writeBuffer.empty()) { + // If we are writing FIN only from here, do not use currentWriteOffset in + // case some bufMeta has been sent before. + writeOffset = stream.finalWriteOffset.value(); + } auto dataLen = writeStreamFrameHeader( builder, stream.id, - stream.currentWriteOffset, + writeOffset, bufferLen, flowControlLen, canWriteFin, diff --git a/quic/api/QuicTransportFunctions.cpp b/quic/api/QuicTransportFunctions.cpp index 241fb1eba..d1cfbbc4b 100644 --- a/quic/api/QuicTransportFunctions.cpp +++ b/quic/api/QuicTransportFunctions.cpp @@ -425,6 +425,7 @@ void handleNewStreamBufMetaWritten( // If FIN is written, nothing should be left in the writeBufMeta. CHECK_EQ(0, stream.writeBufMeta.length); ++stream.writeBufMeta.offset; + CHECK_GT(stream.writeBufMeta.offset, *stream.finalWriteOffset); } CHECK(stream.retransmissionBufMetas .emplace( diff --git a/quic/api/test/QuicPacketSchedulerTest.cpp b/quic/api/test/QuicPacketSchedulerTest.cpp index 5f9cf7a71..09ac9fbd1 100644 --- a/quic/api/test/QuicPacketSchedulerTest.cpp +++ b/quic/api/test/QuicPacketSchedulerTest.cpp @@ -1741,7 +1741,7 @@ TEST_F(QuicPacketSchedulerTest, RunOutFlowControlDuringStreamWrite) { EXPECT_EQ(200, stream2->retransmissionBuffer[0]->data.chainLength()); } -TEST_F(QuicPacketSchedulerTest, NoFinWhenThereIsPendingWriteBuf) { +TEST_F(QuicPacketSchedulerTest, WritingFINWithAndWithoutBufMetas) { QuicServerConnectionState conn( FizzServerQuicHandshakeContext::Builder().build()); conn.streamManager->setMaxLocalBidirectionalStreams(10); @@ -1772,6 +1772,74 @@ TEST_F(QuicPacketSchedulerTest, NoFinWhenThereIsPendingWriteBuf) { EXPECT_EQ(streamFrame.len, 6); EXPECT_EQ(streamFrame.offset, 0); EXPECT_FALSE(streamFrame.fin); + handleNewStreamDataWritten(*stream, streamFrame.len, streamFrame.fin); + + // Pretent all the bufMetas were sent, without FIN bit + stream->writeBufMeta.split(5000); + ASSERT_EQ(0, stream->writeBufMeta.length); + ASSERT_GT(stream->writeBufMeta.offset, 0); + conn.streamManager->updateWritableStreams(*stream); + // Do another non-DSR stream write, it will write FIN with correct offset + ShortHeader header2( + ProtectionType::KeyPhaseOne, + conn.clientConnectionId.value_or(getTestConnectionId()), + 1); + RegularQuicPacketBuilder builder2( + conn.udpSendPacketLen, + std::move(header2), + conn.ackStates.appDataAckState.largestAckedByPeer.value_or(0)); + builder2.encodePacketHeader(); + StreamFrameScheduler scheduler2(conn); + scheduler2.writeStreams(builder2); + auto packet2 = std::move(builder2).buildPacket().packet; + EXPECT_EQ(1, packet2.frames.size()); + auto streamFrame2 = *packet2.frames[0].asWriteStreamFrame(); + EXPECT_EQ(streamFrame2.len, 0); + EXPECT_EQ(streamFrame2.offset, 6 + 5000); + EXPECT_TRUE(streamFrame2.fin); +} + +TEST_F(QuicPacketSchedulerTest, NoFINWriteWhenBufMetaWrittenFIN) { + QuicServerConnectionState conn( + FizzServerQuicHandshakeContext::Builder().build()); + conn.streamManager->setMaxLocalBidirectionalStreams(10); + conn.flowControlState.peerAdvertisedMaxOffset = 100000; + auto* stream = *(conn.streamManager->createNextBidirectionalStream()); + stream->flowControlState.peerAdvertisedMaxOffset = 100000; + + writeDataToQuicStream(*stream, folly::IOBuf::copyBuffer("Ascent"), false); + stream->dsrSender = std::make_unique(); + BufferMeta bufferMeta(5000); + writeBufMetaToQuicStream(*stream, bufferMeta, true); + EXPECT_TRUE(stream->finalWriteOffset.hasValue()); + PacketNum packetNum = 0; + ShortHeader header( + ProtectionType::KeyPhaseOne, + conn.clientConnectionId.value_or(getTestConnectionId()), + packetNum); + RegularQuicPacketBuilder builder( + conn.udpSendPacketLen, + std::move(header), + conn.ackStates.appDataAckState.largestAckedByPeer.value_or(0)); + builder.encodePacketHeader(); + StreamFrameScheduler scheduler(conn); + scheduler.writeStreams(builder); + auto packet = std::move(builder).buildPacket().packet; + EXPECT_EQ(1, packet.frames.size()); + auto streamFrame = *packet.frames[0].asWriteStreamFrame(); + EXPECT_EQ(streamFrame.len, 6); + EXPECT_EQ(streamFrame.offset, 0); + EXPECT_FALSE(streamFrame.fin); + handleNewStreamDataWritten(*stream, streamFrame.len, streamFrame.fin); + + // Pretent all the bufMetas were sent, without FIN bit + stream->writeBufMeta.split(5000); + stream->writeBufMeta.offset++; + ASSERT_EQ(0, stream->writeBufMeta.length); + ASSERT_GT(stream->writeBufMeta.offset, 0); + conn.streamManager->updateWritableStreams(*stream); + StreamFrameScheduler scheduler2(conn); + EXPECT_FALSE(scheduler2.hasPendingData()); } INSTANTIATE_TEST_CASE_P( diff --git a/quic/state/StreamData.h b/quic/state/StreamData.h index 18f9154c2..6615782bc 100644 --- a/quic/state/StreamData.h +++ b/quic/state/StreamData.h @@ -314,7 +314,8 @@ struct QuicStreamState : public QuicStreamLike { * still have BufMetas to send. */ return writeBufMeta.length == 0 && - currentWriteOffset <= *finalWriteOffset; + currentWriteOffset <= *finalWriteOffset && + writeBufMeta.offset <= *finalWriteOffset; } return false; }