diff --git a/media/BUILD.gn b/media/BUILD.gn index 0e6b340d4c..61a29f168c 100644 --- a/media/BUILD.gn +++ b/media/BUILD.gn @@ -411,6 +411,7 @@ if (rtc_build_dcsctp) { "../p2p:rtc_p2p", "../rtc_base:checks", "../rtc_base:rtc_base_approved", + "../rtc_base:socket", "../rtc_base:threading", "../rtc_base/task_utils:pending_task_safety_flag", "../rtc_base/task_utils:to_queued_task", diff --git a/media/sctp/dcsctp_transport.cc b/media/sctp/dcsctp_transport.cc index 3b89af1ec2..2f251980df 100644 --- a/media/sctp/dcsctp_transport.cc +++ b/media/sctp/dcsctp_transport.cc @@ -26,6 +26,7 @@ #include "p2p/base/packet_transport_internal.h" #include "rtc_base/checks.h" #include "rtc_base/logging.h" +#include "rtc_base/socket.h" #include "rtc_base/strings/string_builder.h" #include "rtc_base/thread.h" #include "rtc_base/trace_event.h" @@ -34,6 +35,7 @@ namespace webrtc { namespace { +using ::dcsctp::SendPacketStatus; enum class WebrtcPPID : dcsctp::PPID::UnderlyingType { // https://www.rfc-editor.org/rfc/rfc8832.html#section-8.1 @@ -308,7 +310,8 @@ void DcSctpTransport::set_debug_name_for_testing(const char* debug_name) { debug_name_ = debug_name; } -void DcSctpTransport::SendPacket(rtc::ArrayView data) { +SendPacketStatus DcSctpTransport::SendPacketWithStatus( + rtc::ArrayView data) { RTC_DCHECK_RUN_ON(network_thread_); RTC_DCHECK(socket_); @@ -318,12 +321,12 @@ void DcSctpTransport::SendPacket(rtc::ArrayView data) { "SCTP seems to have made a packet that is bigger " "than its official MTU: " << data.size() << " vs max of " << socket_->options().mtu; - return; + return SendPacketStatus::kError; } TRACE_EVENT0("webrtc", "DcSctpTransport::SendPacket"); if (!transport_ || !transport_->writable()) - return; + return SendPacketStatus::kError; RTC_LOG(LS_VERBOSE) << debug_name_ << "->SendPacket(length=" << data.size() << ")"; @@ -336,7 +339,13 @@ void DcSctpTransport::SendPacket(rtc::ArrayView data) { RTC_LOG(LS_WARNING) << debug_name_ << "->SendPacket(length=" << data.size() << ") failed with error: " << transport_->GetError() << "."; + + if (rtc::IsBlockingError(transport_->GetError())) { + return SendPacketStatus::kTemporaryFailure; + } + return SendPacketStatus::kError; } + return SendPacketStatus::kSuccess; } std::unique_ptr DcSctpTransport::CreateTimeout() { diff --git a/media/sctp/dcsctp_transport.h b/media/sctp/dcsctp_transport.h index 15933383b5..c8c5199396 100644 --- a/media/sctp/dcsctp_transport.h +++ b/media/sctp/dcsctp_transport.h @@ -59,7 +59,8 @@ class DcSctpTransport : public cricket::SctpTransportInternal, private: // dcsctp::DcSctpSocketCallbacks - void SendPacket(rtc::ArrayView data) override; + dcsctp::SendPacketStatus SendPacketWithStatus( + rtc::ArrayView data) override; std::unique_ptr CreateTimeout() override; dcsctp::TimeMs TimeMillis() override; uint32_t GetRandomInt(uint32_t low, uint32_t high) override; diff --git a/net/dcsctp/public/dcsctp_socket.h b/net/dcsctp/public/dcsctp_socket.h index dde0cb68cd..ad40a1b24b 100644 --- a/net/dcsctp/public/dcsctp_socket.h +++ b/net/dcsctp/public/dcsctp_socket.h @@ -155,6 +155,19 @@ inline constexpr absl::string_view ToString(ResetStreamsStatus error) { } } +// Return value of DcSctpSocketCallbacks::SendPacketWithStatus. +enum class SendPacketStatus { + // Indicates that the packet was successfully sent. As sending is unreliable, + // there are no guarantees that the packet was actually delivered. + kSuccess, + // The packet was not sent due to a temporary failure, such as the local send + // buffer becoming exhausted. This return value indicates that the socket will + // recover and sending that packet can be retried at a later time. + kTemporaryFailure, + // The packet was not sent due to other reasons. + kError, +}; + // Tracked metrics, which is the return value of GetMetrics. Optional members // will be unset when they are not yet known. struct Metrics { @@ -205,9 +218,22 @@ class DcSctpSocketCallbacks { // Called when the library wants the packet serialized as `data` to be sent. // + // TODO(bugs.webrtc.org/12943): This method is deprecated, see + // `SendPacketWithStatus`. + // // Note that it's NOT ALLOWED to call into this library from within this // callback. - virtual void SendPacket(rtc::ArrayView data) = 0; + virtual void SendPacket(rtc::ArrayView data) {} + + // Called when the library wants the packet serialized as `data` to be sent. + // + // Note that it's NOT ALLOWED to call into this library from within this + // callback. + virtual SendPacketStatus SendPacketWithStatus( + rtc::ArrayView data) { + SendPacket(data); + return SendPacketStatus::kSuccess; + } // Called when the library wants to create a Timeout. The callback must return // an object that implements that interface. diff --git a/net/dcsctp/socket/callback_deferrer.h b/net/dcsctp/socket/callback_deferrer.h index 197cf434af..b3251c84d5 100644 --- a/net/dcsctp/socket/callback_deferrer.h +++ b/net/dcsctp/socket/callback_deferrer.h @@ -59,9 +59,10 @@ class CallbackDeferrer : public DcSctpSocketCallbacks { } } - void SendPacket(rtc::ArrayView data) override { + SendPacketStatus SendPacketWithStatus( + rtc::ArrayView data) override { // Will not be deferred - call directly. - underlying_.SendPacket(data); + return underlying_.SendPacketWithStatus(data); } std::unique_ptr CreateTimeout() override { diff --git a/net/dcsctp/socket/dcsctp_socket.cc b/net/dcsctp/socket/dcsctp_socket.cc index e2b04af363..f1aa0ecaa1 100644 --- a/net/dcsctp/socket/dcsctp_socket.cc +++ b/net/dcsctp/socket/dcsctp_socket.cc @@ -858,7 +858,7 @@ void DcSctpSocket::SendPacket(SctpPacket::Builder& builder) { packet_observer_->OnSentPacket(callbacks_.TimeMillis(), payload); } ++metrics_.tx_packets_count; - callbacks_.SendPacket(payload); + callbacks_.SendPacketWithStatus(payload); } bool DcSctpSocket::ValidateHasTCB() { diff --git a/net/dcsctp/socket/mock_context.h b/net/dcsctp/socket/mock_context.h index d86b99a20d..88e71d1b35 100644 --- a/net/dcsctp/socket/mock_context.h +++ b/net/dcsctp/socket/mock_context.h @@ -42,7 +42,7 @@ class MockContext : public Context { ON_CALL(*this, callbacks).WillByDefault(testing::ReturnRef(callbacks_)); ON_CALL(*this, current_rto).WillByDefault(testing::Return(DurationMs(123))); ON_CALL(*this, Send).WillByDefault([this](SctpPacket::Builder& builder) { - callbacks_.SendPacket(builder.Build()); + callbacks_.SendPacketWithStatus(builder.Build()); }); } diff --git a/net/dcsctp/socket/mock_dcsctp_socket_callbacks.h b/net/dcsctp/socket/mock_dcsctp_socket_callbacks.h index bcf1bde5b8..894dd9ac5a 100644 --- a/net/dcsctp/socket/mock_dcsctp_socket_callbacks.h +++ b/net/dcsctp/socket/mock_dcsctp_socket_callbacks.h @@ -56,10 +56,11 @@ class MockDcSctpSocketCallbacks : public DcSctpSocketCallbacks { : log_prefix_(name.empty() ? "" : std::string(name) + ": "), random_(internal::GetUniqueSeed()), timeout_manager_([this]() { return now_; }) { - ON_CALL(*this, SendPacket) + ON_CALL(*this, SendPacketWithStatus) .WillByDefault([this](rtc::ArrayView data) { sent_packets_.emplace_back( std::vector(data.begin(), data.end())); + return SendPacketStatus::kSuccess; }); ON_CALL(*this, OnMessageReceived) .WillByDefault([this](DcSctpMessage message) { @@ -80,8 +81,9 @@ class MockDcSctpSocketCallbacks : public DcSctpSocketCallbacks { }); ON_CALL(*this, TimeMillis).WillByDefault([this]() { return now_; }); } - MOCK_METHOD(void, - SendPacket, + + MOCK_METHOD(SendPacketStatus, + SendPacketWithStatus, (rtc::ArrayView data), (override)); diff --git a/net/dcsctp/socket/stream_reset_handler_test.cc b/net/dcsctp/socket/stream_reset_handler_test.cc index a8e96fbf20..8955c839e8 100644 --- a/net/dcsctp/socket/stream_reset_handler_test.cc +++ b/net/dcsctp/socket/stream_reset_handler_test.cc @@ -183,7 +183,7 @@ class StreamResetHandlerTest : public testing::Test { }; TEST_F(StreamResetHandlerTest, ChunkWithNoParametersReturnsError) { - EXPECT_CALL(callbacks_, SendPacket).Times(0); + EXPECT_CALL(callbacks_, SendPacketWithStatus).Times(0); EXPECT_CALL(callbacks_, OnError).Times(1); handler_.HandleReConfig(ReConfigChunk(Parameters())); } @@ -198,7 +198,7 @@ TEST_F(StreamResetHandlerTest, ChunkWithInvalidParametersReturnsError) { ReconfigRequestSN(10), kPeerInitialTsn, {StreamID(2)})); - EXPECT_CALL(callbacks_, SendPacket).Times(0); + EXPECT_CALL(callbacks_, SendPacketWithStatus).Times(0); EXPECT_CALL(callbacks_, OnError).Times(1); handler_.HandleReConfig(ReConfigChunk(builder.Build())); } @@ -375,7 +375,7 @@ TEST_F(StreamResetHandlerTest, SendOutgoingResettingOnPositiveResponse) { // Processing a response shouldn't result in sending anything. EXPECT_CALL(callbacks_, OnError).Times(0); - EXPECT_CALL(callbacks_, SendPacket).Times(0); + EXPECT_CALL(callbacks_, SendPacketWithStatus).Times(0); handler_.HandleReConfig(std::move(response_reconfig)); } @@ -401,7 +401,7 @@ TEST_F(StreamResetHandlerTest, SendOutgoingResetRollbackOnError) { // Only requests should result in sending responses. EXPECT_CALL(callbacks_, OnError).Times(0); - EXPECT_CALL(callbacks_, SendPacket).Times(0); + EXPECT_CALL(callbacks_, SendPacketWithStatus).Times(0); handler_.HandleReConfig(std::move(response_reconfig)); } @@ -430,12 +430,12 @@ TEST_F(StreamResetHandlerTest, SendOutgoingResetRetransmitOnInProgress) { // Processing a response shouldn't result in sending anything. EXPECT_CALL(callbacks_, OnError).Times(0); - EXPECT_CALL(callbacks_, SendPacket).Times(0); + EXPECT_CALL(callbacks_, SendPacketWithStatus).Times(0); handler_.HandleReConfig(std::move(response_reconfig)); // Let some time pass, so that the reconfig timer expires, and retries the // same request. - EXPECT_CALL(callbacks_, SendPacket).Times(1); + EXPECT_CALL(callbacks_, SendPacketWithStatus).Times(1); AdvanceTime(kRto); std::vector payload = callbacks_.ConsumeSentPacket(); @@ -486,7 +486,7 @@ TEST_F(StreamResetHandlerTest, ResetWhileRequestIsSentWillQueue) { // Processing a response shouldn't result in sending anything. EXPECT_CALL(callbacks_, OnError).Times(0); - EXPECT_CALL(callbacks_, SendPacket).Times(0); + EXPECT_CALL(callbacks_, SendPacketWithStatus).Times(0); handler_.HandleReConfig(std::move(response_reconfig)); // Response has been processed. A new request can be sent.