From c8e8e767e2782a87cefdafdcae73759dabca7625 Mon Sep 17 00:00:00 2001 From: Victor Boivie Date: Fri, 10 Jun 2022 22:50:49 +0200 Subject: [PATCH] dcsctp: Restart T3-RTX on fast rtx of lowest TSN RFC 4960, 7.2.4. Fast Retransmit on Gap Reports 4) Restart the T3-rtx timer only if the last SACK acknowledged the lowest outstanding TSN number sent to that address, or the endpoint is retransmitting the first outstanding DATA chunk sent to that address. The second part of this sentence, "or the endpoint is retransmitting the first outstanding DATA chunk sent to that address." This means that on fast retransmit, and if the retransmitted chunks weren't quickly acknowledged by the peer, the retransmission timer would expire too quickly. With this CL, it will restart, as it should. Bug: webrtc:12943 Change-Id: I7627253718530876acc516460562e66bfcc79533 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/265620 Reviewed-by: Harald Alvestrand Commit-Queue: Victor Boivie Cr-Commit-Position: refs/heads/main@{#37193} --- net/dcsctp/tx/retransmission_queue.cc | 11 +++ net/dcsctp/tx/retransmission_queue_test.cc | 84 +++++++++++++++++++++- 2 files changed, 94 insertions(+), 1 deletion(-) diff --git a/net/dcsctp/tx/retransmission_queue.cc b/net/dcsctp/tx/retransmission_queue.cc index 57559195f0..f26e8baa44 100644 --- a/net/dcsctp/tx/retransmission_queue.cc +++ b/net/dcsctp/tx/retransmission_queue.cc @@ -398,6 +398,17 @@ RetransmissionQueue::GetChunksForFastRetransmit(size_t bytes_in_packet) { outstanding_data_.GetChunksToBeFastRetransmitted(bytes_in_packet); RTC_DCHECK(!to_be_sent.empty()); + // https://tools.ietf.org/html/rfc4960#section-7.2.4 + // "4) Restart the T3-rtx timer only if ... the endpoint is retransmitting + // the first outstanding DATA chunk sent to that address." + if (to_be_sent[0].first == + outstanding_data_.last_cumulative_tsn_ack().next_value().Wrap()) { + RTC_DLOG(LS_VERBOSE) + << log_prefix_ + << "First outstanding DATA to be retransmitted - restarting T3-RTX"; + t3_rtx_.Stop(); + } + // https://tools.ietf.org/html/rfc4960#section-6.3.2 // "Every time a DATA chunk is sent to any address (including a // retransmission), if the T3-rtx timer of that address is not running, diff --git a/net/dcsctp/tx/retransmission_queue_test.cc b/net/dcsctp/tx/retransmission_queue_test.cc index b232bb2d82..1d28cb23a1 100644 --- a/net/dcsctp/tx/retransmission_queue_test.cc +++ b/net/dcsctp/tx/retransmission_queue_test.cc @@ -29,6 +29,7 @@ #include "net/dcsctp/packet/data.h" #include "net/dcsctp/public/dcsctp_options.h" #include "net/dcsctp/testing/data_generator.h" +#include "net/dcsctp/testing/testing_macros.h" #include "net/dcsctp/timer/fake_timeout.h" #include "net/dcsctp/timer/timer.h" #include "net/dcsctp/tx/mock_send_queue.h" @@ -70,7 +71,7 @@ class RetransmissionQueueTest : public testing::Test { timer_(timer_manager_.CreateTimer( "test/t3_rtx", []() { return absl::nullopt; }, - TimerOptions(DurationMs(0)))) {} + TimerOptions(options_.rto_initial))) {} std::function CreateChunk() { return [this](TimeMs now, size_t max_size) { @@ -302,6 +303,87 @@ TEST_F(RetransmissionQueueTest, ResendPacketsWhenNackedThreeTimes) { Pair(TSN(20), State::kAcked))); } +TEST_F(RetransmissionQueueTest, RestartsT3RtxOnRetransmitFirstOutstandingTSN) { + // Verifies that if fast retransmit is retransmitting the first outstanding + // TSN, it will also restart T3-RTX. + RetransmissionQueue queue = CreateQueue(); + EXPECT_CALL(producer_, Produce) + .WillOnce(CreateChunk()) + .WillOnce(CreateChunk()) + .WillOnce(CreateChunk()) + .WillRepeatedly([](TimeMs, size_t) { return absl::nullopt; }); + + static constexpr TimeMs kStartTime(100000); + now_ = kStartTime; + + EXPECT_THAT(GetSentPacketTSNs(queue), + testing::ElementsAre(TSN(10), TSN(11), TSN(12))); + + // Ack 10, 12, after 100ms. + now_ += DurationMs(100); + queue.HandleSack( + now_, SackChunk(TSN(10), kArwnd, {SackChunk::GapAckBlock(2, 2)}, {})); + + EXPECT_THAT(queue.GetChunkStatesForTesting(), + ElementsAre(Pair(TSN(10), State::kAcked), // + Pair(TSN(11), State::kNacked), // + Pair(TSN(12), State::kAcked))); + + // Send 13 + EXPECT_CALL(producer_, Produce) + .WillOnce(CreateChunk()) + .WillRepeatedly([](TimeMs, size_t) { return absl::nullopt; }); + EXPECT_THAT(GetSentPacketTSNs(queue), testing::ElementsAre(TSN(13))); + + // Ack 10, 12-13, after 100ms. + now_ += DurationMs(100); + queue.HandleSack( + now_, SackChunk(TSN(10), kArwnd, {SackChunk::GapAckBlock(2, 3)}, {})); + + // Send 14 + EXPECT_CALL(producer_, Produce) + .WillOnce(CreateChunk()) + .WillRepeatedly([](TimeMs, size_t) { return absl::nullopt; }); + EXPECT_THAT(GetSentPacketTSNs(queue), testing::ElementsAre(TSN(14))); + + // Ack 10, 12-14, after 100 ms. + now_ += DurationMs(100); + queue.HandleSack( + now_, SackChunk(TSN(10), kArwnd, {SackChunk::GapAckBlock(2, 4)}, {})); + + EXPECT_THAT(queue.GetChunkStatesForTesting(), + ElementsAre(Pair(TSN(10), State::kAcked), // + Pair(TSN(11), State::kToBeRetransmitted), // + Pair(TSN(12), State::kAcked), // + Pair(TSN(13), State::kAcked), // + Pair(TSN(14), State::kAcked))); + + // This will trigger "fast retransmit" mode and only chunks 13 and 16 will be + // resent right now. The send queue will not even be queried. + EXPECT_CALL(producer_, Produce).Times(0); + + EXPECT_THAT(GetTSNsForFastRetransmit(queue), testing::ElementsAre(TSN(11))); + + EXPECT_THAT(queue.GetChunkStatesForTesting(), + ElementsAre(Pair(TSN(10), State::kAcked), // + Pair(TSN(11), State::kInFlight), // + Pair(TSN(12), State::kAcked), // + Pair(TSN(13), State::kAcked), // + Pair(TSN(14), State::kAcked))); + + // Verify that the timer was really restarted when fast-retransmitting. The + // timeout is `options_.rto_initial`, so advance the time just before that. + now_ += options_.rto_initial - DurationMs(1); + EXPECT_FALSE(timeout_manager_.GetNextExpiredTimeout().has_value()); + + // And ensure it really is running. + now_ += DurationMs(1); + ASSERT_HAS_VALUE_AND_ASSIGN(TimeoutID timeout, + timeout_manager_.GetNextExpiredTimeout()); + // An expired timeout has to be handled (asserts validate this). + timer_manager_.HandleTimeout(timeout); +} + TEST_F(RetransmissionQueueTest, CanOnlyProduceTwoPacketsButWantsToSendThree) { RetransmissionQueue queue = CreateQueue(); EXPECT_CALL(producer_, Produce)