Remove SignalDataChannelTransportChannelClosed_s
This removes one sigslot and also simplifies the teardown procedure
of a data channel when the channel is closed by the transport.
In this case we no longer need an additional async teardown task that
releases the last remaining reference to the channel.
Bug: webrtc:11943, webrtc:11547
Change-Id: I1c170349a6cbb3cb3c5a47d284e3a3d416c92b11
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/296981
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#39551}
diff --git a/pc/data_channel_controller.cc b/pc/data_channel_controller.cc
index 0ebfdfa..1cad4ea 100644
--- a/pc/data_channel_controller.cc
+++ b/pc/data_channel_controller.cc
@@ -12,6 +12,7 @@
#include <utility>
+#include "absl/algorithm/container.h"
#include "api/peer_connection_interface.h"
#include "api/rtc_error.h"
#include "pc/peer_connection_internal.h"
@@ -52,8 +53,6 @@
webrtc_data_channel, &SctpDataChannel::OnDataReceived);
SignalDataChannelTransportChannelClosing_s.connect(
webrtc_data_channel, &SctpDataChannel::OnClosingProcedureStartedRemotely);
- SignalDataChannelTransportChannelClosed_s.connect(
- webrtc_data_channel, &SctpDataChannel::OnClosingProcedureComplete);
return true;
}
@@ -68,7 +67,6 @@
SignalDataChannelTransportWritable_s.disconnect(webrtc_data_channel);
SignalDataChannelTransportReceivedData_s.disconnect(webrtc_data_channel);
SignalDataChannelTransportChannelClosing_s.disconnect(webrtc_data_channel);
- SignalDataChannelTransportChannelClosed_s.disconnect(webrtc_data_channel);
}
void DataChannelController::AddSctpDataStream(int sid) {
@@ -143,7 +141,22 @@
signaling_thread()->PostTask(
SafeTask(signaling_safety_.flag(), [this, channel_id] {
RTC_DCHECK_RUN_ON(signaling_thread());
- SignalDataChannelTransportChannelClosed_s(channel_id);
+ auto it = absl::c_find_if(sctp_data_channels_, [&](const auto& c) {
+ return c->id() == channel_id;
+ });
+
+ // Remove the channel from our list, close it and free up resources.
+ if (it != sctp_data_channels_.end()) {
+ rtc::scoped_refptr<SctpDataChannel> channel = std::move(*it);
+ // Note: this causes OnSctpDataChannelClosed() to not do anything
+ // when called from within `OnClosingProcedureComplete`.
+ sctp_data_channels_.erase(it);
+
+ DisconnectDataChannel(channel.get());
+ sid_allocator_.ReleaseSid(channel->sid());
+
+ channel->OnClosingProcedureComplete();
+ }
}));
}
diff --git a/pc/data_channel_controller.h b/pc/data_channel_controller.h
index dcb6366..5fd0afb 100644
--- a/pc/data_channel_controller.h
+++ b/pc/data_channel_controller.h
@@ -163,8 +163,6 @@
RTC_GUARDED_BY(signaling_thread());
sigslot::signal1<int> SignalDataChannelTransportChannelClosing_s
RTC_GUARDED_BY(signaling_thread());
- sigslot::signal1<int> SignalDataChannelTransportChannelClosed_s
- RTC_GUARDED_BY(signaling_thread());
// Owning PeerConnection.
PeerConnectionInternal* const pc_;
diff --git a/pc/data_channel_unittest.cc b/pc/data_channel_unittest.cc
index 973c694..529cb6b 100644
--- a/pc/data_channel_unittest.cc
+++ b/pc/data_channel_unittest.cc
@@ -162,20 +162,21 @@
// Tests the state of the data channel.
TEST_F(SctpDataChannelTest, StateTransition) {
+ AddObserver();
+
EXPECT_EQ(DataChannelInterface::kConnecting, webrtc_data_channel_->state());
- EXPECT_EQ(controller_->channels_opened(), 0);
- EXPECT_EQ(controller_->channels_closed(), 0);
+ EXPECT_EQ(observer_->on_state_change_count(), 0u);
SetChannelReady();
EXPECT_EQ(DataChannelInterface::kOpen, webrtc_data_channel_->state());
- EXPECT_EQ(controller_->channels_opened(), 1);
- EXPECT_EQ(controller_->channels_closed(), 0);
+ EXPECT_EQ(observer_->on_state_change_count(), 1u);
+ // `Close()` should trigger two state changes, first `kClosing`, then
+ // `kClose`.
webrtc_data_channel_->Close();
EXPECT_EQ(DataChannelInterface::kClosed, webrtc_data_channel_->state());
+ EXPECT_EQ(observer_->on_state_change_count(), 3u);
EXPECT_TRUE(webrtc_data_channel_->error().ok());
- EXPECT_EQ(controller_->channels_opened(), 1);
- EXPECT_EQ(controller_->channels_closed(), 1);
// Verifies that it's disconnected from the transport.
EXPECT_FALSE(controller_->IsConnected(webrtc_data_channel_.get()));
}
diff --git a/pc/sctp_data_channel.cc b/pc/sctp_data_channel.cc
index 81668dd..be2a480 100644
--- a/pc/sctp_data_channel.cc
+++ b/pc/sctp_data_channel.cc
@@ -345,11 +345,7 @@
// If the queue is non-empty, we're waiting for SignalReadyToSend,
// so just add to the end of the queue and keep waiting.
if (!queued_send_data_.Empty()) {
- if (!QueueSendDataMessage(buffer)) {
- // Queue is full
- return false;
- }
- return true;
+ return QueueSendDataMessage(buffer);
}
SendDataMessage(buffer, true);
@@ -389,16 +385,14 @@
}
}
-void SctpDataChannel::OnClosingProcedureComplete(int sid) {
+void SctpDataChannel::OnClosingProcedureComplete() {
RTC_DCHECK_RUN_ON(signaling_thread_);
- if (id_.stream_id_int() == sid) {
- // If the closing procedure is complete, we should have finished sending
- // all pending data and transitioned to kClosing already.
- RTC_DCHECK_EQ(state_, kClosing);
- RTC_DCHECK(queued_send_data_.Empty());
- DisconnectFromTransport();
- SetState(kClosed);
- }
+ // If the closing procedure is complete, we should have finished sending
+ // all pending data and transitioned to kClosing already.
+ RTC_DCHECK_EQ(state_, kClosing);
+ RTC_DCHECK(queued_send_data_.Empty());
+
+ SetState(kClosed);
}
void SctpDataChannel::OnTransportChannelCreated() {
@@ -515,9 +509,7 @@
return;
}
- if (connected_to_transport_) {
- DisconnectFromTransport();
- }
+ DisconnectFromTransport();
// Closing abruptly means any queued data gets thrown away.
queued_send_data_.Clear();
diff --git a/pc/sctp_data_channel.h b/pc/sctp_data_channel.h
index 7fa7173..17cad26 100644
--- a/pc/sctp_data_channel.h
+++ b/pc/sctp_data_channel.h
@@ -194,7 +194,7 @@
// The closing procedure is complete; both incoming and outgoing stream
// resets are done and the channel can transition to kClosed. Called
// asynchronously after RemoveSctpDataStream.
- void OnClosingProcedureComplete(int sid);
+ void OnClosingProcedureComplete();
// Called when the transport channel is created.
// Only needs to be called for SCTP data channels.
void OnTransportChannelCreated();
diff --git a/pc/test/fake_data_channel_controller.h b/pc/test/fake_data_channel_controller.h
index f7a2afe..b0c16b7 100644
--- a/pc/test/fake_data_channel_controller.h
+++ b/pc/test/fake_data_channel_controller.h
@@ -87,8 +87,11 @@
// instantly, doing the same snapshot thing as below.
for (webrtc::SctpDataChannel* ch : std::set<webrtc::SctpDataChannel*>(
connected_channels_.begin(), connected_channels_.end())) {
- if (connected_channels_.count(ch)) {
- ch->OnClosingProcedureComplete(sid);
+ if (connected_channels_.count(ch) && ch->id() == sid) {
+ // This path mimics the DCC's OnChannelClosed handler since the FDCC
+ // (this class) doesn't have a transport that would do that.
+ DisconnectDataChannel(ch);
+ ch->OnClosingProcedureComplete();
}
}
}
