From 1d26fd33caa35125ed1ed8dc313c570851b2b179 Mon Sep 17 00:00:00 2001 From: Tommi Date: Mon, 18 Mar 2024 17:57:52 +0100 Subject: [PATCH] Replace SignalClosed sigslot with absl::AnyInvocable This restricts the interface such that only a single onclose handler can be set and that only one OnClose() notification will be fired. That behavior is the same as how the previous sigslot was being used, but the difference is that, in addition to removing sigslot, this pattern is now more explicitly checked in the design. Bug: webrtc:11943 Change-Id: I469c8cab3d62544988c8145b326af60b06b76d8e Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/343340 Commit-Queue: Tomas Gunnarsson Reviewed-by: Danil Chapovalov Cr-Commit-Position: refs/heads/main@{#41920} --- media/sctp/dcsctp_transport.cc | 19 ++++++++----------- media/sctp/dcsctp_transport.h | 2 -- p2p/base/dtls_transport.cc | 4 ++-- p2p/base/fake_packet_transport.h | 1 + p2p/base/packet_transport_internal.cc | 15 +++++++++++++++ p2p/base/packet_transport_internal.h | 4 +++- .../packet_transport_internal_unittest.cc | 11 +++++++++++ 7 files changed, 40 insertions(+), 16 deletions(-) diff --git a/media/sctp/dcsctp_transport.cc b/media/sctp/dcsctp_transport.cc index bff9c29d17..52890b3bd2 100644 --- a/media/sctp/dcsctp_transport.cc +++ b/media/sctp/dcsctp_transport.cc @@ -617,7 +617,13 @@ void DcSctpTransport::ConnectTransportSignals() { const rtc::ReceivedPacket& packet) { OnTransportReadPacket(transport, packet); }); - transport_->SignalClosed.connect(this, &DcSctpTransport::OnTransportClosed); + transport_->SetOnCloseCallback([this]() { + RTC_DCHECK_RUN_ON(network_thread_); + RTC_DLOG(LS_VERBOSE) << debug_name_ << "->OnTransportClosed()."; + if (data_channel_sink_) { + data_channel_sink_->OnTransportClosed({}); + } + }); } void DcSctpTransport::DisconnectTransportSignals() { @@ -627,7 +633,7 @@ void DcSctpTransport::DisconnectTransportSignals() { } transport_->SignalWritableState.disconnect(this); transport_->DeregisterReceivedPacketCallback(this); - transport_->SignalClosed.disconnect(this); + transport_->SetOnCloseCallback(nullptr); } void DcSctpTransport::OnTransportWritableState( @@ -656,15 +662,6 @@ void DcSctpTransport::OnTransportReadPacket( } } -void DcSctpTransport::OnTransportClosed( - rtc::PacketTransportInternal* transport) { - RTC_DCHECK_RUN_ON(network_thread_); - RTC_DLOG(LS_VERBOSE) << debug_name_ << "->OnTransportClosed()."; - if (data_channel_sink_) { - data_channel_sink_->OnTransportClosed({}); - } -} - void DcSctpTransport::MaybeConnectSocket() { if (transport_ && transport_->writable() && socket_ && socket_->state() == dcsctp::SocketState::kClosed) { diff --git a/media/sctp/dcsctp_transport.h b/media/sctp/dcsctp_transport.h index aa301d8496..21a6a9513f 100644 --- a/media/sctp/dcsctp_transport.h +++ b/media/sctp/dcsctp_transport.h @@ -98,8 +98,6 @@ class DcSctpTransport : public cricket::SctpTransportInternal, void OnTransportWritableState(rtc::PacketTransportInternal* transport); void OnTransportReadPacket(rtc::PacketTransportInternal* transport, const rtc::ReceivedPacket& packet); - void OnTransportClosed(rtc::PacketTransportInternal* transport); - void MaybeConnectSocket(); rtc::Thread* network_thread_; diff --git a/p2p/base/dtls_transport.cc b/p2p/base/dtls_transport.cc index 7547863bd6..42353e2c22 100644 --- a/p2p/base/dtls_transport.cc +++ b/p2p/base/dtls_transport.cc @@ -733,7 +733,7 @@ void DtlsTransport::OnDtlsEvent(rtc::StreamInterface* dtls, int sig, int err) { RTC_LOG(LS_INFO) << ToString() << ": DTLS transport closed by remote"; set_writable(false); set_dtls_state(webrtc::DtlsTransportState::kClosed); - SignalClosed(this); + NotifyOnClose(); } else if (ret == rtc::SR_ERROR) { // Remote peer shut down the association with an error. RTC_LOG(LS_INFO) @@ -742,7 +742,7 @@ void DtlsTransport::OnDtlsEvent(rtc::StreamInterface* dtls, int sig, int err) { << read_error; set_writable(false); set_dtls_state(webrtc::DtlsTransportState::kFailed); - SignalClosed(this); + NotifyOnClose(); } } while (ret == rtc::SR_SUCCESS); } diff --git a/p2p/base/fake_packet_transport.h b/p2p/base/fake_packet_transport.h index 48fb088170..df6c62ac1f 100644 --- a/p2p/base/fake_packet_transport.h +++ b/p2p/base/fake_packet_transport.h @@ -99,6 +99,7 @@ class FakePacketTransport : public PacketTransportInternal { SignalNetworkRouteChanged(network_route); } + using PacketTransportInternal::NotifyOnClose; using PacketTransportInternal::NotifyPacketReceived; private: diff --git a/p2p/base/packet_transport_internal.cc b/p2p/base/packet_transport_internal.cc index 4d82501b71..36f797a93f 100644 --- a/p2p/base/packet_transport_internal.cc +++ b/p2p/base/packet_transport_internal.cc @@ -40,10 +40,25 @@ void PacketTransportInternal::DeregisterReceivedPacketCallback(void* id) { received_packet_callback_list_.RemoveReceivers(id); } +void PacketTransportInternal::SetOnCloseCallback( + absl::AnyInvocable callback) { + RTC_DCHECK_RUN_ON(&network_checker_); + RTC_DCHECK(!on_close_ || !callback); + on_close_ = std::move(callback); +} + void PacketTransportInternal::NotifyPacketReceived( const rtc::ReceivedPacket& packet) { RTC_DCHECK_RUN_ON(&network_checker_); received_packet_callback_list_.Send(this, packet); } +void PacketTransportInternal::NotifyOnClose() { + RTC_DCHECK_RUN_ON(&network_checker_); + if (on_close_) { + std::move(on_close_)(); + on_close_ = nullptr; + } +} + } // namespace rtc diff --git a/p2p/base/packet_transport_internal.h b/p2p/base/packet_transport_internal.h index 98c37ab385..54d7f07584 100644 --- a/p2p/base/packet_transport_internal.h +++ b/p2p/base/packet_transport_internal.h @@ -98,19 +98,21 @@ class RTC_EXPORT PacketTransportInternal : public sigslot::has_slots<> { sigslot::signal1> SignalNetworkRouteChanged; // Signalled when the transport is closed. - sigslot::signal1 SignalClosed; + void SetOnCloseCallback(absl::AnyInvocable callback); protected: PacketTransportInternal(); ~PacketTransportInternal() override; void NotifyPacketReceived(const rtc::ReceivedPacket& packet); + void NotifyOnClose(); webrtc::SequenceChecker network_checker_{webrtc::SequenceChecker::kDetached}; private: webrtc::CallbackList received_packet_callback_list_ RTC_GUARDED_BY(&network_checker_); + absl::AnyInvocable on_close_; }; } // namespace rtc diff --git a/p2p/base/packet_transport_internal_unittest.cc b/p2p/base/packet_transport_internal_unittest.cc index b8e589a852..25492ea00b 100644 --- a/p2p/base/packet_transport_internal_unittest.cc +++ b/p2p/base/packet_transport_internal_unittest.cc @@ -41,4 +41,15 @@ TEST(PacketTransportInternal, packet_transport.DeregisterReceivedPacketCallback(&receiver); } +TEST(PacketTransportInternal, NotifiesOnceOnClose) { + rtc::FakePacketTransport packet_transport("test"); + int call_count = 0; + packet_transport.SetOnCloseCallback([&]() { ++call_count; }); + ASSERT_EQ(call_count, 0); + packet_transport.NotifyOnClose(); + EXPECT_EQ(call_count, 1); + packet_transport.NotifyOnClose(); + EXPECT_EQ(call_count, 1); // Call count should not have increased. +} + } // namespace