Revert "Make deletion of Connection objects more deterministic."
This reverts commit 942cac2e9e6a205fd673dc003a051cfb3867f2e3.
Reason for revert: Reverting while downstream updates are made.
Original change's description:
> Make deletion of Connection objects more deterministic.
>
> This changes most deletion paths of Connection objects to go through
> the owner class of the Connection instances, Port.
>
> In situations where Connection objects still need to be deleted
> asynchronously, `async = true` can be passed to
> `Port::DestroyConnection` and get the same behavior as
> `Connection::Destroy` formerly gave.
>
> The `Destroy()` method still exists for downstream compatibility, but
> instead of deleting connection objects asynchronously, the deletion
> now happens synchronously via the Port class.
>
> Bug: webrtc:13892, webrtc:13865
> Change-Id: I07edb7bb5e5d93b33542581b4b09def548de9e12
> Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/259826
> Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
> Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
> Cr-Commit-Position: refs/heads/main@{#36676}
Bug: webrtc:13892, webrtc:13865
Change-Id: I37a15692c8201716402ba5c10f249e4d3754ce4c
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/260862
Bot-Commit: rubber-stamper@appspot.gserviceaccount.com <rubber-stamper@appspot.gserviceaccount.com>
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#36736}
diff --git a/p2p/base/connection.cc b/p2p/base/connection.cc
index 04da6bd..25a4d01 100644
--- a/p2p/base/connection.cc
+++ b/p2p/base/connection.cc
@@ -18,6 +18,7 @@
#include <vector>
#include "absl/algorithm/container.h"
+#include "absl/memory/memory.h"
#include "absl/strings/match.h"
#include "p2p/base/port_allocator.h"
#include "rtc_base/checks.h"
@@ -320,7 +321,6 @@
Connection::~Connection() {
RTC_DCHECK_RUN_ON(network_thread_);
- RTC_DCHECK(!port_);
}
webrtc::TaskQueueBase* Connection::network_thread() const {
@@ -832,14 +832,8 @@
void Connection::Destroy() {
RTC_DCHECK_RUN_ON(network_thread_);
- if (port_)
- port_->DestroyConnection(this);
-}
-
-bool Connection::Shutdown() {
- RTC_DCHECK_RUN_ON(network_thread_);
if (!port_)
- return false; // already shut down.
+ return;
RTC_DLOG(LS_VERBOSE) << ToString() << ": Connection destroyed";
@@ -856,7 +850,20 @@
// information required for logging needs access to `port_`.
port_.reset();
- return true;
+ // Unwind the stack before deleting the object in case upstream callers
+ // need to refer to the Connection's state as part of teardown.
+ // NOTE: We move ownership of 'this' into the capture section of the lambda
+ // so that the object will always be deleted, including if PostTask fails.
+ // In such a case (only tests), deletion would happen inside of the call
+ // to `Destroy()`.
+ network_thread_->PostTask(
+ webrtc::ToQueuedTask([me = absl::WrapUnique(this)]() {}));
+}
+
+void Connection::FailAndDestroy() {
+ RTC_DCHECK_RUN_ON(network_thread_);
+ set_state(IceCandidatePairState::FAILED);
+ Destroy();
}
void Connection::FailAndPrune() {
@@ -866,9 +873,9 @@
// and Connection. In some cases (Port dtor), a Connection object is deleted
// without using the `Destroy` method (port_ won't be nulled and some
// functionality won't run as expected), while in other cases
- // the Connection object is deleted asynchronously and in that case `port_`
- // will be nulled.
- // In such a case, there's a chance that the Port object gets
+ // (AddOrReplaceConnection), the Connection object is deleted asynchronously
+ // via the `Destroy` method and in that case `port_` will be nulled.
+ // However, in that case, there's a chance that the Port object gets
// deleted before the Connection object ends up being deleted.
if (!port_)
return;
@@ -968,7 +975,7 @@
// Update the receiving state.
UpdateReceiving(now);
if (dead(now)) {
- port_->DestroyConnectionAsync(this);
+ Destroy();
}
}
@@ -1386,8 +1393,7 @@
RTC_LOG(LS_ERROR) << ToString()
<< ": Received STUN error response, code=" << error_code
<< "; killing connection";
- set_state(IceCandidatePairState::FAILED);
- port_->DestroyConnectionAsync(this);
+ FailAndDestroy();
}
}
diff --git a/p2p/base/connection.h b/p2p/base/connection.h
index d4232cd..8d22da2 100644
--- a/p2p/base/connection.h
+++ b/p2p/base/connection.h
@@ -188,18 +188,11 @@
int receiving_timeout() const;
void set_receiving_timeout(absl::optional<int> receiving_timeout_ms);
- // The preferred way of destroying a `Connection` instance is by calling
- // the `DestroyConnection` method in `Port`. This method is provided
- // for some external callers that still need it. Basically just forwards
- // the call to port_.
- [[deprecated]] void Destroy();
+ // Makes the connection go away.
+ void Destroy();
- // Signals object destruction, releases outstanding references and performs
- // final logging.
- // The function will return `true` when shutdown was performed, signals
- // emitted and outstanding references released. If the function returns
- // `false`, `Shutdown()` has previously been called.
- bool Shutdown();
+ // Makes the connection go away, in a failed state.
+ void FailAndDestroy();
// Prunes the connection and sets its state to STATE_FAILED,
// It will not be used or send pings although it can still receive packets.
diff --git a/p2p/base/p2p_transport_channel.cc b/p2p/base/p2p_transport_channel.cc
index 1bdda7b..1daec12 100644
--- a/p2p/base/p2p_transport_channel.cc
+++ b/p2p/base/p2p_transport_channel.cc
@@ -222,10 +222,9 @@
TRACE_EVENT0("webrtc", "P2PTransportChannel::~P2PTransportChannel");
RTC_DCHECK_RUN_ON(network_thread_);
std::vector<Connection*> copy(connections().begin(), connections().end());
- for (Connection* connection : copy) {
- connection->SignalDestroyed.disconnect(this);
- ice_controller_->OnConnectionDestroyed(connection);
- connection->port()->DestroyConnection(connection);
+ for (Connection* con : copy) {
+ con->SignalDestroyed.disconnect(this);
+ con->Destroy();
}
resolvers_.clear();
}
@@ -1696,7 +1695,7 @@
RTC_DCHECK(!FindConnection(connection));
if (selected_connection_ == connection)
selected_connection_ = nullptr;
- connection->port()->DestroyConnection(connection);
+ connection->Destroy();
}
// Monitor connection states.
@@ -2041,7 +2040,7 @@
}
connection->SignalDestroyed.disconnect(this);
ice_controller_->OnConnectionDestroyed(connection);
- connection->port()->DestroyConnection(connection);
+ connection->Destroy();
}
if (update_selected_connection)
diff --git a/p2p/base/port.cc b/p2p/base/port.cc
index a8b4e8a..3bd09dc 100644
--- a/p2p/base/port.cc
+++ b/p2p/base/port.cc
@@ -18,7 +18,6 @@
#include <vector>
#include "absl/algorithm/container.h"
-#include "absl/memory/memory.h"
#include "absl/strings/match.h"
#include "absl/strings/string_view.h"
#include "p2p/base/connection.h"
@@ -187,7 +186,22 @@
Port::~Port() {
RTC_DCHECK_RUN_ON(thread_);
CancelPendingTasks();
- DestroyAllConnections();
+
+ // Delete all of the remaining connections. We copy the list up front
+ // because each deletion will cause it to be modified.
+
+ std::vector<Connection*> list;
+
+ AddressMap::iterator iter = connections_.begin();
+ while (iter != connections_.end()) {
+ list.push_back(iter->second);
+ ++iter;
+ }
+
+ for (uint32_t i = 0; i < list.size(); i++) {
+ list[i]->SignalDestroyed.disconnect(this);
+ delete list[i];
+ }
}
const std::string& Port::Type() const {
@@ -338,11 +352,11 @@
<< ": A new connection was created on an existing remote address. "
"New remote candidate: "
<< conn->remote_candidate().ToSensitiveString();
- auto old_conn = absl::WrapUnique(ret.first->second);
+ ret.first->second->SignalDestroyed.disconnect(this);
+ ret.first->second->Destroy();
ret.first->second = conn;
- HandleConnectionDestroyed(old_conn.get());
- old_conn->Shutdown();
}
+ conn->SignalDestroyed.connect(this, &Port::OnConnectionDestroyed);
}
void Port::OnReadPacket(const char* data,
@@ -600,9 +614,9 @@
void Port::DestroyAllConnections() {
RTC_DCHECK_RUN_ON(thread_);
- for (auto& [unused, connection] : connections_) {
- connection->Shutdown();
- delete connection;
+ for (auto kv : connections_) {
+ kv.second->SignalDestroyed.disconnect(this);
+ kv.second->Destroy();
}
connections_.clear();
}
@@ -914,24 +928,6 @@
}
}
-void Port::DestroyConnectionInternal(Connection* conn, bool async) {
- RTC_DCHECK_RUN_ON(thread_);
- OnConnectionDestroyed(conn);
- conn->Shutdown();
- if (async) {
- // Unwind the stack before deleting the object in case upstream callers
- // need to refer to the Connection's state as part of teardown.
- // NOTE: We move ownership of `conn` into the capture section of the lambda
- // so that the object will always be deleted, including if PostTask fails.
- // In such a case (only tests), deletion would happen inside of the call
- // to `DestroyConnection()`.
- thread_->PostTask(
- webrtc::ToQueuedTask([conn = absl::WrapUnique(conn)]() {}));
- } else {
- delete conn;
- }
-}
-
void Port::Destroy() {
RTC_DCHECK(connections_.empty());
RTC_LOG(LS_INFO) << ToString() << ": Port deleted";
diff --git a/p2p/base/port.h b/p2p/base/port.h
index 9f24b7b..b1dab5e 100644
--- a/p2p/base/port.h
+++ b/p2p/base/port.h
@@ -293,20 +293,6 @@
// Returns the connection to the given address or NULL if none exists.
Connection* GetConnection(const rtc::SocketAddress& remote_addr) override;
- // Removes and deletes a connection object. By default the connection will
- // be deleted directly inside the function, but a caller may optionally pass
- // `async = true` in order to defer the actual `delete` call to happen when
- // the current call stack has been unwound. This may be needed when a
- // connection object needs to be deleted but pointers to the object are held
- // call stack.
- void DestroyConnection(Connection* conn) {
- DestroyConnectionInternal(conn, false);
- }
-
- void DestroyConnectionAsync(Connection* conn) {
- DestroyConnectionInternal(conn, true);
- }
-
// In a shared socket mode each port which shares the socket will decide
// to accept the packet based on the `remote_addr`. Currently only UDP
// port implemented this method.
@@ -466,14 +452,9 @@
private:
void Construct();
-
- // Called internally when deleting a connection object.
+ // Called when one of our connections deletes itself.
void OnConnectionDestroyed(Connection* conn);
- // Private implementation of DestroyConnection to keep the async usage
- // distinct.
- void DestroyConnectionInternal(Connection* conn, bool async);
-
void OnNetworkTypeChanged(const rtc::Network* network);
rtc::Thread* const thread_;
diff --git a/p2p/base/port_unittest.cc b/p2p/base/port_unittest.cc
index b9115f5..1c73c96 100644
--- a/p2p/base/port_unittest.cc
+++ b/p2p/base/port_unittest.cc
@@ -274,7 +274,11 @@
[this](PortInterface* port) { OnSrcPortDestroyed(port); });
}
- ~TestChannel() { Stop(); }
+ ~TestChannel() {
+ if (conn_) {
+ conn_->SignalDestroyed.disconnect(this);
+ }
+ }
int complete_count() { return complete_count_; }
Connection* conn() { return conn_; }
@@ -283,7 +287,6 @@
void Start() { port_->PrepareAddress(); }
void CreateConnection(const Candidate& remote_candidate) {
- RTC_DCHECK(!conn_);
conn_ = port_->CreateConnection(remote_candidate, Port::ORIGIN_MESSAGE);
IceMode remote_ice_mode =
(ice_mode_ == ICEMODE_FULL) ? ICEMODE_LITE : ICEMODE_FULL;
@@ -295,7 +298,6 @@
&TestChannel::OnConnectionReadyToSend);
connection_ready_to_send_ = false;
}
-
void OnConnectionStateChange(Connection* conn) {
if (conn->write_state() == Connection::STATE_WRITABLE) {
conn->set_use_candidate_attr(true);
@@ -303,10 +305,6 @@
}
}
void AcceptConnection(const Candidate& remote_candidate) {
- if (conn_) {
- conn_->SignalDestroyed.disconnect(this);
- conn_ = nullptr;
- }
ASSERT_TRUE(remote_request_.get() != NULL);
Candidate c = remote_candidate;
c.set_address(remote_address_);
@@ -319,7 +317,8 @@
void Ping(int64_t now) { conn_->Ping(now); }
void Stop() {
if (conn_) {
- port_->DestroyConnection(conn_);
+ conn_->SignalDestroyed.disconnect(this);
+ conn_->Destroy();
conn_ = nullptr;
}
}
@@ -359,7 +358,7 @@
void OnDestroyed(Connection* conn) {
ASSERT_EQ(conn_, conn);
RTC_LOG(LS_INFO) << "OnDestroy connection " << conn << " deleted";
- conn_ = nullptr;
+ conn_ = NULL;
// When the connection is destroyed, also clear these fields so future
// connections are possible.
remote_request_.reset();
@@ -678,7 +677,7 @@
// Speed up destroying ch2's connection such that the test is ready to
// accept a new connection from ch1 before ch1's connection destroys itself.
- ch2->Stop();
+ ch2->conn()->Destroy();
EXPECT_TRUE_WAIT(ch2->conn() == NULL, kDefaultTimeout);
}
diff --git a/p2p/base/tcp_port.cc b/p2p/base/tcp_port.cc
index 90842e1..2964c8b 100644
--- a/p2p/base/tcp_port.cc
+++ b/p2p/base/tcp_port.cc
@@ -509,8 +509,9 @@
port()->thread()->PostDelayedTask(
webrtc::ToQueuedTask(network_safety_,
[this]() {
- if (pretending_to_be_writable_)
- port()->DestroyConnection(this);
+ if (pretending_to_be_writable_) {
+ Destroy();
+ }
}),
reconnection_timeout());
} else if (!pretending_to_be_writable_) {
@@ -519,7 +520,7 @@
// to manually destroy here as this connection, as never connected, will not
// be scheduled for ping to trigger destroy.
socket_->UnsubscribeClose(this);
- port()->DestroyConnectionAsync(this);
+ Destroy();
}
}
diff --git a/p2p/base/turn_port.h b/p2p/base/turn_port.h
index 9308c5e..74d2493 100644
--- a/p2p/base/turn_port.h
+++ b/p2p/base/turn_port.h
@@ -201,11 +201,12 @@
// Finds the turn entry with `address` and sets its channel id.
// Returns true if the entry is found.
bool SetEntryChannelId(const rtc::SocketAddress& address, int channel_id);
+ // Visible for testing.
+ // Shuts down the turn port, usually because of some fatal errors.
+ void Close();
void HandleConnectionDestroyed(Connection* conn) override;
- void CloseForTest() { Close(); }
-
protected:
TurnPort(rtc::Thread* thread,
rtc::PacketSocketFactory* factory,
@@ -248,9 +249,6 @@
rtc::DiffServCodePoint StunDscpValue() const override;
- // Shuts down the turn port, frees requests and deletes connections.
- void Close();
-
private:
enum {
MSG_ALLOCATE_ERROR = MSG_FIRST_AVAILABLE,
diff --git a/p2p/base/turn_port_unittest.cc b/p2p/base/turn_port_unittest.cc
index 496b6a2..2244b9d 100644
--- a/p2p/base/turn_port_unittest.cc
+++ b/p2p/base/turn_port_unittest.cc
@@ -664,8 +664,7 @@
// Destroy the connection on the TURN port. The TurnEntry still exists, so
// the TURN port should still process a ping from an unknown address.
- turn_port_->DestroyConnection(conn2);
-
+ conn2->Destroy();
conn1->Ping(0);
EXPECT_TRUE_SIMULATED_WAIT(turn_unknown_address_, kSimulatedRtt,
fake_clock_);
@@ -1269,7 +1268,7 @@
EXPECT_EQ_SIMULATED_WAIT(Connection::STATE_WRITABLE, conn2->write_state(),
kSimulatedRtt * 2, fake_clock_);
- turn_port_->CloseForTest();
+ turn_port_->Close();
SIMULATED_WAIT(false, kSimulatedRtt, fake_clock_);
turn_unknown_address_ = false;
conn2->Ping(0);
