From ddc2f334c49c5287dae7ade273a530f4817ee0a6 Mon Sep 17 00:00:00 2001 From: Victor Boivie Date: Wed, 20 Apr 2022 10:10:38 +0200 Subject: [PATCH] Revert "dcsctp: Avoid bundling FORWARD-TSN and DATA chunks" This proved to be not very efficient unfortunately, so revert it and keep bundling FORWARD-TSN with other packets to be more efficient. https://github.com/sctplab/usrsctp/issues/597 is still unresolved. Note that this is not a clean revert; The logic to rate limit the sending of FORWARD-TSN is kept, as it still makes sense. This partly reverts commit 0ca62e3752149ad37f73bf074db0a5f8fcaf6585. Bug: webrtc:12961 Change-Id: I42728434290e7ece19e9c23f24ef6f3d3b171315 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/259520 Reviewed-by: Harald Alvestrand Commit-Queue: Victor Boivie Cr-Commit-Position: refs/heads/main@{#36584} --- net/dcsctp/socket/dcsctp_socket_test.cc | 84 ------------------- .../socket/transmission_control_block.cc | 4 +- 2 files changed, 1 insertion(+), 87 deletions(-) diff --git a/net/dcsctp/socket/dcsctp_socket_test.cc b/net/dcsctp/socket/dcsctp_socket_test.cc index a6b8aa62bf..b4bc9c486e 100644 --- a/net/dcsctp/socket/dcsctp_socket_test.cc +++ b/net/dcsctp/socket/dcsctp_socket_test.cc @@ -27,7 +27,6 @@ #include "net/dcsctp/packet/chunk/data_chunk.h" #include "net/dcsctp/packet/chunk/data_common.h" #include "net/dcsctp/packet/chunk/error_chunk.h" -#include "net/dcsctp/packet/chunk/forward_tsn_chunk.h" #include "net/dcsctp/packet/chunk/heartbeat_ack_chunk.h" #include "net/dcsctp/packet/chunk/heartbeat_request_chunk.h" #include "net/dcsctp/packet/chunk/idata_chunk.h" @@ -2023,89 +2022,6 @@ TEST_P(DcSctpSocketParametrizedTest, SendsOnlyLargePackets) { MaybeHandoverSocketAndSendMessage(a, std::move(z)); } -TEST_P(DcSctpSocketParametrizedTest, DoesntBundleForwardTsnWithData) { - SocketUnderTest a("A"); - auto z = std::make_unique("Z"); - - ConnectSockets(a, *z); - z = MaybeHandoverSocket(std::move(z)); - - // Force an RTT measurement using heartbeats. - AdvanceTime(a, *z, a.options.heartbeat_interval); - - // HEARTBEAT - std::vector hb_req_a = a.cb.ConsumeSentPacket(); - std::vector hb_req_z = z->cb.ConsumeSentPacket(); - - constexpr DurationMs kRtt = DurationMs(80); - AdvanceTime(a, *z, kRtt); - z->socket.ReceivePacket(hb_req_a); - a.socket.ReceivePacket(hb_req_z); - - // HEARTBEAT_ACK - a.socket.ReceivePacket(z->cb.ConsumeSentPacket()); - z->socket.ReceivePacket(a.cb.ConsumeSentPacket()); - - SendOptions send_options; - send_options.max_retransmissions = 0; - std::vector payload(a.options.mtu - 100); - - // Send an initial message that is received, but the SACK was lost - a.socket.Send(DcSctpMessage(StreamID(1), PPID(51), payload), send_options); - // DATA - z->socket.ReceivePacket(a.cb.ConsumeSentPacket()); - // SACK (lost) - std::vector sack = z->cb.ConsumeSentPacket(); - - // Queue enough messages to fill the congestion window. - do { - a.socket.Send(DcSctpMessage(StreamID(1), PPID(51), payload), send_options); - } while (!a.cb.ConsumeSentPacket().empty()); - - // Enqueue at least one more. - a.socket.Send(DcSctpMessage(StreamID(1), PPID(51), payload), send_options); - - // Let all of them expire by T3-RTX and inspect what's sent. - AdvanceTime(a, *z, a.options.rto_initial); - - std::vector sent1 = a.cb.ConsumeSentPacket(); - ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket packet1, SctpPacket::Parse(sent1)); - - EXPECT_THAT(packet1.descriptors(), SizeIs(1)); - EXPECT_EQ(packet1.descriptors()[0].type, ForwardTsnChunk::kType); - - std::vector sent2 = a.cb.ConsumeSentPacket(); - ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket packet2, SctpPacket::Parse(sent2)); - - EXPECT_GE(packet2.descriptors().size(), 1u); - EXPECT_EQ(packet2.descriptors()[0].type, DataChunk::kType); - - // Drop all remaining packets that A has sent. - while (!a.cb.ConsumeSentPacket().empty()) { - } - - // Replay the SACK, and see if a FORWARD-TSN is sent again. - a.socket.ReceivePacket(sack); - - // It shouldn't be sent as not enough time has passed yet. Instead, more - // DATA chunks are sent, that are in the queue. - std::vector sent3 = a.cb.ConsumeSentPacket(); - ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket packet3, SctpPacket::Parse(sent3)); - - EXPECT_GE(packet2.descriptors().size(), 1u); - EXPECT_EQ(packet3.descriptors()[0].type, DataChunk::kType); - - // Now let RTT time pass, to allow a FORWARD-TSN to be sent again. - AdvanceTime(a, *z, kRtt); - a.socket.ReceivePacket(sack); - - std::vector sent4 = a.cb.ConsumeSentPacket(); - ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket packet4, SctpPacket::Parse(sent4)); - - EXPECT_THAT(packet4.descriptors(), SizeIs(1)); - EXPECT_EQ(packet4.descriptors()[0].type, ForwardTsnChunk::kType); -} - TEST(DcSctpSocketTest, SendMessagesAfterHandover) { SocketUnderTest a("A"); auto z = std::make_unique("Z"); diff --git a/net/dcsctp/socket/transmission_control_block.cc b/net/dcsctp/socket/transmission_control_block.cc index 0fc0d4c029..539fde7300 100644 --- a/net/dcsctp/socket/transmission_control_block.cc +++ b/net/dcsctp/socket/transmission_control_block.cc @@ -104,9 +104,6 @@ void TransmissionControlBlock::MaybeSendForwardTsn(SctpPacket::Builder& builder, void TransmissionControlBlock::SendBufferedPackets(SctpPacket::Builder& builder, TimeMs now) { - // FORWARD-TSNs are sent as separate packets to avoid bugs.webrtc.org/12961. - MaybeSendForwardTsn(builder, now); - for (int packet_idx = 0; packet_idx < options_.max_burst && retransmission_queue_.can_send_data(); ++packet_idx) { @@ -131,6 +128,7 @@ void TransmissionControlBlock::SendBufferedPackets(SctpPacket::Builder& builder, builder.Add(data_tracker_.CreateSelectiveAck( reassembly_queue_.remaining_bytes())); } + MaybeSendForwardTsn(builder, now); absl::optional reconfig = stream_reset_handler_.MakeStreamResetRequest(); if (reconfig.has_value()) {