From d6a876f2016885da3aae833eabd7190746d1f1a4 Mon Sep 17 00:00:00 2001 From: Matt Joras Date: Tue, 7 Dec 2021 18:16:42 -0800 Subject: [PATCH] If there's lost data, we have data to write. Summary: This is an edge case we weren't handling properly. This can lead to situations where we run out of conn flow control, the peer hasn't received all data, and then we never write the data to trigger a flow control update. Reviewed By: afrind Differential Revision: D32887140 fbshipit-source-id: df5dfad8e0775ef43e5ca6ab98b8ca6b5987ce31 --- quic/api/QuicPacketScheduler.cpp | 5 +++-- quic/api/QuicTransportFunctions.cpp | 6 ++++-- quic/api/test/QuicPacketSchedulerTest.cpp | 3 +++ quic/api/test/QuicTransportFunctionsTest.cpp | 18 ++++++++++++++++++ quic/api/test/QuicTransportTest.cpp | 12 +++++++++++- 5 files changed, 39 insertions(+), 5 deletions(-) diff --git a/quic/api/QuicPacketScheduler.cpp b/quic/api/QuicPacketScheduler.cpp index d5508f5b7..b8a26bea7 100644 --- a/quic/api/QuicPacketScheduler.cpp +++ b/quic/api/QuicPacketScheduler.cpp @@ -473,8 +473,9 @@ void StreamFrameScheduler::writeStreams(PacketBuilderInterface& builder) { } // namespace quic bool StreamFrameScheduler::hasPendingData() const { - return conn_.streamManager->hasNonDSRWritable() && - getSendConnFlowControlBytesWire(conn_) > 0; + return conn_.streamManager->hasLoss() || + (conn_.streamManager->hasNonDSRWritable() && + getSendConnFlowControlBytesWire(conn_) > 0); } bool StreamFrameScheduler::writeStreamFrame( diff --git a/quic/api/QuicTransportFunctions.cpp b/quic/api/QuicTransportFunctions.cpp index 6349b23be..3f3f1f897 100644 --- a/quic/api/QuicTransportFunctions.cpp +++ b/quic/api/QuicTransportFunctions.cpp @@ -1601,8 +1601,10 @@ WriteDataReason hasNonAckDataToWrite(const QuicConnectionStateBase& conn) { if (conn.streamManager->hasBlocked()) { return WriteDataReason::BLOCKED; } - if (getSendConnFlowControlBytesWire(conn) != 0 && - conn.streamManager->hasWritable()) { + // If we have lost data or flow control + stream data. + if (conn.streamManager->hasLoss() || + (getSendConnFlowControlBytesWire(conn) != 0 && + conn.streamManager->hasWritable())) { return WriteDataReason::STREAM; } if (!conn.pendingEvents.frames.empty()) { diff --git a/quic/api/test/QuicPacketSchedulerTest.cpp b/quic/api/test/QuicPacketSchedulerTest.cpp index a97c376f2..1f2be7cce 100644 --- a/quic/api/test/QuicPacketSchedulerTest.cpp +++ b/quic/api/test/QuicPacketSchedulerTest.cpp @@ -1641,6 +1641,7 @@ TEST_F(QuicPacketSchedulerTest, WriteLossWithoutFlowControl) { conn.streamManager->updateWritableStreams(*stream); StreamFrameScheduler scheduler(conn); + EXPECT_TRUE(scheduler.hasPendingData()); ShortHeader shortHeader1( ProtectionType::KeyPhaseZero, getTestConnectionId(), @@ -1666,6 +1667,8 @@ TEST_F(QuicPacketSchedulerTest, WriteLossWithoutFlowControl) { stream->lossBuffer.emplace_back(std::move(*stream->retransmissionBuffer[0])); stream->retransmissionBuffer.clear(); conn.streamManager->updateWritableStreams(*stream); + conn.streamManager->updateLossStreams(*stream); + EXPECT_TRUE(scheduler.hasPendingData()); // Write again ShortHeader shortHeader2( diff --git a/quic/api/test/QuicTransportFunctionsTest.cpp b/quic/api/test/QuicTransportFunctionsTest.cpp index 6aa44c5af..e8cb6606b 100644 --- a/quic/api/test/QuicTransportFunctionsTest.cpp +++ b/quic/api/test/QuicTransportFunctionsTest.cpp @@ -2804,6 +2804,24 @@ TEST_F(QuicTransportFunctionsTest, ShouldWriteDataNoConnFlowControl) { EXPECT_EQ(WriteDataReason::NO_WRITE, shouldWriteData(*conn)); } +TEST_F(QuicTransportFunctionsTest, ShouldWriteDataNoConnFlowControlLoss) { + auto conn = createConn(); + conn->oneRttWriteCipher = test::createNoOpAead(); + auto mockCongestionController = + std::make_unique>(); + auto rawCongestionController = mockCongestionController.get(); + EXPECT_CALL(*rawCongestionController, getWritableBytes()) + .WillRepeatedly(Return(1500)); + auto stream1 = conn->streamManager->createNextBidirectionalStream().value(); + auto buf = IOBuf::copyBuffer("0123456789"); + writeDataToQuicStream(*stream1, buf->clone(), false); + EXPECT_NE(WriteDataReason::NO_WRITE, shouldWriteData(*conn)); + // Artificially limit the connection flow control. + conn->streamManager->addLoss(stream1->id); + conn->flowControlState.peerAdvertisedMaxOffset = 0; + EXPECT_NE(WriteDataReason::NO_WRITE, shouldWriteData(*conn)); +} + TEST_F(QuicTransportFunctionsTest, HasAckDataToWriteCipherAndAckStateMatch) { auto conn = createConn(); EXPECT_FALSE(hasAckDataToWrite(*conn)); diff --git a/quic/api/test/QuicTransportTest.cpp b/quic/api/test/QuicTransportTest.cpp index bc6861130..e457b1eb0 100644 --- a/quic/api/test/QuicTransportTest.cpp +++ b/quic/api/test/QuicTransportTest.cpp @@ -270,7 +270,17 @@ TEST_F(QuicTransportTest, NotAppLimitedWithLoss) { auto stream = transport_->createBidirectionalStream().value(); auto lossStream = transport_->createBidirectionalStream().value(); - conn.streamManager->addLoss(lossStream); + auto lossStreamState = conn.streamManager->findStream(lossStream); + ASSERT_TRUE(lossStreamState); + auto largeBuf = folly::IOBuf::createChain(conn.udpSendPacketLen * 20, 4096); + auto curBuf = largeBuf.get(); + do { + curBuf->append(curBuf->capacity()); + curBuf = curBuf->next(); + } while (curBuf != largeBuf.get()); + lossStreamState->lossBuffer.emplace_back(std::move(largeBuf), 31, false); + conn.streamManager->updateWritableStreams(*lossStreamState); + conn.streamManager->updateLossStreams(*lossStreamState); transport_->writeChain( stream, IOBuf::copyBuffer("An elephant sitting still"), false, nullptr); EXPECT_CALL(*rawCongestionController, setAppLimited()).Times(0);