network: rolling back turned-off nodelay code (#15102)

Risk Level: low (removing unused code)
Testing: n/a
Docs Changes: n/a
Release Notes: n/a

Signed-off-by: Alyssa Wilk <[email protected]>

source/common/http/codec_client.cc

@@ -49,9 +49,7 @@ 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.
-  if (!Runtime::runtimeFeatureEnabled("envoy.reloadable_features.always_nodelay")) {
-    connection_->noDelay(true);
-  }
+  connection_->noDelay(true);
 }
 
 CodecClient::~CodecClient() = default;

source/common/network/connection_impl.cc

@@ -21,7 +21,6 @@
 #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 {
@@ -777,11 +776,7 @@ 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) {
-  if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.always_nodelay")) {
-    noDelay(true);
-  }
-}
+                     connected) {}
 
 void ServerConnectionImpl::setTransportSocketConnectTimeout(std::chrono::milliseconds timeout) {
   if (!transport_connect_pending_) {
@@ -861,9 +856,6 @@ 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 {

source/common/runtime/runtime_features.cc

@@ -99,8 +99,6 @@ constexpr const char* runtime_features[] = {
 // When features are added here, there should be a tracking bug assigned to the
 // code owner to flip the default after sufficient testing.
 constexpr const char* disabled_runtime_features[] = {
-    // TODO(alyssawilk) either sort out throughput changes or revert this as low-priority
-    "envoy.reloadable_features.always_nodelay",
     // v2 is fatal-by-default.
     "envoy.reloadable_features.enable_deprecated_v2_api",
     // Allow Envoy to upgrade or downgrade version of type url, should be removed when support for

source/common/tcp/conn_pool.cc

@@ -6,7 +6,6 @@
 #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,10 +30,7 @@ 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});
-
-  if (!Runtime::runtimeFeatureEnabled("envoy.reloadable_features.always_nodelay")) {
-    connection_->noDelay(true);
-  }
+  connection_->noDelay(true);
   connection_->connect();
 }
 

source/common/tcp/original_conn_pool.cc

@@ -6,7 +6,6 @@
 #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"
 
@@ -401,9 +400,7 @@ OriginalConnPoolImpl::ActiveConn::ActiveConn(OriginalConnPoolImpl& parent)
 
   // We just universally set no delay on connections. Theoretically we might at some point want
   // to make this configurable.
-  if (!Runtime::runtimeFeatureEnabled("envoy.reloadable_features.always_nodelay")) {
-    conn_->noDelay(true);
-  }
+  conn_->noDelay(true);
 }
 
 OriginalConnPoolImpl::ActiveConn::~ActiveConn() {

source/common/upstream/health_checker_impl.cc

@@ -554,9 +554,7 @@ void TcpHealthCheckerImpl::TcpActiveHealthCheckSession::onInterval() {
 
     expect_close_ = false;
     client_->connect();
-    if (!Runtime::runtimeFeatureEnabled("envoy.reloadable_features.always_nodelay")) {
-      client_->noDelay(true);
-    }
+    client_->noDelay(true);
   }
 
   if (!parent_.send_bytes_.empty()) {

source/extensions/filters/network/common/redis/client_impl.cc

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

source/server/connection_handler_impl.cc

@@ -12,7 +12,6 @@
 #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,9 +590,7 @@ 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.
-  if (!Runtime::runtimeFeatureEnabled("envoy.reloadable_features.always_nodelay")) {
-    connection_->noDelay(true);
-  }
+  connection_->noDelay(true);
   auto& listener = active_connections_.listener_;
   listener.stats_.downstream_cx_total_.inc();
   listener.stats_.downstream_cx_active_.inc();

test/common/network/BUILD

@@ -86,7 +86,6 @@ 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",

test/common/network/connection_impl_test.cc

@@ -123,7 +123,6 @@ 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> {
@@ -401,16 +400,14 @@ 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>();
   EXPECT_CALL(*mocks.dispatcher_,
               createScaledTypedTimer_(Event::ScaledTimerType::TransportSocketConnectTimeout, _))
       .WillOnce(DoAll(SaveArg<1>(&mock_timer->callback_), Return(mock_timer)));
   auto server_connection = std::make_unique<Network::ServerConnectionImpl>(
       *mocks.dispatcher_,
-      std::make_unique<ConnectionSocketImpl>(std::move(io_handle), local_addr, nullptr),
+      std::make_unique<ConnectionSocketImpl>(std::move(io_handle), nullptr, nullptr),
       std::move(mocks.transport_socket_), stream_info_, true);
 
   EXPECT_CALL(*mock_timer, enableTimer(std::chrono::milliseconds(3 * 1000), _));
@@ -425,11 +422,10 @@ 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), local_addr, nullptr),
+      std::make_unique<ConnectionSocketImpl>(std::move(io_handle), nullptr, nullptr),
       std::move(mocks.transport_socket_), stream_info_, true);
 
   transport_socket->callbacks_->raiseEvent(ConnectionEvent::Connected);
@@ -444,15 +440,14 @@ 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>();
   EXPECT_CALL(*mocks.dispatcher_,
               createScaledTypedTimer_(Event::ScaledTimerType::TransportSocketConnectTimeout, _))
       .WillOnce(DoAll(SaveArg<1>(&mock_timer->callback_), Return(mock_timer)));
   auto server_connection = std::make_unique<Network::ServerConnectionImpl>(
       *mocks.dispatcher_,
-      std::make_unique<ConnectionSocketImpl>(std::move(io_handle), local_addr, nullptr),
+      std::make_unique<ConnectionSocketImpl>(std::move(io_handle), nullptr, nullptr),
       std::move(mocks.transport_socket_), stream_info_, true);
 
   bool timer_destroyed = false;
@@ -610,24 +605,7 @@ 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());
@@ -1906,19 +1884,22 @@ TEST_P(ConnectionImplTest, DelayedCloseTimeoutNullStats) {
 }
 
 // Test DumpState methods.
-TEST_P(ConnectionImplTest, NetworkAndPipeSocketDumpsWithoutAllocatingMemory) {
+TEST_P(ConnectionImplTest, NetworkSocketDumpsWithoutAllocatingMemory) {
   std::array<char, 1024> buffer;
   OutputBufferStream ostream{buffer.data(), buffer.size()};
   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");
   Address::InstanceConstSharedPtr server_addr;
+  Address::InstanceConstSharedPtr local_addr;
   if (GetParam() == Network::Address::IpVersion::v4) {
     server_addr = Network::Address::InstanceConstSharedPtr{
         new Network::Address::Ipv4Instance("1.1.1.1", 80, nullptr)};
+    local_addr = Network::Address::InstanceConstSharedPtr{
+        new Network::Address::Ipv4Instance("1.2.3.4", 56789, nullptr)};
   } else {
     server_addr = Network::Address::InstanceConstSharedPtr{
         new Network::Address::Ipv6Instance("::1", 80, nullptr)};
+    local_addr = Network::Address::InstanceConstSharedPtr{
+        new Network::Address::Ipv6Instance("::1:2:3:4", 56789, nullptr)};
   }
 
   auto connection_socket =
@@ -1940,12 +1921,12 @@ TEST_P(ConnectionImplTest, NetworkAndPipeSocketDumpsWithoutAllocatingMemory) {
         contents,
         HasSubstr(
             "remote_address_: 1.1.1.1:80, direct_remote_address_: 1.1.1.1:80, local_address_: "
-            "/pipe/path"));
+            "1.2.3.4:56789"));
   } else {
     EXPECT_THAT(
         contents,
         HasSubstr("remote_address_: [::1]:80, direct_remote_address_: [::1]:80, local_address_: "
-                  "/pipe/path"));
+                  "[::1:2:3:4]:56789"));
   }
 }
 
