diff --git a/net/dcsctp/rx/data_tracker.cc b/net/dcsctp/rx/data_tracker.cc index 5083e0981b..9c21126373 100644 --- a/net/dcsctp/rx/data_tracker.cc +++ b/net/dcsctp/rx/data_tracker.cc @@ -58,24 +58,36 @@ void DataTracker::Observe(TSN tsn, if (duplicate_tsns_.size() < kMaxDuplicateTsnReported) { duplicate_tsns_.insert(unwrapped_tsn.Wrap()); } - return; - } - - if (unwrapped_tsn == last_cumulative_acked_tsn_.next_value()) { - last_cumulative_acked_tsn_ = unwrapped_tsn; - // The cumulative acked tsn may be moved even further, if a gap was filled. - while (!additional_tsns_.empty() && - *additional_tsns_.begin() == - last_cumulative_acked_tsn_.next_value()) { - last_cumulative_acked_tsn_.Increment(); - additional_tsns_.erase(additional_tsns_.begin()); - } + // https://datatracker.ietf.org/doc/html/rfc4960#section-6.2 + // "When a packet arrives with duplicate DATA chunk(s) and with no new DATA + // chunk(s), the endpoint MUST immediately send a SACK with no delay. If a + // packet arrives with duplicate DATA chunk(s) bundled with new DATA chunks, + // the endpoint MAY immediately send a SACK." + UpdateAckState(AckState::kImmediate, "duplicate data"); } else { - bool inserted = additional_tsns_.insert(unwrapped_tsn).second; - if (!inserted) { - // Already seen before. - if (duplicate_tsns_.size() < kMaxDuplicateTsnReported) { - duplicate_tsns_.insert(unwrapped_tsn.Wrap()); + if (unwrapped_tsn == last_cumulative_acked_tsn_.next_value()) { + last_cumulative_acked_tsn_ = unwrapped_tsn; + // The cumulative acked tsn may be moved even further, if a gap was + // filled. + while (!additional_tsns_.empty() && + *additional_tsns_.begin() == + last_cumulative_acked_tsn_.next_value()) { + last_cumulative_acked_tsn_.Increment(); + additional_tsns_.erase(additional_tsns_.begin()); + } + } else { + bool inserted = additional_tsns_.insert(unwrapped_tsn).second; + if (!inserted) { + // Already seen before. + if (duplicate_tsns_.size() < kMaxDuplicateTsnReported) { + duplicate_tsns_.insert(unwrapped_tsn.Wrap()); + } + // https://datatracker.ietf.org/doc/html/rfc4960#section-6.2 + // "When a packet arrives with duplicate DATA chunk(s) and with no new + // DATA chunk(s), the endpoint MUST immediately send a SACK with no + // delay. If a packet arrives with duplicate DATA chunk(s) bundled with + // new DATA chunks, the endpoint MAY immediately send a SACK." + // No need to do this. SACKs are sent immediately on packet loss below. } } } diff --git a/net/dcsctp/rx/data_tracker_test.cc b/net/dcsctp/rx/data_tracker_test.cc index a19ac8ba4c..5a955524d6 100644 --- a/net/dcsctp/rx/data_tracker_test.cc +++ b/net/dcsctp/rx/data_tracker_test.cc @@ -295,5 +295,97 @@ TEST_F(DataTrackerTest, LimitsNumberOfGapAckBlocksReported) { SizeIs(DataTracker::kMaxGapAckBlocksReported)); } +TEST_F(DataTrackerTest, SendsSackForFirstPacketObserved) { + Observer({11}); + buf_.ObservePacketEnd(); + EXPECT_TRUE(buf_.ShouldSendAck()); + EXPECT_FALSE(timer_->is_running()); +} + +TEST_F(DataTrackerTest, SendsSackEverySecondPacketWhenThereIsNoPacketLoss) { + Observer({11}); + buf_.ObservePacketEnd(); + EXPECT_TRUE(buf_.ShouldSendAck()); + EXPECT_FALSE(timer_->is_running()); + Observer({12}); + buf_.ObservePacketEnd(); + EXPECT_FALSE(buf_.ShouldSendAck()); + EXPECT_TRUE(timer_->is_running()); + Observer({13}); + buf_.ObservePacketEnd(); + EXPECT_TRUE(buf_.ShouldSendAck()); + EXPECT_FALSE(timer_->is_running()); + Observer({14}); + buf_.ObservePacketEnd(); + EXPECT_FALSE(buf_.ShouldSendAck()); + EXPECT_TRUE(timer_->is_running()); + Observer({15}); + buf_.ObservePacketEnd(); + EXPECT_TRUE(buf_.ShouldSendAck()); + EXPECT_FALSE(timer_->is_running()); +} + +TEST_F(DataTrackerTest, SendsSackEveryPacketOnPacketLoss) { + Observer({11}); + buf_.ObservePacketEnd(); + EXPECT_TRUE(buf_.ShouldSendAck()); + EXPECT_FALSE(timer_->is_running()); + Observer({13}); + buf_.ObservePacketEnd(); + EXPECT_TRUE(buf_.ShouldSendAck()); + EXPECT_FALSE(timer_->is_running()); + Observer({14}); + buf_.ObservePacketEnd(); + EXPECT_TRUE(buf_.ShouldSendAck()); + EXPECT_FALSE(timer_->is_running()); + Observer({15}); + buf_.ObservePacketEnd(); + EXPECT_TRUE(buf_.ShouldSendAck()); + EXPECT_FALSE(timer_->is_running()); + Observer({16}); + buf_.ObservePacketEnd(); + EXPECT_TRUE(buf_.ShouldSendAck()); + EXPECT_FALSE(timer_->is_running()); + // Fill the hole. + Observer({12}); + buf_.ObservePacketEnd(); + EXPECT_FALSE(buf_.ShouldSendAck()); + EXPECT_TRUE(timer_->is_running()); + // Goes back to every second packet + Observer({17}); + buf_.ObservePacketEnd(); + EXPECT_TRUE(buf_.ShouldSendAck()); + EXPECT_FALSE(timer_->is_running()); + Observer({18}); + buf_.ObservePacketEnd(); + EXPECT_FALSE(buf_.ShouldSendAck()); + EXPECT_TRUE(timer_->is_running()); +} + +TEST_F(DataTrackerTest, SendsSackOnDuplicateDataChunks) { + Observer({11}); + buf_.ObservePacketEnd(); + EXPECT_TRUE(buf_.ShouldSendAck()); + EXPECT_FALSE(timer_->is_running()); + Observer({11}); + buf_.ObservePacketEnd(); + EXPECT_TRUE(buf_.ShouldSendAck()); + EXPECT_FALSE(timer_->is_running()); + Observer({12}); + buf_.ObservePacketEnd(); + EXPECT_FALSE(buf_.ShouldSendAck()); + EXPECT_TRUE(timer_->is_running()); + // Goes back to every second packet + Observer({13}); + buf_.ObservePacketEnd(); + EXPECT_TRUE(buf_.ShouldSendAck()); + EXPECT_FALSE(timer_->is_running()); + // Duplicate again + Observer({12}); + buf_.ObservePacketEnd(); + EXPECT_TRUE(buf_.ShouldSendAck()); + EXPECT_FALSE(timer_->is_running()); +} + } // namespace } // namespace dcsctp