envoyproxy · alyssawilk · Jan 14, 2021 · Jan 5, 2021 · Jan 7, 2021 · Jan 12, 2021
diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst
@@ -28,6 +28,7 @@ Minor Behavior Changes
 * mongo proxy metrics: swapped network connection remote and local closed counters previously set reversed (`cx_destroy_local_with_active_rq` and `cx_destroy_remote_with_active_rq`).
 * outlier detection: added :ref:`max_ejection_time <envoy_v3_api_field_config.cluster.v3.OutlierDetection.max_ejection_time>` to limit ejection time growth when a node stays unhealthy for extended period of time. By default :ref:`max_ejection_time <envoy_v3_api_field_config.cluster.v3.OutlierDetection.max_ejection_time>` limits ejection time to 5 minutes. Additionally, when the node stays healthy, ejection time decreases. See :ref:`ejection algorithm<arch_overview_outlier_detection_algorithm>` for more info. Previously, ejection time could grow without limit and never decreased.
 * performance: improve performance when handling large HTTP/1 bodies.
+* tcp: setting NODELAY in the base connection class. This should have no effect for TCP or HTTP proxying, but may improve throughput in other areas. This behavior can be temporarily reverted by setting `envoy.reloadable_features.always_nodelay` to false.
 * tls: removed RSA key transport and SHA-1 cipher suites from the client-side defaults.
 * watchdog: the watchdog action :ref:`abort_action <envoy_v3_api_msg_watchdog.v3alpha.AbortActionConfig>` is now the default action to terminate the process if watchdog kill / multikill is enabled.
 * xds: to support TTLs, heartbeating has been added to xDS. As a result, responses that contain empty resources without updating the version will no longer be propagated to the

diff --git a/source/common/http/codec_client.cc b/source/common/http/codec_client.cc
@@ -49,7 +49,9 @@ CodecClient::CodecClient(Type type, Network::ClientConnectionPtr&& connection,
 
   // We just universally set no delay on connections. Theoretically we might at some point want
   // to make this configurable.
-  connection_->noDelay(true);
+  if (!Runtime::runtimeFeatureEnabled("envoy.reloadable_features.always_nodelay")) {
+    connection_->noDelay(true);
+  }
 }
 
 CodecClient::~CodecClient() = default;

diff --git a/source/common/network/connection_impl.cc b/source/common/network/connection_impl.cc
@@ -18,6 +18,7 @@
 #include "common/network/listen_socket_impl.h"
 #include "common/network/raw_buffer_socket.h"
 #include "common/network/utility.h"
