diff --git a/quic/api/QuicTransportFunctions.cpp b/quic/api/QuicTransportFunctions.cpp index eb1c951bf..f65cebc87 100644 --- a/quic/api/QuicTransportFunctions.cpp +++ b/quic/api/QuicTransportFunctions.cpp @@ -98,16 +98,11 @@ void handleNewStreamDataWritten( stream.currentWriteOffset += frameLen; auto bufWritten = stream.writeBuffer.split(folly::to(frameLen)); stream.currentWriteOffset += frameFin ? 1 : 0; - auto insertIt = std::upper_bound( - stream.retransmissionBuffer.begin(), - stream.retransmissionBuffer.end(), - originalOffset, - [](const auto& offset, const auto& compare) { - // TODO: huh? why isn't this a >= ? - return compare.offset > offset; - }); - stream.retransmissionBuffer.emplace( - insertIt, std::move(bufWritten), originalOffset, frameFin); + CHECK( + stream.retransmissionBuffer.empty() || + stream.retransmissionBuffer.back().offset < originalOffset); + stream.retransmissionBuffer.emplace_back( + std::move(bufWritten), originalOffset, frameFin); } void handleRetransmissionWritten( diff --git a/quic/api/test/QuicTransportFunctionsTest.cpp b/quic/api/test/QuicTransportFunctionsTest.cpp index c2fbee6e6..28597bc96 100644 --- a/quic/api/test/QuicTransportFunctionsTest.cpp +++ b/quic/api/test/QuicTransportFunctionsTest.cpp @@ -1155,6 +1155,41 @@ TEST_F(QuicTransportFunctionsTest, WriteQuicDataToSocketWithNoBytesForHeader) { EXPECT_TRUE(conn->outstandingPackets.empty()); } +TEST_F(QuicTransportFunctionsTest, WriteQuicDataToSocketRetxBufferSorted) { + EventBase evb; + folly::test::MockAsyncUDPSocket socket(&evb); + auto conn = createConn(); + auto stream = conn->streamManager->createNextBidirectionalStream().value(); + auto buf1 = IOBuf::copyBuffer("Whatsapp"); + writeDataToQuicStream(*stream, std::move(buf1), false); + writeQuicDataToSocket( + socket, + *conn, + *conn->clientConnectionId, + *conn->serverConnectionId, + *aead, + *headerCipher, + getVersion(*conn), + conn->transportSettings.writeConnectionDataPacketsLimit); + EXPECT_EQ(1, stream->retransmissionBuffer.size()); + + auto buf2 = IOBuf::copyBuffer("Google Buzz"); + writeDataToQuicStream(*stream, std::move(buf2), false); + writeQuicDataToSocket( + socket, + *conn, + *conn->clientConnectionId, + *conn->serverConnectionId, + *aead, + *headerCipher, + getVersion(*conn), + conn->transportSettings.writeConnectionDataPacketsLimit); + EXPECT_EQ(2, stream->retransmissionBuffer.size()); + EXPECT_GT( + stream->retransmissionBuffer.back().offset, + stream->retransmissionBuffer.front().offset); +} + TEST_F(QuicTransportFunctionsTest, NothingWritten) { auto conn = createConn(); auto mockCongestionController = std::make_unique(); diff --git a/quic/loss/test/QuicLossFunctionsTest.cpp b/quic/loss/test/QuicLossFunctionsTest.cpp index 8dc8f1af0..cb63ce001 100644 --- a/quic/loss/test/QuicLossFunctionsTest.cpp +++ b/quic/loss/test/QuicLossFunctionsTest.cpp @@ -389,6 +389,46 @@ TEST_F(QuicLossFunctionsTest, TestMarkPacketLoss) { EXPECT_TRUE(eq(buf, buffer.data.move())); } +TEST_F(QuicLossFunctionsTest, RetxBufferSortedAfterLoss) { + folly::EventBase evb; + MockAsyncUDPSocket socket(&evb); + auto conn = createConn(); + auto stream = conn->streamManager->createNextBidirectionalStream().value(); + auto buf1 = IOBuf::copyBuffer("Worse case scenario"); + auto buf2 = IOBuf::copyBuffer("The hard problem"); + auto buf3 = IOBuf::copyBuffer("And then we had a flash of insight..."); + writeQuicPacket( + *conn, + *conn->clientConnectionId, + *conn->serverConnectionId, + socket, + *stream, + *buf1); + writeQuicPacket( + *conn, + *conn->clientConnectionId, + *conn->serverConnectionId, + socket, + *stream, + *buf2); + writeQuicPacket( + *conn, + *conn->clientConnectionId, + *conn->serverConnectionId, + socket, + *stream, + *buf3); + EXPECT_EQ(3, stream->retransmissionBuffer.size()); + EXPECT_EQ(3, conn->outstandingPackets.size()); + auto packet = conn->outstandingPackets[folly::Random::rand32() % 3]; + markPacketLoss( + *conn, packet.packet, false, packet.packet.header.getPacketSequenceNum()); + EXPECT_EQ(2, stream->retransmissionBuffer.size()); + EXPECT_GT( + stream->retransmissionBuffer.back().offset, + stream->retransmissionBuffer.front().offset); +} + TEST_F(QuicLossFunctionsTest, TestMarkCryptoLostAfterCancelRetransmission) { folly::EventBase evb; MockAsyncUDPSocket socket(&evb); diff --git a/quic/state/stream/test/StreamStateMachineTest.cpp b/quic/state/stream/test/StreamStateMachineTest.cpp index 884b710ae..94728fb90 100644 --- a/quic/state/stream/test/StreamStateMachineTest.cpp +++ b/quic/state/stream/test/StreamStateMachineTest.cpp @@ -217,6 +217,55 @@ TEST_F(QuicOpenStateTest, AckStream) { ASSERT_TRUE(isState(stream->send)); } +TEST_F(QuicOpenStateTest, RetxBufferSortedAfterAck) { + auto conn = createConn(); + auto stream = conn->streamManager->createNextBidirectionalStream().value(); + EventBase evb; + folly::test::MockAsyncUDPSocket socket(&evb); + folly::Optional serverChosenConnId = *conn->clientConnectionId; + serverChosenConnId.value().data()[0] ^= 0x01; + + auto buf1 = IOBuf::copyBuffer("Alice"); + auto buf2 = IOBuf::copyBuffer("Bob"); + auto buf3 = IOBuf::copyBuffer("NSA"); + writeQuicPacket( + *conn, + *conn->clientConnectionId, + *serverChosenConnId, + socket, + *stream, + *buf1, + false); + writeQuicPacket( + *conn, + *conn->clientConnectionId, + *serverChosenConnId, + socket, + *stream, + *buf2, + false); + writeQuicPacket( + *conn, + *conn->clientConnectionId, + *serverChosenConnId, + socket, + *stream, + *buf3, + false); + + EXPECT_EQ(3, stream->retransmissionBuffer.size()); + EXPECT_EQ(3, conn->outstandingPackets.size()); + auto packet = conn->outstandingPackets[folly::Random::rand32() % 3]; + auto streamFrame = boost::get( + conn->outstandingPackets[std::rand() % 3].packet.frames.front()); + StreamEvents::AckStreamFrame ack(streamFrame); + invokeHandler(stream->send, ack, *stream); + EXPECT_EQ(2, stream->retransmissionBuffer.size()); + EXPECT_GT( + stream->retransmissionBuffer.back().offset, + stream->retransmissionBuffer.front().offset); +} + TEST_F(QuicOpenStateTest, AckStreamAfterSkip) { auto conn = createConn(); conn->partialReliabilityEnabled = true;