From 9def99487e3b41a044ae4854ff620b8ff817f10f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Niels=20M=C3=B6ller?= Date: Tue, 7 Sep 2021 09:16:49 +0200 Subject: [PATCH] Delete BasicPacketSocketFactory constructor with thread argument In callers where it's non-trivial to explicitly pass the right SocketFactory, pull the call to rtc::Thread::socketserver() into the caller, with a TODO comment. Bug: webrtc:13145 Change-Id: I029d3adca385d822180e089f016c3778e0d4fd0c Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/231227 Reviewed-by: Harald Alvestrand Commit-Queue: Niels Moller Cr-Commit-Position: refs/heads/main@{#35063} --- p2p/base/basic_packet_socket_factory.cc | 21 ++++----------------- p2p/base/basic_packet_socket_factory.h | 5 ----- p2p/base/fake_port_allocator.h | 4 +++- p2p/base/port_unittest.cc | 2 +- p2p/base/stun_port_unittest.cc | 2 +- p2p/base/tcp_port_unittest.cc | 2 +- p2p/base/test_turn_server.h | 7 +++++-- p2p/base/turn_port_unittest.cc | 2 +- p2p/client/basic_port_allocator.cc | 2 +- pc/connection_context.cc | 8 ++++++-- rtc_tools/network_tester/test_controller.cc | 4 +++- 11 files changed, 26 insertions(+), 33 deletions(-) diff --git a/p2p/base/basic_packet_socket_factory.cc b/p2p/base/basic_packet_socket_factory.cc index d20649763b..c23b5d9cb3 100644 --- a/p2p/base/basic_packet_socket_factory.cc +++ b/p2p/base/basic_packet_socket_factory.cc @@ -25,16 +25,12 @@ #include "rtc_base/socket_adapters.h" #include "rtc_base/socket_server.h" #include "rtc_base/ssl_adapter.h" -#include "rtc_base/thread.h" namespace rtc { -BasicPacketSocketFactory::BasicPacketSocketFactory(Thread* thread) - : thread_(thread), socket_factory_(NULL) {} - BasicPacketSocketFactory::BasicPacketSocketFactory( SocketFactory* socket_factory) - : thread_(NULL), socket_factory_(socket_factory) {} + : socket_factory_(socket_factory) {} BasicPacketSocketFactory::~BasicPacketSocketFactory() {} @@ -43,7 +39,7 @@ AsyncPacketSocket* BasicPacketSocketFactory::CreateUdpSocket( uint16_t min_port, uint16_t max_port) { // UDP sockets are simple. - Socket* socket = socket_factory()->CreateSocket(address.family(), SOCK_DGRAM); + Socket* socket = socket_factory_->CreateSocket(address.family(), SOCK_DGRAM); if (!socket) { return NULL; } @@ -67,7 +63,7 @@ AsyncPacketSocket* BasicPacketSocketFactory::CreateServerTcpSocket( } Socket* socket = - socket_factory()->CreateSocket(local_address.family(), SOCK_STREAM); + socket_factory_->CreateSocket(local_address.family(), SOCK_STREAM); if (!socket) { return NULL; } @@ -104,7 +100,7 @@ AsyncPacketSocket* BasicPacketSocketFactory::CreateClientTcpSocket( const std::string& user_agent, const PacketSocketTcpOptions& tcp_options) { Socket* socket = - socket_factory()->CreateSocket(local_address.family(), SOCK_STREAM); + socket_factory_->CreateSocket(local_address.family(), SOCK_STREAM); if (!socket) { return NULL; } @@ -215,13 +211,4 @@ int BasicPacketSocketFactory::BindSocket(Socket* socket, return ret; } -SocketFactory* BasicPacketSocketFactory::socket_factory() { - if (thread_) { - RTC_DCHECK(thread_ == Thread::Current()); - return thread_->socketserver(); - } else { - return socket_factory_; - } -} - } // namespace rtc diff --git a/p2p/base/basic_packet_socket_factory.h b/p2p/base/basic_packet_socket_factory.h index 368c976a9c..030eb82f4f 100644 --- a/p2p/base/basic_packet_socket_factory.h +++ b/p2p/base/basic_packet_socket_factory.h @@ -19,11 +19,9 @@ namespace rtc { class SocketFactory; -class Thread; class BasicPacketSocketFactory : public PacketSocketFactory { public: - explicit BasicPacketSocketFactory(Thread* thread); explicit BasicPacketSocketFactory(SocketFactory* socket_factory); ~BasicPacketSocketFactory() override; @@ -49,9 +47,6 @@ class BasicPacketSocketFactory : public PacketSocketFactory { uint16_t min_port, uint16_t max_port); - SocketFactory* socket_factory(); - - Thread* thread_; SocketFactory* socket_factory_; }; diff --git a/p2p/base/fake_port_allocator.h b/p2p/base/fake_port_allocator.h index efe9a53a16..83b27dbafe 100644 --- a/p2p/base/fake_port_allocator.h +++ b/p2p/base/fake_port_allocator.h @@ -208,11 +208,13 @@ class FakePortAllocatorSession : public PortAllocatorSession { class FakePortAllocator : public cricket::PortAllocator { public: + // TODO(bugs.webrtc.org/13145): Require non-null `factory`. FakePortAllocator(rtc::Thread* network_thread, rtc::PacketSocketFactory* factory) : network_thread_(network_thread), factory_(factory) { if (factory_ == NULL) { - owned_factory_.reset(new rtc::BasicPacketSocketFactory(network_thread_)); + owned_factory_.reset(new rtc::BasicPacketSocketFactory( + network_thread_ ? network_thread_->socketserver() : nullptr)); factory_ = owned_factory_.get(); } diff --git a/p2p/base/port_unittest.cc b/p2p/base/port_unittest.cc index 7d7bfdb5e4..9c4bc0f075 100644 --- a/p2p/base/port_unittest.cc +++ b/p2p/base/port_unittest.cc @@ -395,7 +395,7 @@ class PortTest : public ::testing::Test, public sigslot::has_slots<> { PortTest() : ss_(new rtc::VirtualSocketServer()), main_(ss_.get()), - socket_factory_(rtc::Thread::Current()), + socket_factory_(ss_.get()), nat_factory1_(ss_.get(), kNatAddr1, SocketAddress()), nat_factory2_(ss_.get(), kNatAddr2, SocketAddress()), nat_socket_factory1_(&nat_factory1_), diff --git a/p2p/base/stun_port_unittest.cc b/p2p/base/stun_port_unittest.cc index 36733ccf5a..5ee707b22b 100644 --- a/p2p/base/stun_port_unittest.cc +++ b/p2p/base/stun_port_unittest.cc @@ -48,7 +48,7 @@ class StunPortTestBase : public ::testing::Test, public sigslot::has_slots<> { : ss_(new rtc::VirtualSocketServer()), thread_(ss_.get()), network_("unittest", "unittest", kLocalAddr.ipaddr(), 32), - socket_factory_(rtc::Thread::Current()), + socket_factory_(ss_.get()), stun_server_1_(cricket::TestStunServer::Create(ss_.get(), kStunAddr1)), stun_server_2_(cricket::TestStunServer::Create(ss_.get(), kStunAddr2)), done_(false), diff --git a/p2p/base/tcp_port_unittest.cc b/p2p/base/tcp_port_unittest.cc index 2c9fbceeae..79f1314e24 100644 --- a/p2p/base/tcp_port_unittest.cc +++ b/p2p/base/tcp_port_unittest.cc @@ -61,7 +61,7 @@ class TCPPortTest : public ::testing::Test, public sigslot::has_slots<> { TCPPortTest() : ss_(new rtc::VirtualSocketServer()), main_(ss_.get()), - socket_factory_(rtc::Thread::Current()), + socket_factory_(ss_.get()), username_(rtc::CreateRandomString(ICE_UFRAG_LENGTH)), password_(rtc::CreateRandomString(ICE_PWD_LENGTH)) {} diff --git a/p2p/base/test_turn_server.h b/p2p/base/test_turn_server.h index 479ca8bb39..e1deb5901e 100644 --- a/p2p/base/test_turn_server.h +++ b/p2p/base/test_turn_server.h @@ -58,8 +58,11 @@ class TestTurnServer : public TurnAuthInterface { const std::string& common_name = "test turn server") : server_(thread), thread_(thread) { AddInternalSocket(int_addr, int_protocol, ignore_bad_cert, common_name); - server_.SetExternalSocketFactory(new rtc::BasicPacketSocketFactory(thread), - udp_ext_addr); + // TODO(bugs.webrtc.org/13145): Take a SocketFactory as argument, so we + // don't need thread_->socketserver(). + server_.SetExternalSocketFactory( + new rtc::BasicPacketSocketFactory(thread_->socketserver()), + udp_ext_addr); server_.set_realm(kTestRealm); server_.set_software(kTestSoftware); server_.set_auth_hook(this); diff --git a/p2p/base/turn_port_unittest.cc b/p2p/base/turn_port_unittest.cc index 7a7092d396..7854fdb986 100644 --- a/p2p/base/turn_port_unittest.cc +++ b/p2p/base/turn_port_unittest.cc @@ -172,7 +172,7 @@ class TurnPortTest : public ::testing::Test, TurnPortTest() : ss_(new TurnPortTestVirtualSocketServer()), main_(ss_.get()), - socket_factory_(rtc::Thread::Current()), + socket_factory_(ss_.get()), turn_server_(&main_, kTurnUdpIntAddr, kTurnUdpExtAddr), turn_ready_(false), turn_error_(false), diff --git a/p2p/client/basic_port_allocator.cc b/p2p/client/basic_port_allocator.cc index 6284625bed..2a601d5191 100644 --- a/p2p/client/basic_port_allocator.cc +++ b/p2p/client/basic_port_allocator.cc @@ -378,7 +378,7 @@ void BasicPortAllocatorSession::StartGettingPorts() { state_ = SessionState::GATHERING; if (!socket_factory_) { owned_socket_factory_.reset( - new rtc::BasicPacketSocketFactory(network_thread_)); + new rtc::BasicPacketSocketFactory(network_thread_->socketserver())); socket_factory_ = owned_socket_factory_.get(); } diff --git a/pc/connection_context.cc b/pc/connection_context.cc index 6fdcac3113..691dd908eb 100644 --- a/pc/connection_context.cc +++ b/pc/connection_context.cc @@ -121,8 +121,12 @@ ConnectionContext::ConnectionContext( default_network_manager_ = std::make_unique( network_monitor_factory_.get()); - default_socket_factory_ = - std::make_unique(network_thread()); + // TODO(bugs.webrtc.org/13145): Either require that a PacketSocketFactory + // always is injected (with no need to construct this default factory), or get + // the appropriate underlying SocketFactory without going through the + // rtc::Thread::socketserver() accessor. + default_socket_factory_ = std::make_unique( + network_thread()->socketserver()); worker_thread_->Invoke(RTC_FROM_HERE, [&]() { channel_manager_ = cricket::ChannelManager::Create( diff --git a/rtc_tools/network_tester/test_controller.cc b/rtc_tools/network_tester/test_controller.cc index 85a5a57bc0..9c0d324533 100644 --- a/rtc_tools/network_tester/test_controller.cc +++ b/rtc_tools/network_tester/test_controller.cc @@ -23,7 +23,9 @@ TestController::TestController(int min_port, int max_port, const std::string& config_file_path, const std::string& log_file_path) - : socket_factory_(rtc::ThreadManager::Instance()->WrapCurrentThread()), + // TODO(bugs.webrtc.org/13145): Add a SocketFactory argument. + : socket_factory_( + rtc::ThreadManager::Instance()->WrapCurrentThread()->socketserver()), config_file_path_(config_file_path), packet_logger_(log_file_path), local_test_done_(false),