@@ -1954,11 +1935,10 @@ TEST_P(ConnectionImplTest, NetworkConnectionDumpsWithoutAllocatingMemory) {
   OutputBufferStream ostream{buffer.data(), buffer.size()};
   ConnectionMocks mocks = createConnectionMocks(false);
   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), local_addr, nullptr),
+      std::make_unique<ConnectionSocketImpl>(std::move(io_handle), nullptr, nullptr),
       std::move(mocks.transport_socket_), stream_info_, true);
 
   // Start measuring memory and dump state.

test/extensions/filters/network/common/redis/BUILD

@@ -58,7 +58,6 @@ envoy_cc_test(
         "//test/mocks/thread_local:thread_local_mocks",
         "//test/mocks/upstream:host_mocks",
         "//test/test_common:simulated_time_system_lib",
-        "//test/test_common:test_runtime_lib",
         "@envoy_api//envoy/extensions/filters/network/redis_proxy/v3:pkg_cc_proto",
     ],
 )

test/extensions/filters/network/common/redis/client_impl_test.cc

@@ -14,7 +14,6 @@
 #include "test/mocks/network/mocks.h"
 #include "test/mocks/upstream/host.h"
 #include "test/test_common/simulated_time_system.h"
-#include "test/test_common/test_runtime.h"
 
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
@@ -76,9 +75,8 @@ class RedisClientImplTest : public testing::Test,
     EXPECT_CALL(*upstream_connection_, addReadFilter(_))
         .WillOnce(SaveArg<0>(&upstream_read_filter_));
     EXPECT_CALL(*upstream_connection_, connect());
