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
@@ -9,6 +9,8 @@ Minor Behavior Changes
 ----------------------
 *Changes that may cause incompatibilities for some users, but should not for most*
 
+* 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.
+
 Bug Fixes
 ---------
 *Changes expected to improve the state of the world and are unlikely to have negative effects*
@@ -21,4 +23,4 @@ New Features
 ------------
 
 Deprecated
-----------
+----------
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
@@ -20,6 +20,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 {
@@ -781,7 +782,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_) {
@@ -860,6 +865,9 @@ void ClientConnectionImpl::connect() {
                  socket_->addressProvider().remoteAddress()->asString());
   const Api::SysCallIntResult result = socket_->connect(socket_->addressProvider().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
@@ -526,7 +526,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"
@@ -589,7 +590,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
@@ -122,6 +122,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> {
@@ -399,11 +400,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), _));
@@ -418,10 +421,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);
@@ -436,11 +440,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;
@@ -598,7 +603,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());

diff --git a/test/extensions/quic_listeners/quiche/integration/quic_http_integration_test.cc b/test/extensions/quic_listeners/quiche/integration/quic_http_integration_test.cc
@@ -310,6 +310,9 @@ TEST_P(QuicHttpIntegrationTest, GetRequestAndEmptyResponse) {
 }
 
 TEST_P(QuicHttpIntegrationTest, GetRequestAndResponseWithBody) {
+  // Use the old nodelay in a random test for coverage. nodelay is a no-op for QUIC.
+  config_helper_.addRuntimeOverride("envoy.reloadable_features.always_nodelay", "false");
+
   initialize();
   sendRequestAndVerifyResponse(default_request_headers_, /*request_size=*/0,
                                default_response_headers_, /*response_size=*/1024,

diff --git a/test/integration/integration_test.cc b/test/integration/integration_test.cc
@@ -1261,6 +1261,14 @@ TEST_P(IntegrationTest, ViaAppendWith100Continue) {
   testEnvoyHandling100Continue(false, "foo");
 }
 
+// Pick a random test and use the old nodelay for coverage. This test can be
+// removed when the code path is removed.
+TEST_P(IntegrationTest, ViaAppendWith100ContinueWithOldNodelay) {
+  config_helper_.addRuntimeOverride("envoy.reloadable_features.always_nodelay", "false");
+  config_helper_.addConfigModifier(setVia("foo"));
+  testEnvoyHandling100Continue(false, "foo");
+}
+
 // Test delayed close semantics for downstream HTTP/1.1 connections. When an early response is
 // sent by Envoy, it will wait for response acknowledgment (via FIN/RST) from the client before
 // closing the socket (with a timeout for ensuring cleanup).

diff --git a/test/integration/tcp_proxy_integration_test.cc b/test/integration/tcp_proxy_integration_test.cc
@@ -99,6 +99,8 @@ TEST_P(TcpProxyIntegrationTest, TcpProxyUpstreamWritesFirst) {
 
 // Test TLS upstream.
 TEST_P(TcpProxyIntegrationTest, TcpProxyUpstreamTls) {
+  // Make sure old style nodelay is covered in at least one integration test.
+  config_helper_.addRuntimeOverride("envoy.reloadable_features.always_nodelay", "false");
   upstream_tls_ = true;
   setUpstreamProtocol(FakeHttpConnection::Type::HTTP1);
   config_helper_.configureUpstreamTls();