diff --git a/net/dcsctp/socket/dcsctp_socket_test.cc b/net/dcsctp/socket/dcsctp_socket_test.cc index 6e0bbf7b56..1c8525a6f3 100644 --- a/net/dcsctp/socket/dcsctp_socket_test.cc +++ b/net/dcsctp/socket/dcsctp_socket_test.cc @@ -26,6 +26,7 @@ #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" @@ -1764,5 +1765,86 @@ TEST_F(DcSctpSocketTest, SendsOnlyLargePackets) { } } +TEST_F(DcSctpSocketTest, DoesntBundleForwardTsnWithData) { + ConnectSockets(); + + // Force an RTT measurement using heartbeats. + AdvanceTime(options_.heartbeat_interval); + RunTimers(); + + // HEARTBEAT + std::vector hb_req_a = cb_a_.ConsumeSentPacket(); + std::vector hb_req_z = cb_z_.ConsumeSentPacket(); + + constexpr DurationMs kRtt = DurationMs(80); + AdvanceTime(kRtt); + sock_z_.ReceivePacket(hb_req_a); + sock_a_.ReceivePacket(hb_req_z); + + // HEARTBEAT_ACK + sock_a_.ReceivePacket(cb_z_.ConsumeSentPacket()); + sock_z_.ReceivePacket(cb_a_.ConsumeSentPacket()); + + SendOptions send_options; + send_options.max_retransmissions = 0; + std::vector payload(options_.mtu - 100); + + // Send an initial message that is received, but the SACK was lost + sock_a_.Send(DcSctpMessage(StreamID(1), PPID(51), payload), send_options); + // DATA + sock_z_.ReceivePacket(cb_a_.ConsumeSentPacket()); + // SACK (lost) + std::vector sack = cb_z_.ConsumeSentPacket(); + + // Queue enough messages to fill the congestion window. + do { + sock_a_.Send(DcSctpMessage(StreamID(1), PPID(51), payload), send_options); + } while (!cb_a_.ConsumeSentPacket().empty()); + + // Enqueue at least one more. + sock_a_.Send(DcSctpMessage(StreamID(1), PPID(51), payload), send_options); + + // Let all of them expire by T3-RTX and inspect what's sent. + AdvanceTime(options_.rto_initial); + RunTimers(); + + std::vector sent1 = cb_a_.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 = cb_a_.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 (!cb_a_.ConsumeSentPacket().empty()) { + } + + // Replay the SACK, and see if a FORWARD-TSN is sent again. + sock_a_.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 = cb_a_.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(kRtt); + sock_a_.ReceivePacket(sack); + + std::vector sent4 = cb_a_.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); +} + } // namespace } // namespace dcsctp diff --git a/net/dcsctp/socket/transmission_control_block.cc b/net/dcsctp/socket/transmission_control_block.cc index cc29ebd67e..f0f1ab9782 100644 --- a/net/dcsctp/socket/transmission_control_block.cc +++ b/net/dcsctp/socket/transmission_control_block.cc @@ -82,8 +82,31 @@ void TransmissionControlBlock::MaybeSendSack() { } } +void TransmissionControlBlock::MaybeSendForwardTsn(SctpPacket::Builder& builder, + TimeMs now) { + if (now >= limit_forward_tsn_until_ && + retransmission_queue_.ShouldSendForwardTsn(now)) { + if (capabilities_.message_interleaving) { + builder.Add(retransmission_queue_.CreateIForwardTsn()); + } else { + builder.Add(retransmission_queue_.CreateForwardTsn()); + } + packet_sender_.Send(builder); + // https://datatracker.ietf.org/doc/html/rfc3758 + // "IMPLEMENTATION NOTE: An implementation may wish to limit the number of + // duplicate FORWARD TSN chunks it sends by ... waiting a full RTT before + // sending a duplicate FORWARD TSN." + // "Any delay applied to the sending of FORWARD TSN chunk SHOULD NOT exceed + // 200ms and MUST NOT exceed 500ms". + limit_forward_tsn_until_ = now + std::min(DurationMs(200), rto_.srtt()); + } +} + 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) { @@ -108,13 +131,6 @@ void TransmissionControlBlock::SendBufferedPackets(SctpPacket::Builder& builder, builder.Add(data_tracker_.CreateSelectiveAck( reassembly_queue_.remaining_bytes())); } - if (retransmission_queue_.ShouldSendForwardTsn(now)) { - if (capabilities_.message_interleaving) { - builder.Add(retransmission_queue_.CreateIForwardTsn()); - } else { - builder.Add(retransmission_queue_.CreateForwardTsn()); - } - } absl::optional reconfig = stream_reset_handler_.MakeStreamResetRequest(); if (reconfig.has_value()) { diff --git a/net/dcsctp/socket/transmission_control_block.h b/net/dcsctp/socket/transmission_control_block.h index b189ae82e0..4510d83365 100644 --- a/net/dcsctp/socket/transmission_control_block.h +++ b/net/dcsctp/socket/transmission_control_block.h @@ -152,6 +152,9 @@ class TransmissionControlBlock : public Context { // Sends a SACK, if there is a need to. void MaybeSendSack(); + // Sends a FORWARD-TSN, if it is needed and allowed (rate-limited). + void MaybeSendForwardTsn(SctpPacket::Builder& builder, TimeMs now); + // Will be set while the socket is in kCookieEcho state. In this state, there // can only be a single packet outstanding, and it must contain the COOKIE // ECHO chunk as the first chunk in that packet, until the COOKIE ACK has been @@ -206,6 +209,8 @@ class TransmissionControlBlock : public Context { const TieTag tie_tag_; const std::function is_connection_established_; PacketSender& packet_sender_; + // Rate limiting of FORWARD-TSN. Next can be sent at or after this timestamp. + TimeMs limit_forward_tsn_until_ = TimeMs(0); RetransmissionTimeout rto_; RetransmissionErrorCounter tx_error_counter_;