-    if (legacy_nodelay_) {
-      EXPECT_CALL(*upstream_connection_, noDelay(true));
-    }
+    EXPECT_CALL(*upstream_connection_, noDelay(true));
+
     redis_command_stats_ =
         Common::Redis::RedisCommandStats::createRedisCommandStats(stats_.symbolTable());
 
@@ -147,7 +145,6 @@ class RedisClientImplTest : public testing::Test,
   Common::Redis::RedisCommandStatsSharedPtr redis_command_stats_;
   std::string auth_username_;
   std::string auth_password_;
-  bool legacy_nodelay_{};
 };
 
 TEST_F(RedisClientImplTest, BatchWithZeroBufferAndTimeout) {
@@ -341,62 +338,6 @@ TEST_F(RedisClientImplTest, Basic) {
   client_->close();
 }
 
-TEST_F(RedisClientImplTest, BasicLegacyNodelay) {
-  legacy_nodelay_ = true;
-  TestScopedRuntime scoped_runtime;
-  Runtime::LoaderSingleton::getExisting()->mergeValues(
-      {{"envoy.reloadable_features.always_nodelay", "false"}});
-  InSequence s;
-
-  setup();
-
-  client_->initialize(auth_username_, auth_password_);
-
-  Common::Redis::RespValue request1;
-  MockClientCallbacks callbacks1;
-  EXPECT_CALL(*encoder_, encode(Ref(request1), _));
-  EXPECT_CALL(*flush_timer_, enabled()).WillOnce(Return(false));
-  PoolRequest* handle1 = client_->makeRequest(request1, callbacks1);
-  EXPECT_NE(nullptr, handle1);
-
-  onConnected();
-
-  Common::Redis::RespValue request2;
-  MockClientCallbacks callbacks2;
-  EXPECT_CALL(*encoder_, encode(Ref(request2), _));
-  EXPECT_CALL(*flush_timer_, enabled()).WillOnce(Return(false));
-  PoolRequest* handle2 = client_->makeRequest(request2, callbacks2);
-  EXPECT_NE(nullptr, handle2);
-
-  EXPECT_EQ(2UL, host_->cluster_.stats_.upstream_rq_total_.value());
-  EXPECT_EQ(2UL, host_->cluster_.stats_.upstream_rq_active_.value());
-  EXPECT_EQ(2UL, host_->stats_.rq_total_.value());
-  EXPECT_EQ(2UL, host_->stats_.rq_active_.value());
-
-  Buffer::OwnedImpl fake_data;
-  EXPECT_CALL(*decoder_, decode(Ref(fake_data))).WillOnce(Invoke([&](Buffer::Instance&) -> void {
-    InSequence s;
-    Common::Redis::RespValuePtr response1(new Common::Redis::RespValue());
-    EXPECT_CALL(callbacks1, onResponse_(Ref(response1)));
-    EXPECT_CALL(*connect_or_op_timer_, enableTimer(_, _));
-    EXPECT_CALL(host_->outlier_detector_,
-                putResult(Upstream::Outlier::Result::ExtOriginRequestSuccess, _));
-    callbacks_->onRespValue(std::move(response1));
-
-    Common::Redis::RespValuePtr response2(new Common::Redis::RespValue());
-    EXPECT_CALL(callbacks2, onResponse_(Ref(response2)));
-    EXPECT_CALL(*connect_or_op_timer_, disableTimer());
-    EXPECT_CALL(host_->outlier_detector_,
-                putResult(Upstream::Outlier::Result::ExtOriginRequestSuccess, _));
-    callbacks_->onRespValue(std::move(response2));
-  }));
-  upstream_read_filter_->onData(fake_data, false);
-
-  EXPECT_CALL(*upstream_connection_, close(Network::ConnectionCloseType::NoFlush));
-  EXPECT_CALL(*connect_or_op_timer_, disableTimer());
-  client_->close();
-}
-
 class ConfigEnableCommandStats : public Config {
   bool disableOutlierEvents() const override { return false; }
   std::chrono::milliseconds opTimeout() const override { return std::chrono::milliseconds(25); }

test/extensions/quic_listeners/quiche/integration/quic_http_integration_test.cc

@@ -310,9 +310,6 @@ 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,