+#include "common/runtime/runtime_features.h"
 
 namespace Envoy {
 namespace Network {
@@ -758,7 +759,11 @@ ServerConnectionImpl::ServerConnectionImpl(Event::Dispatcher& dispatcher,
                                            TransportSocketPtr&& transport_socket,
                                            StreamInfo::StreamInfo& stream_info, bool connected)
     : ConnectionImpl(dispatcher, std::move(socket), std::move(transport_socket), stream_info,
-                     connected) {}
+                     connected) {
+  if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.always_nodelay")) {
+    noDelay(true);
+  }
+}
 
 void ServerConnectionImpl::setTransportSocketConnectTimeout(std::chrono::milliseconds timeout) {
   if (!transport_connect_pending_) {
@@ -836,6 +841,9 @@ void ClientConnectionImpl::connect() {
   ENVOY_CONN_LOG(debug, "connecting to {}", *this, socket_->remoteAddress()->asString());
   const Api::SysCallIntResult result = socket_->connect(socket_->remoteAddress());
   if (result.rc_ == 0) {
+    if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.always_nodelay")) {
+      noDelay(true);
+    }
     // write will become ready.
     ASSERT(connecting_);
   } else {

diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc
@@ -61,6 +61,7 @@ constexpr const char* runtime_features[] = {
     "envoy.reloadable_features.allow_500_after_100",
     "envoy.reloadable_features.allow_preconnect",
     "envoy.reloadable_features.allow_response_for_timeout",
+    "envoy.reloadable_features.always_nodelay",
     "envoy.reloadable_features.consume_all_retry_headers",
     "envoy.reloadable_features.check_ocsp_policy",
     "envoy.reloadable_features.disable_tls_inspector_injection",

diff --git a/source/common/tcp/conn_pool.cc b/source/common/tcp/conn_pool.cc
@@ -6,6 +6,7 @@
 #include "envoy/event/timer.h"
 #include "envoy/upstream/upstream.h"
 
+#include "common/runtime/runtime_features.h"
 #include "common/stats/timespan_impl.h"
 #include "common/upstream/upstream_impl.h"
 
@@ -31,7 +32,10 @@ ActiveTcpClient::ActiveTcpClient(Envoy::ConnectionPool::ConnPoolImplBase& parent
                                    host->cluster().stats().upstream_cx_tx_bytes_total_,
                                    host->cluster().stats().upstream_cx_tx_bytes_buffered_,
                                    &host->cluster().stats().bind_errors_, nullptr});
-  connection_->noDelay(true);
+
+  if (!Runtime::runtimeFeatureEnabled("envoy.reloadable_features.always_nodelay")) {
+    connection_->noDelay(true);
+  }
   connection_->connect();
 }
 

diff --git a/source/common/tcp/original_conn_pool.cc b/source/common/tcp/original_conn_pool.cc
@@ -6,6 +6,7 @@
 #include "envoy/event/timer.h"
 #include "envoy/upstream/upstream.h"
 
+#include "common/runtime/runtime_features.h"
 #include "common/stats/timespan_impl.h"
 #include "common/upstream/upstream_impl.h"
 
@@ -400,7 +401,9 @@ OriginalConnPoolImpl::ActiveConn::ActiveConn(OriginalConnPoolImpl& parent)
 
   // We just universally set no delay on connections. Theoretically we might at some point want
   // to make this configurable.
-  conn_->noDelay(true);
+  if (!Runtime::runtimeFeatureEnabled("envoy.reloadable_features.always_nodelay")) {
+    conn_->noDelay(true);
+  }
 }
 
 OriginalConnPoolImpl::ActiveConn::~ActiveConn() {

diff --git a/source/common/upstream/health_checker_impl.cc b/source/common/upstream/health_checker_impl.cc
@@ -523,7 +523,9 @@ void TcpHealthCheckerImpl::TcpActiveHealthCheckSession::onInterval() {
 
     expect_close_ = false;
     client_->connect();
-    client_->noDelay(true);
+    if (!Runtime::runtimeFeatureEnabled("envoy.reloadable_features.always_nodelay")) {
+      client_->noDelay(true);
+    }
   }
 
   if (!parent_.send_bytes_.empty()) {

diff --git a/source/extensions/filters/network/common/redis/client_impl.cc b/source/extensions/filters/network/common/redis/client_impl.cc
@@ -2,6 +2,8 @@
 
 #include "envoy/extensions/filters/network/redis_proxy/v3/redis_proxy.pb.h"
 
+#include "common/runtime/runtime_features.h"
+
 namespace Envoy {
 namespace Extensions {
 namespace NetworkFilters {
@@ -64,7 +66,9 @@ ClientPtr ClientImpl::create(Upstream::HostConstSharedPtr host, Event::Dispatche
   client->connection_->addConnectionCallbacks(*client);
   client->connection_->addReadFilter(Network::ReadFilterSharedPtr{new UpstreamReadFilter(*client)});
   client->connection_->connect();
-  client->connection_->noDelay(true);
+  if (!Runtime::runtimeFeatureEnabled("envoy.reloadable_features.always_nodelay")) {
+    client->connection_->noDelay(true);
+  }
   return client;
 }
 

diff --git a/source/server/connection_handler_impl.cc b/source/server/connection_handler_impl.cc
@@ -12,6 +12,7 @@
 #include "common/event/deferred_task.h"
 #include "common/network/connection_impl.h"
 #include "common/network/utility.h"
+#include "common/runtime/runtime_features.h"
 #include "common/stats/timespan_impl.h"
 
 #include "extensions/transport_sockets/well_known_names.h"
@@ -591,7 +592,9 @@ ConnectionHandlerImpl::ActiveTcpConnection::ActiveTcpConnection(
           active_connections_.listener_.stats_.downstream_cx_length_ms_, time_source)) {
   // We just universally set no delay on connections. Theoretically we might at some point want
   // to make this configurable.
-  connection_->noDelay(true);
+  if (!Runtime::runtimeFeatureEnabled("envoy.reloadable_features.always_nodelay")) {
+    connection_->noDelay(true);
+  }
   auto& listener = active_connections_.listener_;
   listener.stats_.downstream_cx_total_.inc();
   listener.stats_.downstream_cx_active_.inc();

diff --git a/test/common/network/BUILD b/test/common/network/BUILD
@@ -86,6 +86,7 @@ envoy_cc_test(
         "//test/mocks/api:api_mocks",
         "//test/mocks/buffer:buffer_mocks",
         "//test/mocks/event:event_mocks",
+        "//test/mocks/network:io_handle_mocks",
         "//test/mocks/network:network_mocks",
         "//test/mocks/stats:stats_mocks",
         "//test/test_common:environment_lib",

diff --git a/test/common/network/connection_impl_test.cc b/test/common/network/connection_impl_test.cc
@@ -120,6 +120,7 @@ class TestClientConnectionImpl : public Network::ClientConnectionImpl {
 public:
   using ClientConnectionImpl::ClientConnectionImpl;
   Buffer::Instance& readBuffer() { return *read_buffer_; }
+  ConnectionSocketPtr& socket() { return socket_; }
 };
 
 class ConnectionImplTest : public testing::TestWithParam<Address::IpVersion> {
@@ -396,11 +397,13 @@ TEST_P(ConnectionImplTest, SetServerTransportSocketTimeout) {
   ConnectionMocks mocks = createConnectionMocks(false);
   MockTransportSocket* transport_socket = mocks.transport_socket_.get();
   IoHandlePtr io_handle = std::make_unique<IoSocketHandleImpl>(0);
+  // Avoid setting noDelay on the fake fd of 0.
+  auto local_addr = std::make_shared<Network::Address::PipeInstance>("/pipe/path");
 
   auto* mock_timer = new NiceMock<Event::MockTimer>(mocks.dispatcher_.get());
   auto server_connection = std::make_unique<Network::ServerConnectionImpl>(
       *mocks.dispatcher_,
-      std::make_unique<ConnectionSocketImpl>(std::move(io_handle), nullptr, nullptr),
+      std::make_unique<ConnectionSocketImpl>(std::move(io_handle), local_addr, nullptr),
       std::move(mocks.transport_socket_), stream_info_, true);
 
   EXPECT_CALL(*mock_timer, enableTimer(std::chrono::milliseconds(3 * 1000), _));
@@ -415,10 +418,11 @@ TEST_P(ConnectionImplTest, SetServerTransportSocketTimeoutAfterConnect) {
   ConnectionMocks mocks = createConnectionMocks(false);
   MockTransportSocket* transport_socket = mocks.transport_socket_.get();
   IoHandlePtr io_handle = std::make_unique<IoSocketHandleImpl>(0);
+  auto local_addr = std::make_shared<Network::Address::PipeInstance>("/pipe/path");
 
   auto server_connection = std::make_unique<Network::ServerConnectionImpl>(
       *mocks.dispatcher_,
-      std::make_unique<ConnectionSocketImpl>(std::move(io_handle), nullptr, nullptr),
+      std::make_unique<ConnectionSocketImpl>(std::move(io_handle), local_addr, nullptr),
       std::move(mocks.transport_socket_), stream_info_, true);
 
   transport_socket->callbacks_->raiseEvent(ConnectionEvent::Connected);
@@ -433,11 +437,12 @@ TEST_P(ConnectionImplTest, ServerTransportSocketTimeoutDisabledOnConnect) {
   ConnectionMocks mocks = createConnectionMocks(false);
   MockTransportSocket* transport_socket = mocks.transport_socket_.get();
   IoHandlePtr io_handle = std::make_unique<IoSocketHandleImpl>(0);
+  auto local_addr = std::make_shared<Network::Address::PipeInstance>("/pipe/path");
 
   auto* mock_timer = new NiceMock<Event::MockTimer>(mocks.dispatcher_.get());
   auto server_connection = std::make_unique<Network::ServerConnectionImpl>(
       *mocks.dispatcher_,
-      std::make_unique<ConnectionSocketImpl>(std::move(io_handle), nullptr, nullptr),
+      std::make_unique<ConnectionSocketImpl>(std::move(io_handle), local_addr, nullptr),
       std::move(mocks.transport_socket_), stream_info_, true);
 
   bool timer_destroyed = false;
@@ -595,7 +600,24 @@ TEST_P(ConnectionImplTest, ConnectionStats) {
   MockConnectionStats client_connection_stats;
   client_connection_->setConnectionStats(client_connection_stats.toBufferStats());
   EXPECT_TRUE(client_connection_->connecting());
+
+  // Make sure that NO_DELAY starts out false, so that the check below verifies that it transitions
+  // to true actually tests something.
+  int initial_value = 0;
+  socklen_t size = sizeof(int);
+  Api::SysCallIntResult result = testClientConnection()->socket()->getSocketOption(
+      IPPROTO_TCP, TCP_NODELAY, &initial_value, &size);
+  ASSERT_EQ(0, result.rc_);
+  ASSERT_EQ(0, initial_value);
+
   client_connection_->connect();
+
+  int new_value = 0;
+  result = testClientConnection()->socket()->getSocketOption(IPPROTO_TCP, TCP_NODELAY,
+                                                             &initial_value, &size);
+  ASSERT_EQ(0, result.rc_);
+  ASSERT_EQ(0, new_value);
+
   // The Network::Connection class oddly uses onWrite as its indicator of if
   // it's done connection, rather than the Connected event.
   EXPECT_TRUE(client_connection_->connecting());

diff --git a/test/common/tcp/conn_pool_test.cc b/test/common/tcp/conn_pool_test.cc
@@ -319,7 +319,6 @@ class TcpConnPoolImplDestructorTest : public Event::TestUsingSimulatedTime,
     EXPECT_CALL(*connection_, addReadFilter(_));
     EXPECT_CALL(*connection_, connect());
     EXPECT_CALL(*connection_, setConnectionStats(_));
-    EXPECT_CALL(*connection_, noDelay(true));
     EXPECT_CALL(*connection_, streamInfo()).Times(2);
     EXPECT_CALL(*connection_, id()).Times(AnyNumber());
 

diff --git a/test/extensions/filters/network/common/redis/client_impl_test.cc b/test/extensions/filters/network/common/redis/client_impl_test.cc
@@ -75,7 +75,6 @@ class RedisClientImplTest : public testing::Test,
     EXPECT_CALL(*upstream_connection_, addReadFilter(_))
         .WillOnce(SaveArg<0>(&upstream_read_filter_));
     EXPECT_CALL(*upstream_connection_, connect());
-    EXPECT_CALL(*upstream_connection_, noDelay(true));
 
     redis_command_stats_ =
         Common::Redis::RedisCommandStats::createRedisCommandStats(stats_.symbolTable());