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 <hta@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#36683}
diff --git a/net/dcsctp/tx/outstanding_data.cc b/net/dcsctp/tx/outstanding_data.cc
index 05277d0..b0aa09f 100644
--- a/net/dcsctp/tx/outstanding_data.cc
+++ b/net/dcsctp/tx/outstanding_data.cc
@@ -39,8 +39,8 @@
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 c161cbb..4363524d 100644
--- a/net/dcsctp/tx/outstanding_data_test.cc
+++ b/net/dcsctp/tx/outstanding_data_test.cc
@@ -397,5 +397,79 @@
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<SackChunk::GapAckBlock> 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<SackChunk::GapAckBlock> 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<SackChunk::GapAckBlock> 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<SackChunk::GapAckBlock> 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<SackChunk::GapAckBlock> 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<SackChunk::GapAckBlock> 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<SackChunk::GapAckBlock> 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<SackChunk::GapAckBlock> 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<SackChunk::GapAckBlock> 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
