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 <deadbeef@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Niels Moller <nisse@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#35302}

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",
 

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));

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<TCPConnection*>(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<TCPConnection*>(ch2.conn())
+                ->socket()
+                ->GetOption(rtc::Socket::OPT_NODELAY, &option_value);
+  ASSERT_EQ(0, success);
+  EXPECT_EQ(1, option_value);
 }
 
 TEST_F(PortTest, TestDelayedBindingUdp) {

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;

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<rtc::AsyncListenSocket> 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<rtc::Socket::Option, int> socket_options_;
+
   int error_;
   std::list<Incoming> incoming_;
 

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<AsyncListenSocket*, AsyncPacketSocket*> SignalNewConnection;
 };
 

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);
 

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: