From 01a0db2e74b347db1e8720c78c208b218fe6636f Mon Sep 17 00:00:00 2001 From: Victor Boivie Date: Fri, 22 Apr 2022 13:33:36 +0200 Subject: [PATCH] dcsctp: Don't re-nack a fragment until sent again This is mainly an issue when sending items with partial reliability, with high bandwidth on a link with packet loss. In SCTP, when a fragment isn't included in the SACK a number of times, it's scheduled to be retransmitted or abandoned, if it has been retransmitted too many times already (depending configuration). Before this CL, if a fragment was NACKed and scheduled for retransmission, but couldn't be retransmitted immediately (due to congestion window not allowing it), future received SACKs - that would still indicate that the fragment hasn't been received yet - would still increment the retransmission counter. Which wasn't fair, because this fragment hasn't had a chance to be retransmitted yet. With this CL, the fragment will only have its retransmission counter increased when it is not already scheduled to be retransmitted, but actually sent on the wire and considered in-flight again. Bug: webrtc:12943 Change-Id: I2af08255650221c044cc14591a5835c885e94c58 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/259825 Reviewed-by: Harald Alvestrand Commit-Queue: Victor Boivie Cr-Commit-Position: refs/heads/main@{#36683} --- net/dcsctp/tx/outstanding_data.cc | 4 +- net/dcsctp/tx/outstanding_data_test.cc | 74 ++++++++++++++++++++++++++ 2 files changed, 76 insertions(+), 2 deletions(-) diff --git a/net/dcsctp/tx/outstanding_data.cc b/net/dcsctp/tx/outstanding_data.cc index 05277d0584..b0aa09fc36 100644 --- a/net/dcsctp/tx/outstanding_data.cc +++ b/net/dcsctp/tx/outstanding_data.cc @@ -39,8 +39,8 @@ OutstandingData::Item::NackAction OutstandingData::Item::Nack( bool retransmit_now) { ack_state_ = AckState::kNacked; ++nack_count_; - if ((retransmit_now || nack_count_ >= kNumberOfNacksForRetransmission) && - !is_abandoned_) { + if (!should_be_retransmitted() && !is_abandoned() && + (retransmit_now || nack_count_ >= kNumberOfNacksForRetransmission)) { // Nacked enough times - it's considered lost. if (num_retransmissions_ < *max_retransmissions_) { should_be_retransmitted_ = true; diff --git a/net/dcsctp/tx/outstanding_data_test.cc b/net/dcsctp/tx/outstanding_data_test.cc index c161cbb6da..4363524d0e 100644 --- a/net/dcsctp/tx/outstanding_data_test.cc +++ b/net/dcsctp/tx/outstanding_data_test.cc @@ -397,5 +397,79 @@ TEST_F(OutstandingDataTest, MeasureRTT) { EXPECT_EQ(duration, kDuration - DurationMs(1)); } +TEST_F(OutstandingDataTest, MustRetransmitBeforeGettingNackedAgain) { + // This test case verifies that a chunk that has been nacked, and scheduled to + // be retransmitted, doesn't get nacked again until it has been actually sent + // on the wire. + + static constexpr MaxRetransmits kOneRetransmission(1); + for (int tsn = 10; tsn <= 20; ++tsn) { + buf_.Insert(gen_.Ordered({1}, tsn == 10 ? "B" : tsn == 20 ? "E" : ""), + kOneRetransmission, kNow, TimeMs::InfiniteFuture()); + } + + std::vector gab1 = {SackChunk::GapAckBlock(2, 2)}; + EXPECT_FALSE( + buf_.HandleSack(unwrapper_.Unwrap(TSN(9)), gab1, false).has_packet_loss); + EXPECT_FALSE(buf_.has_data_to_be_retransmitted()); + + std::vector gab2 = {SackChunk::GapAckBlock(2, 3)}; + EXPECT_FALSE( + buf_.HandleSack(unwrapper_.Unwrap(TSN(9)), gab2, false).has_packet_loss); + EXPECT_FALSE(buf_.has_data_to_be_retransmitted()); + + std::vector gab3 = {SackChunk::GapAckBlock(2, 4)}; + OutstandingData::AckInfo ack = + buf_.HandleSack(unwrapper_.Unwrap(TSN(9)), gab3, false); + EXPECT_TRUE(ack.has_packet_loss); + EXPECT_TRUE(buf_.has_data_to_be_retransmitted()); + + // Don't call GetChunksToBeRetransmitted yet - simulate that the congestion + // window doesn't allow it to be retransmitted yet. It does however get more + // SACKs indicating packet loss. + + std::vector gab4 = {SackChunk::GapAckBlock(2, 5)}; + EXPECT_FALSE( + buf_.HandleSack(unwrapper_.Unwrap(TSN(9)), gab4, false).has_packet_loss); + EXPECT_TRUE(buf_.has_data_to_be_retransmitted()); + + std::vector gab5 = {SackChunk::GapAckBlock(2, 6)}; + EXPECT_FALSE( + buf_.HandleSack(unwrapper_.Unwrap(TSN(9)), gab5, false).has_packet_loss); + EXPECT_TRUE(buf_.has_data_to_be_retransmitted()); + + std::vector gab6 = {SackChunk::GapAckBlock(2, 7)}; + OutstandingData::AckInfo ack2 = + buf_.HandleSack(unwrapper_.Unwrap(TSN(9)), gab6, false); + + EXPECT_FALSE(ack2.has_packet_loss); + EXPECT_TRUE(buf_.has_data_to_be_retransmitted()); + + // Now it's retransmitted. + EXPECT_THAT(buf_.GetChunksToBeRetransmitted(1000), + ElementsAre(Pair(TSN(10), _))); + + // And obviously lost, as it will get NACKed and abandoned. + std::vector gab7 = {SackChunk::GapAckBlock(2, 8)}; + EXPECT_FALSE( + buf_.HandleSack(unwrapper_.Unwrap(TSN(9)), gab7, false).has_packet_loss); + EXPECT_FALSE(buf_.has_data_to_be_retransmitted()); + + std::vector gab8 = {SackChunk::GapAckBlock(2, 9)}; + EXPECT_FALSE( + buf_.HandleSack(unwrapper_.Unwrap(TSN(9)), gab8, false).has_packet_loss); + EXPECT_FALSE(buf_.has_data_to_be_retransmitted()); + + EXPECT_CALL(on_discard_, Call(IsUnordered(false), StreamID(1), MID(42))) + .WillOnce(Return(false)); + + std::vector gab9 = {SackChunk::GapAckBlock(2, 10)}; + OutstandingData::AckInfo ack3 = + buf_.HandleSack(unwrapper_.Unwrap(TSN(9)), gab9, false); + + EXPECT_TRUE(ack3.has_packet_loss); + EXPECT_FALSE(buf_.has_data_to_be_retransmitted()); +} + } // namespace } // namespace dcsctp