From b00d63c88ba247f54e404d9ba973edea7d0db5bd Mon Sep 17 00:00:00 2001 From: Tommi Date: Wed, 12 Apr 2023 19:49:53 +0200 Subject: [PATCH] Merge TeardownDataChannelTransport_n and OnTransportChannelClosed. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This consolidates termination logic in the DataChannelController to make shut down consistent between when the transport notifies of termination and when termination is initiated from the PC side. This removes the need for `OnTransportChannelClosed` from the PC side since we can just use TeardownDataChannelTransport_n (the two were always being called together). Bug: webrtc:11547 Change-Id: I1763f82cbfe1a3d5b8bfabb8d4cba0ee0fa95738 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/300561 Commit-Queue: Tomas Gunnarsson Reviewed-by: Henrik Boström Cr-Commit-Position: refs/heads/main@{#39838} --- pc/data_channel_controller.cc | 38 ++++++++++--------------- pc/data_channel_controller.h | 5 +--- pc/data_channel_controller_unittest.cc | 3 +- pc/peer_connection.cc | 6 ++-- pc/peer_connection.h | 3 +- pc/peer_connection_internal.h | 2 +- pc/sdp_offer_answer.cc | 10 ++----- pc/test/fake_peer_connection_base.h | 2 +- pc/test/mock_peer_connection_internal.h | 2 +- 9 files changed, 28 insertions(+), 43 deletions(-) diff --git a/pc/data_channel_controller.cc b/pc/data_channel_controller.cc index 3545546da6..a4c8d8852b 100644 --- a/pc/data_channel_controller.cc +++ b/pc/data_channel_controller.cc @@ -146,12 +146,17 @@ void DataChannelController::OnReadyToSend() { void DataChannelController::OnTransportClosed(RTCError error) { RTC_DCHECK_RUN_ON(network_thread()); + // This loop will close all data channels and trigger a callback to - // `OnSctpDataChannelClosed` which will modify `sctp_data_channels_n_`, so - // we create a local copy while we do the fan-out. - auto copy = sctp_data_channels_n_; - for (const auto& channel : copy) + // `OnSctpDataChannelClosed`. We'll empty `sctp_data_channels_n_`, first + // and `OnSctpDataChannelClosed` will become a noop but we'll release the + // StreamId here. + std::vector> temp_sctp_dcs; + temp_sctp_dcs.swap(sctp_data_channels_n_); + for (const auto& channel : temp_sctp_dcs) { channel->OnTransportChannelClosed(error); + sid_allocator_.ReleaseSid(channel->sid_n()); + } } void DataChannelController::SetupDataChannelTransport_n() { @@ -168,13 +173,17 @@ void DataChannelController::PrepareForShutdown() { signaling_safety_.reset(PendingTaskSafetyFlag::CreateDetachedInactive()); } -void DataChannelController::TeardownDataChannelTransport_n() { +void DataChannelController::TeardownDataChannelTransport_n(RTCError error) { RTC_DCHECK_RUN_ON(network_thread()); + + OnTransportClosed(error); + if (data_channel_transport_) { data_channel_transport_->SetDataSink(nullptr); set_data_channel_transport(nullptr); } - sctp_data_channels_n_.clear(); + + RTC_DCHECK(sctp_data_channels_n_.empty()); weak_factory_.InvalidateWeakPtrs(); } @@ -404,23 +413,6 @@ void DataChannelController::OnSctpDataChannelClosed(SctpDataChannel* channel) { sctp_data_channels_n_.erase(it); } -void DataChannelController::OnTransportChannelClosed(RTCError error) { - RTC_DCHECK_RUN_ON(network_thread()); - // Use a temporary copy of the SCTP DataChannel list because the - // DataChannel may callback to us and try to modify the list. - // TODO(tommi): `OnTransportChannelClosed` is called from - // `SdpOfferAnswerHandler::DestroyDataChannelTransport` just before - // `TeardownDataChannelTransport_n` is called (but on the network thread) from - // the same function. We can now get rid of this function - // (OnTransportChannelClosed) and run this loop from within the - // TeardownDataChannelTransport_n callback. - std::vector> temp_sctp_dcs; - temp_sctp_dcs.swap(sctp_data_channels_n_); - for (const auto& channel : temp_sctp_dcs) { - channel->OnTransportChannelClosed(error); - } -} - void DataChannelController::set_data_channel_transport( DataChannelTransportInterface* transport) { RTC_DCHECK_RUN_ON(network_thread()); diff --git a/pc/data_channel_controller.h b/pc/data_channel_controller.h index edd21c2390..9e6af3fa5b 100644 --- a/pc/data_channel_controller.h +++ b/pc/data_channel_controller.h @@ -70,7 +70,7 @@ class DataChannelController : public SctpDataChannelControllerInterface, // Called from PeerConnection::SetupDataChannelTransport_n void SetupDataChannelTransport_n(); // Called from PeerConnection::TeardownDataChannelTransport_n - void TeardownDataChannelTransport_n(); + void TeardownDataChannelTransport_n(RTCError error); // Called from PeerConnection::OnTransportChanged // to make required changes to datachannels' transports. @@ -96,9 +96,6 @@ class DataChannelController : public SctpDataChannelControllerInterface, // Accessors void set_data_channel_transport(DataChannelTransportInterface* transport); - // Called when the transport for the data channels is closed or destroyed. - void OnTransportChannelClosed(RTCError error); - void OnSctpDataChannelClosed(SctpDataChannel* channel); protected: diff --git a/pc/data_channel_controller_unittest.cc b/pc/data_channel_controller_unittest.cc index fd3deae0d7..a2a9f00cd3 100644 --- a/pc/data_channel_controller_unittest.cc +++ b/pc/data_channel_controller_unittest.cc @@ -55,7 +55,8 @@ class DataChannelControllerForTest : public DataChannelController { : DataChannelController(pc) {} ~DataChannelControllerForTest() override { - network_thread()->BlockingCall([&] { TeardownDataChannelTransport_n(); }); + network_thread()->BlockingCall( + [&] { TeardownDataChannelTransport_n(RTCError::OK()); }); PrepareForShutdown(); } }; diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc index 82c5914a52..f76ce6396d 100644 --- a/pc/peer_connection.cc +++ b/pc/peer_connection.cc @@ -573,7 +573,7 @@ PeerConnection::~PeerConnection() { transport_controller_copy_ = nullptr; network_thread()->BlockingCall([this] { RTC_DCHECK_RUN_ON(network_thread()); - TeardownDataChannelTransport_n(); + TeardownDataChannelTransport_n(RTCError::OK()); transport_controller_.reset(); port_allocator_.reset(); if (network_thread_safety_) @@ -2544,7 +2544,7 @@ bool PeerConnection::SetupDataChannelTransport_n(const std::string& mid) { return true; } -void PeerConnection::TeardownDataChannelTransport_n() { +void PeerConnection::TeardownDataChannelTransport_n(RTCError error) { if (sctp_mid_n_) { // `sctp_mid_` may still be active through an SCTP transport. If not, unset // it. @@ -2553,7 +2553,7 @@ void PeerConnection::TeardownDataChannelTransport_n() { sctp_mid_n_.reset(); } - data_channel_controller_.TeardownDataChannelTransport_n(); + data_channel_controller_.TeardownDataChannelTransport_n(error); } // Returns false if bundle is enabled and rtcp_mux is disabled. diff --git a/pc/peer_connection.h b/pc/peer_connection.h index 950758be07..e7cfb9a5c1 100644 --- a/pc/peer_connection.h +++ b/pc/peer_connection.h @@ -434,7 +434,8 @@ class PeerConnection : public PeerConnectionInternal, bool SetupDataChannelTransport_n(const std::string& mid) override RTC_RUN_ON(network_thread()); - void TeardownDataChannelTransport_n() override RTC_RUN_ON(network_thread()); + void TeardownDataChannelTransport_n(RTCError error) override + RTC_RUN_ON(network_thread()); const FieldTrialsView& trials() const override { return *trials_; } diff --git a/pc/peer_connection_internal.h b/pc/peer_connection_internal.h index abf5825bd6..ab9789d3ab 100644 --- a/pc/peer_connection_internal.h +++ b/pc/peer_connection_internal.h @@ -118,7 +118,7 @@ class PeerConnectionSdpMethods { // this session. virtual bool SrtpRequired() const = 0; virtual bool SetupDataChannelTransport_n(const std::string& mid) = 0; - virtual void TeardownDataChannelTransport_n() = 0; + virtual void TeardownDataChannelTransport_n(RTCError error) = 0; virtual void SetSctpDataMid(const std::string& mid) = 0; virtual void ResetSctpDataMid() = 0; diff --git a/pc/sdp_offer_answer.cc b/pc/sdp_offer_answer.cc index 0363042687..4a86ba80b7 100644 --- a/pc/sdp_offer_answer.cc +++ b/pc/sdp_offer_answer.cc @@ -5110,18 +5110,12 @@ bool SdpOfferAnswerHandler::CreateDataChannel(const std::string& mid) { void SdpOfferAnswerHandler::DestroyDataChannelTransport(RTCError error) { RTC_DCHECK_RUN_ON(signaling_thread()); - const bool has_sctp = pc_->sctp_mid().has_value(); - context_->network_thread()->BlockingCall( [&, data_channel_controller = data_channel_controller()] { RTC_DCHECK_RUN_ON(context_->network_thread()); - if (has_sctp) - data_channel_controller->OnTransportChannelClosed(error); - pc_->TeardownDataChannelTransport_n(); + pc_->TeardownDataChannelTransport_n(error); }); - - if (has_sctp) - pc_->ResetSctpDataMid(); + pc_->ResetSctpDataMid(); } void SdpOfferAnswerHandler::DestroyAllChannels() { diff --git a/pc/test/fake_peer_connection_base.h b/pc/test/fake_peer_connection_base.h index 0508673d36..3178770f7c 100644 --- a/pc/test/fake_peer_connection_base.h +++ b/pc/test/fake_peer_connection_base.h @@ -361,7 +361,7 @@ class FakePeerConnectionBase : public PeerConnectionInternal { bool SetupDataChannelTransport_n(const std::string& mid) override { return false; } - void TeardownDataChannelTransport_n() override {} + void TeardownDataChannelTransport_n(RTCError error) override {} void SetSctpDataMid(const std::string& mid) override {} void ResetSctpDataMid() override {} diff --git a/pc/test/mock_peer_connection_internal.h b/pc/test/mock_peer_connection_internal.h index 4647567559..b8107a3d5b 100644 --- a/pc/test/mock_peer_connection_internal.h +++ b/pc/test/mock_peer_connection_internal.h @@ -267,7 +267,7 @@ class MockPeerConnectionInternal : public PeerConnectionInternal { SetupDataChannelTransport_n, (const std::string&), (override)); - MOCK_METHOD(void, TeardownDataChannelTransport_n, (), (override)); + MOCK_METHOD(void, TeardownDataChannelTransport_n, (RTCError), (override)); MOCK_METHOD(void, SetSctpDataMid, (const std::string&), (override)); MOCK_METHOD(void, ResetSctpDataMid, (), (override)); MOCK_METHOD(const FieldTrialsView&, trials, (), (const, override));