Merge TeardownDataChannelTransport_n and OnTransportChannelClosed.
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 <tommi@webrtc.org>
Reviewed-by: Henrik Boström <hbos@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#39838}
diff --git a/pc/data_channel_controller.cc b/pc/data_channel_controller.cc
index 3545546..a4c8d88 100644
--- a/pc/data_channel_controller.cc
+++ b/pc/data_channel_controller.cc
@@ -146,12 +146,17 @@
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<rtc::scoped_refptr<SctpDataChannel>> 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 @@
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 @@
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<rtc::scoped_refptr<SctpDataChannel>> 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 edd21c2..9e6af3f 100644
--- a/pc/data_channel_controller.h
+++ b/pc/data_channel_controller.h
@@ -70,7 +70,7 @@
// 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 @@
// 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 fd3deae..a2a9f00 100644
--- a/pc/data_channel_controller_unittest.cc
+++ b/pc/data_channel_controller_unittest.cc
@@ -55,7 +55,8 @@
: 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 82c5914..f76ce63 100644
--- a/pc/peer_connection.cc
+++ b/pc/peer_connection.cc
@@ -573,7 +573,7 @@
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 @@
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 @@
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 950758b..e7cfb9a 100644
--- a/pc/peer_connection.h
+++ b/pc/peer_connection.h
@@ -434,7 +434,8 @@
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 abf5825..ab9789d 100644
--- a/pc/peer_connection_internal.h
+++ b/pc/peer_connection_internal.h
@@ -118,7 +118,7 @@
// 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 0363042..4a86ba8 100644
--- a/pc/sdp_offer_answer.cc
+++ b/pc/sdp_offer_answer.cc
@@ -5110,18 +5110,12 @@
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 0508673..3178770 100644
--- a/pc/test/fake_peer_connection_base.h
+++ b/pc/test/fake_peer_connection_base.h
@@ -361,7 +361,7 @@
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 4647567..b8107a3 100644
--- a/pc/test/mock_peer_connection_internal.h
+++ b/pc/test/mock_peer_connection_internal.h
@@ -267,7 +267,7 @@
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));
