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);