From 646fddc3c9bbbe0f42b306d08ea944521601d8c4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Niels=20M=C3=B6ller?= Date: Tue, 2 Nov 2021 15:56:05 +0100 Subject: [PATCH] Fix TCPPort::SetOption to apply options to the accepted sockets The AsyncListenSocket::SetOption method then gets unused, and can be deleted. Bug: webrtc:13065 Change-Id: Idcf70a75b96036290fdceff6e0f96a8d5617f87f Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/236580 Reviewed-by: Taylor Brandstetter Reviewed-by: Harald Alvestrand Commit-Queue: Niels Moller Cr-Commit-Position: refs/heads/main@{#35302} --- p2p/BUILD.gn | 1 + p2p/base/basic_packet_socket_factory.cc | 8 ----- p2p/base/port_unittest.cc | 41 ++++++++++++++++++++++++- p2p/base/tcp_port.cc | 23 ++++++++------ p2p/base/tcp_port.h | 10 ++++++ rtc_base/async_packet_socket.h | 4 --- rtc_base/async_tcp_socket.cc | 8 ----- rtc_base/async_tcp_socket.h | 3 -- 8 files changed, 65 insertions(+), 33 deletions(-) diff --git a/p2p/BUILD.gn b/p2p/BUILD.gn index d09cab9edc..a9c059bed3 100644 --- a/p2p/BUILD.gn +++ b/p2p/BUILD.gn @@ -111,6 +111,7 @@ rtc_library("rtc_p2p") { "../rtc_base:socket_factory", "../rtc_base:socket_server", "../rtc_base:threading", + "../rtc_base/containers:flat_map", "../rtc_base/experiments:field_trial_parser", "../rtc_base/system:no_unique_address", diff --git a/p2p/base/basic_packet_socket_factory.cc b/p2p/base/basic_packet_socket_factory.cc index c144e4e337..5bc02dd0f0 100644 --- a/p2p/base/basic_packet_socket_factory.cc +++ b/p2p/base/basic_packet_socket_factory.cc @@ -78,14 +78,6 @@ AsyncListenSocket* BasicPacketSocketFactory::CreateServerTcpSocket( return NULL; } - // Set TCP_NODELAY (via OPT_NODELAY) for improved performance; this causes - // small media packets to be sent immediately rather than being buffered up, - // reducing latency. - if (socket->SetOption(Socket::OPT_NODELAY, 1) != 0) { - RTC_LOG(LS_ERROR) << "Setting TCP_NODELAY option failed with error " - << socket->GetError(); - } - RTC_CHECK(!(opts & PacketSocketFactory::OPT_STUN)); return new AsyncTcpListenSocket(absl::WrapUnique(socket)); diff --git a/p2p/base/port_unittest.cc b/p2p/base/port_unittest.cc index 146c4f1347..06dafe8dbf 100644 --- a/p2p/base/port_unittest.cc +++ b/p2p/base/port_unittest.cc @@ -1499,12 +1499,51 @@ TEST_F(PortTest, TestIceRoleConflict) { } TEST_F(PortTest, TestTcpNoDelay) { + rtc::ScopedFakeClock clock; auto port1 = CreateTcpPort(kLocalAddr1); port1->SetIceRole(cricket::ICEROLE_CONTROLLING); int option_value = -1; int success = port1->GetOption(rtc::Socket::OPT_NODELAY, &option_value); ASSERT_EQ(0, success); // GetOption() should complete successfully w/ 0 - ASSERT_EQ(1, option_value); + EXPECT_EQ(1, option_value); + + auto port2 = CreateTcpPort(kLocalAddr2); + port2->SetIceRole(cricket::ICEROLE_CONTROLLED); + + // Set up a connection, and verify that option is set on connected sockets at + // both ends. + TestChannel ch1(std::move(port1)); + TestChannel ch2(std::move(port2)); + // Acquire addresses. + ch1.Start(); + ch2.Start(); + ASSERT_EQ_SIMULATED_WAIT(1, ch1.complete_count(), kDefaultTimeout, clock); + ASSERT_EQ_SIMULATED_WAIT(1, ch2.complete_count(), kDefaultTimeout, clock); + // Connect and send a ping from src to dst. + ch1.CreateConnection(GetCandidate(ch2.port())); + ASSERT_TRUE(ch1.conn() != NULL); + EXPECT_TRUE_SIMULATED_WAIT(ch1.conn()->connected(), kDefaultTimeout, + clock); // for TCP connect + ch1.Ping(); + SIMULATED_WAIT(!ch2.remote_address().IsNil(), kShortTimeout, clock); + + // Accept the connection. + ch2.AcceptConnection(GetCandidate(ch1.port())); + ASSERT_TRUE(ch2.conn() != NULL); + + option_value = -1; + success = static_cast(ch1.conn()) + ->socket() + ->GetOption(rtc::Socket::OPT_NODELAY, &option_value); + ASSERT_EQ(0, success); + EXPECT_EQ(1, option_value); + + option_value = -1; + success = static_cast(ch2.conn()) + ->socket() + ->GetOption(rtc::Socket::OPT_NODELAY, &option_value); + ASSERT_EQ(0, success); + EXPECT_EQ(1, option_value); } TEST_F(PortTest, TestDelayedBindingUdp) { diff --git a/p2p/base/tcp_port.cc b/p2p/base/tcp_port.cc index ea805e5894..9d542074a4 100644 --- a/p2p/base/tcp_port.cc +++ b/p2p/base/tcp_port.cc @@ -106,6 +106,10 @@ TCPPort::TCPPort(rtc::Thread* thread, if (allow_listen_) { TryCreateServerSocket(); } + // Set TCP_NODELAY (via OPT_NODELAY) for improved performance; this causes + // small media packets to be sent immediately rather than being buffered up, + // reducing latency. + SetOption(rtc::Socket::OPT_NODELAY, 1); } TCPPort::~TCPPort() { @@ -243,19 +247,17 @@ int TCPPort::SendTo(const void* data, } int TCPPort::GetOption(rtc::Socket::Option opt, int* value) { - if (listen_socket_) { - return listen_socket_->GetOption(opt, value); - } else { - return SOCKET_ERROR; + auto const& it = socket_options_.find(opt); + if (it == socket_options_.end()) { + return -1; } + *value = it->second; + return 0; } int TCPPort::SetOption(rtc::Socket::Option opt, int value) { - if (listen_socket_) { - return listen_socket_->SetOption(opt, value); - } else { - return SOCKET_ERROR; - } + socket_options_[opt] = value; + return 0; } int TCPPort::GetError() { @@ -274,6 +276,9 @@ void TCPPort::OnNewConnection(rtc::AsyncListenSocket* socket, rtc::AsyncPacketSocket* new_socket) { RTC_DCHECK(socket == listen_socket_.get()); + for (const auto& option : socket_options_) { + new_socket->SetOption(option.first, option.second); + } Incoming incoming; incoming.addr = new_socket->GetRemoteAddress(); incoming.socket = new_socket; diff --git a/p2p/base/tcp_port.h b/p2p/base/tcp_port.h index 124e93ddba..932af50aa4 100644 --- a/p2p/base/tcp_port.h +++ b/p2p/base/tcp_port.h @@ -19,6 +19,7 @@ #include "p2p/base/connection.h" #include "p2p/base/port.h" #include "rtc_base/async_packet_socket.h" +#include "rtc_base/containers/flat_map.h" namespace cricket { @@ -52,6 +53,9 @@ class TCPPort : public Port { void PrepareAddress() override; + // Options apply to accepted sockets. + // TODO(bugs.webrtc.org/13065): Apply also to outgoing and existing + // connections. int GetOption(rtc::Socket::Option opt, int* value) override; int SetOption(rtc::Socket::Option opt, int value) override; int GetError() override; @@ -104,6 +108,12 @@ class TCPPort : public Port { bool allow_listen_; std::unique_ptr listen_socket_; + // Options to be applied to accepted sockets. + // TODO(bugs.webrtc:13065): Configure connect/accept in the same way, but + // currently, setting OPT_NODELAY for client sockets is done (unconditionally) + // by BasicPacketSocketFactory::CreateClientTcpSocket. + webrtc::flat_map socket_options_; + int error_; std::list incoming_; diff --git a/rtc_base/async_packet_socket.h b/rtc_base/async_packet_socket.h index ce36dd6373..b5fcf8edb7 100644 --- a/rtc_base/async_packet_socket.h +++ b/rtc_base/async_packet_socket.h @@ -147,10 +147,6 @@ class RTC_EXPORT AsyncListenSocket : public sigslot::has_slots<> { // socket is not bound yet (GetState() returns kBinding). virtual SocketAddress GetLocalAddress() const = 0; - // Get/set options. - virtual int GetOption(Socket::Option opt, int* value) = 0; - virtual int SetOption(Socket::Option opt, int value) = 0; - sigslot::signal2 SignalNewConnection; }; diff --git a/rtc_base/async_tcp_socket.cc b/rtc_base/async_tcp_socket.cc index 37a1052d52..bc8b7ddbc7 100644 --- a/rtc_base/async_tcp_socket.cc +++ b/rtc_base/async_tcp_socket.cc @@ -332,14 +332,6 @@ SocketAddress AsyncTcpListenSocket::GetLocalAddress() const { return socket_->GetLocalAddress(); } -int AsyncTcpListenSocket::GetOption(Socket::Option opt, int* value) { - return socket_->GetOption(opt, value); -} - -int AsyncTcpListenSocket::SetOption(Socket::Option opt, int value) { - return socket_->SetOption(opt, value); -} - void AsyncTcpListenSocket::OnReadEvent(Socket* socket) { RTC_DCHECK(socket_.get() == socket); diff --git a/rtc_base/async_tcp_socket.h b/rtc_base/async_tcp_socket.h index 901e5cfe33..ca61b54d78 100644 --- a/rtc_base/async_tcp_socket.h +++ b/rtc_base/async_tcp_socket.h @@ -109,9 +109,6 @@ class AsyncTcpListenSocket : public AsyncListenSocket { State GetState() const override; SocketAddress GetLocalAddress() const override; - int GetOption(Socket::Option opt, int* value) override; - int SetOption(Socket::Option opt, int value) override; - virtual void HandleIncomingConnection(rtc::Socket* socket); private: