admin/webrtc_m130
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Revert "Make deletion of Connection objects more deterministic."

Browse Source 
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}
This commit is contained in:
Tomas Gunnarsson 2022-05-02 19:08:43 +00:00 committed by WebRTC LUCI CQ
parent edeeef24a6
commit 869c87a2b9
9 changed files with 71 additions and 99 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

36
p2p/base/connection.cc
View File

@ -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::WeakPtr<Port> port,
Connection::~Connection() {
RTC_DCHECK_RUN_ON(network_thread_);
RTC_DCHECK(!port_);
}
webrtc::TaskQueueBase* Connection::network_thread() const {
@ -831,15 +831,9 @@ void Connection::Prune() {
}
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 @@ bool Connection::Shutdown() {
// 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 @@ void Connection::FailAndPrune() {
// 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 @@ void Connection::UpdateState(int64_t now) {
// Update the receiving state.
UpdateReceiving(now);
if (dead(now)) {
port_->DestroyConnectionAsync(this);
Destroy();
}
}
@ -1386,8 +1393,7 @@ void Connection::OnConnectionRequestErrorResponse(ConnectionRequest* request,
RTC_LOG(LS_ERROR) << ToString()
<< ": Received STUN error response, code=" << error_code
<< "; killing connection";
set_state(IceCandidatePairState::FAILED);
port_->DestroyConnectionAsync(this);
FailAndDestroy();
}
}

15
p2p/base/connection.h
View File

@ -188,18 +188,11 @@ class Connection : public CandidatePairInterface {
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.

11
p2p/base/p2p_transport_channel.cc
View File

@ -222,10 +222,9 @@ P2PTransportChannel::~P2PTransportChannel() {
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 @@ void P2PTransportChannel::RemoveConnectionForTest(Connection* connection) {
RTC_DCHECK(!FindConnection(connection));
if (selected_connection_ == connection)
selected_connection_ = nullptr;
connection->port()->DestroyConnection(connection);
connection->Destroy();
}
// Monitor connection states.
@ -2041,7 +2040,7 @@ void P2PTransportChannel::HandleAllTimedOut() {
}
connection->SignalDestroyed.disconnect(this);
ice_controller_->OnConnectionDestroyed(connection);
connection->port()->DestroyConnection(connection);
connection->Destroy();
}
if (update_selected_connection)

48
p2p/base/port.cc
View File

@ -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 @@ void Port::Construct() {
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 @@ void Port::AddOrReplaceConnection(Connection* conn) {
<< ": 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 @@ rtc::DiffServCodePoint Port::StunDscpValue() const {
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::OnConnectionDestroyed(Connection* conn) {
}
}
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";

21
p2p/base/port.h
View File

@ -293,20 +293,6 @@ class Port : public PortInterface,
// 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 @@ class Port : public PortInterface,
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_;

19
p2p/base/port_unittest.cc
View File

@ -274,7 +274,11 @@ class TestChannel : public sigslot::has_slots<> {
[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 @@ class TestChannel : public sigslot::has_slots<> {
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 @@ class TestChannel : public sigslot::has_slots<> {
&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 @@ class TestChannel : public sigslot::has_slots<> {
}
}
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 @@ class TestChannel : public sigslot::has_slots<> {
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 @@ class TestChannel : public sigslot::has_slots<> {
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 @@ class PortTest : public ::testing::Test, public sigslot::has_slots<> {
// 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);
}

7
p2p/base/tcp_port.cc
View File

@ -509,8 +509,9 @@ void TCPConnection::OnClose(rtc::AsyncPacketSocket* socket, int error) {
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 @@ void TCPConnection::OnClose(rtc::AsyncPacketSocket* socket, int error) {
// 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();
}
}

8
p2p/base/turn_port.h
View File

@ -201,11 +201,12 @@ class TurnPort : public Port {
// 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 @@ class TurnPort : public Port {
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,

5
p2p/base/turn_port_unittest.cc
View File

@ -664,8 +664,7 @@ class TurnPortTest : public ::testing::Test,
// 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 @@ TEST_F(TurnPortTest, TestStopProcessingPacketsAfterClosed) {
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);