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 <tommi@webrtc.org>
Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#41920}
diff --git a/media/sctp/dcsctp_transport.cc b/media/sctp/dcsctp_transport.cc
index bff9c29..52890b3 100644
--- a/media/sctp/dcsctp_transport.cc
+++ b/media/sctp/dcsctp_transport.cc
@@ -617,7 +617,13 @@
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 @@
}
transport_->SignalWritableState.disconnect(this);
transport_->DeregisterReceivedPacketCallback(this);
- transport_->SignalClosed.disconnect(this);
+ transport_->SetOnCloseCallback(nullptr);
}
void DcSctpTransport::OnTransportWritableState(
@@ -656,15 +662,6 @@
}
}
-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 aa301d8..21a6a95 100644
--- a/media/sctp/dcsctp_transport.h
+++ b/media/sctp/dcsctp_transport.h
@@ -98,8 +98,6 @@
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 7547863..42353e2 100644
--- a/p2p/base/dtls_transport.cc
+++ b/p2p/base/dtls_transport.cc
@@ -733,7 +733,7 @@
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 @@
<< 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 48fb088..df6c62a 100644
--- a/p2p/base/fake_packet_transport.h
+++ b/p2p/base/fake_packet_transport.h
@@ -99,6 +99,7 @@
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 4d82501..36f797a 100644
--- a/p2p/base/packet_transport_internal.cc
+++ b/p2p/base/packet_transport_internal.cc
@@ -40,10 +40,25 @@
received_packet_callback_list_.RemoveReceivers(id);
}
+void PacketTransportInternal::SetOnCloseCallback(
+ absl::AnyInvocable<void() &&> 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 98c37ab..54d7f07 100644
--- a/p2p/base/packet_transport_internal.h
+++ b/p2p/base/packet_transport_internal.h
@@ -98,19 +98,21 @@
sigslot::signal1<absl::optional<rtc::NetworkRoute>> SignalNetworkRouteChanged;
// Signalled when the transport is closed.
- sigslot::signal1<PacketTransportInternal*> SignalClosed;
+ void SetOnCloseCallback(absl::AnyInvocable<void() &&> callback);
protected:
PacketTransportInternal();
~PacketTransportInternal() override;
void NotifyPacketReceived(const rtc::ReceivedPacket& packet);
+ void NotifyOnClose();
webrtc::SequenceChecker network_checker_{webrtc::SequenceChecker::kDetached};
private:
webrtc::CallbackList<PacketTransportInternal*, const rtc::ReceivedPacket&>
received_packet_callback_list_ RTC_GUARDED_BY(&network_checker_);
+ absl::AnyInvocable<void() &&> on_close_;
};
} // namespace rtc
diff --git a/p2p/base/packet_transport_internal_unittest.cc b/p2p/base/packet_transport_internal_unittest.cc
index b8e589a..25492ea 100644
--- a/p2p/base/packet_transport_internal_unittest.cc
+++ b/p2p/base/packet_transport_internal_unittest.cc
@@ -41,4 +41,15 @@
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
