dcsctp: Add more unit tests for DataTracker
There were some missing unit tests that are now written. When doing
this, it was found that SACKs weren't sent for duplicate received
chunks, which they should be according to the spec.
Bug: webrtc:12614
Change-Id: I8296473c0c8cbaf0329785de95e9b9945f254339
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/220607
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34165}
diff --git a/net/dcsctp/rx/data_tracker.cc b/net/dcsctp/rx/data_tracker.cc
index 5083e09..9c21126 100644
--- a/net/dcsctp/rx/data_tracker.cc
+++ b/net/dcsctp/rx/data_tracker.cc
@@ -58,24 +58,36 @@
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 a19ac8b..5a95552 100644
--- a/net/dcsctp/rx/data_tracker_test.cc
+++ b/net/dcsctp/rx/data_tracker_test.cc
@@ -295,5 +295,97 @@
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
