ClientConnectionImpl: Delay setTransportSocketCallbacks() until end of the constructor.

source/common/event/dispatcher_impl.cc

@@ -77,8 +77,8 @@ Network::ConnectionPtr
 DispatcherImpl::createServerConnection(Network::ConnectionSocketPtr&& socket,
                                        Network::TransportSocketPtr&& transport_socket) {
   ASSERT(isThreadSafe());
-  return std::make_unique<Network::ConnectionImpl>(*this, std::move(socket),
-                                                   std::move(transport_socket), true);
+  return Network::ConnectionImpl::createServerConnection(*this, std::move(socket),
+                                                         std::move(transport_socket));
 }
 
 Network::ClientConnectionPtr
@@ -87,8 +87,8 @@ DispatcherImpl::createClientConnection(Network::Address::InstanceConstSharedPtr
                                        Network::TransportSocketPtr&& transport_socket,
                                        const Network::ConnectionSocket::OptionsSharedPtr& options) {
   ASSERT(isThreadSafe());
-  return std::make_unique<Network::ClientConnectionImpl>(*this, address, source_address,
-                                                         std::move(transport_socket), options);
+  return Network::ClientConnectionImpl::createClientConnection(
+      *this, address, source_address, std::move(transport_socket), options);
 }
 
 Network::DnsResolverSharedPtr DispatcherImpl::createDnsResolver(

source/common/network/connection_impl.cc

@@ -44,6 +44,16 @@ void ConnectionImplUtility::updateBufferStats(uint64_t delta, uint64_t new_total
 
 std::atomic<uint64_t> ConnectionImpl::next_global_id_;
 
+std::unique_ptr<ConnectionImpl>
+ConnectionImpl::createServerConnection(Event::Dispatcher& dispatcher, ConnectionSocketPtr&& socket,
+                                       TransportSocketPtr&& transport_socket) {
+  // make_shared<> requires a public constructor so we can't use it here.
+  std::unique_ptr<Network::ConnectionImpl> conn(new Network::ConnectionImpl(
+      dispatcher, std::move(socket), std::move(transport_socket), true));
+  conn->transport_socket_->setTransportSocketCallbacks(*conn);
+  return conn;
+}
+
 ConnectionImpl::ConnectionImpl(Event::Dispatcher& dispatcher, ConnectionSocketPtr&& socket,
                                TransportSocketPtr&& transport_socket, bool connected)
     : transport_socket_(std::move(transport_socket)), filter_manager_(*this, *this),
@@ -65,8 +75,6 @@ ConnectionImpl::ConnectionImpl(Event::Dispatcher& dispatcher, ConnectionSocketPt
   file_event_ = dispatcher_.createFileEvent(
       fd(), [this](uint32_t events) -> void { onFileEvent(events); }, Event::FileTriggerType::Edge,
       Event::FileReadyType::Read | Event::FileReadyType::Write);
-
-  transport_socket_->setTransportSocketCallbacks(*this);
 }
 
 ConnectionImpl::~ConnectionImpl() {
@@ -593,6 +601,18 @@ void ConnectionImpl::onDelayedCloseTimeout() {
   closeSocket(ConnectionEvent::LocalClose);
 }
 
+std::unique_ptr<ClientConnectionImpl> ClientConnectionImpl::createClientConnection(
+    Event::Dispatcher& dispatcher, const Address::InstanceConstSharedPtr& remote_address,
+    const Address::InstanceConstSharedPtr& source_address,
+    Network::TransportSocketPtr&& transport_socket,
+    const Network::ConnectionSocket::OptionsSharedPtr& options) {
+  // make_shared<> requires a public constructor so we can't use it here.
+  std::unique_ptr<Network::ClientConnectionImpl> conn(new Network::ClientConnectionImpl(
+      dispatcher, remote_address, source_address, std::move(transport_socket), options));
+  conn->transport_socket_->setTransportSocketCallbacks(*conn);
+  return conn;
+}
+
 ClientConnectionImpl::ClientConnectionImpl(
     Event::Dispatcher& dispatcher, const Address::InstanceConstSharedPtr& remote_address,
     const Network::Address::InstanceConstSharedPtr& source_address,

source/common/network/connection_impl.h

@@ -51,10 +51,14 @@ class ConnectionImpl : public virtual Connection,
                        public BufferSource,
                        public TransportSocketCallbacks,
                        protected Logger::Loggable<Logger::Id::connection> {
-public:
+protected:
   ConnectionImpl(Event::Dispatcher& dispatcher, ConnectionSocketPtr&& socket,
                  TransportSocketPtr&& transport_socket, bool connected);
 
+public:
+  static std::unique_ptr<ConnectionImpl>
+  createServerConnection(Event::Dispatcher& dispatcher, ConnectionSocketPtr&& socket,
+                         TransportSocketPtr&& transport_socket);
   ~ConnectionImpl();
 
   // Network::FilterManager
@@ -200,13 +204,21 @@ class ConnectionImpl : public virtual Connection,
  * libevent implementation of Network::ClientConnection.
  */
 class ClientConnectionImpl : public ConnectionImpl, virtual public ClientConnection {
-public:
+protected:
   ClientConnectionImpl(Event::Dispatcher& dispatcher,
                        const Address::InstanceConstSharedPtr& remote_address,
                        const Address::InstanceConstSharedPtr& source_address,
                        Network::TransportSocketPtr&& transport_socket,
                        const Network::ConnectionSocket::OptionsSharedPtr& options);
 
+public:
+  static std::unique_ptr<ClientConnectionImpl>
+  createClientConnection(Event::Dispatcher& dispatcher,
+                         const Address::InstanceConstSharedPtr& remote_address,
+                         const Address::InstanceConstSharedPtr& source_address,
+                         Network::TransportSocketPtr&& transport_socket,
+                         const Network::ConnectionSocket::OptionsSharedPtr& options);
+
   // Network::ClientConnection
   void connect() override;
 };

source/server/config_validation/BUILD

@@ -54,7 +54,10 @@ envoy_cc_library(
 
 envoy_cc_library(
     name = "dispatcher_lib",
-    srcs = ["dispatcher.cc"],
+    srcs = [
+        "connection.cc",
+        "dispatcher.cc",
+    ],
     hdrs = [
         "connection.h",
         "dispatcher.h",

source/server/config_validation/connection.cc

@@ -0,0 +1,20 @@
+#include "server/config_validation/connection.h"
+
+namespace Envoy {
+namespace Network {
+
+std::unique_ptr<ConfigValidateConnection>
+ConfigValidateConnection::create(Event::ValidationDispatcher& dispatcher,
+                                 const Address::InstanceConstSharedPtr& remote_address,
+                                 const Address::InstanceConstSharedPtr& source_address,
+                                 Network::TransportSocketPtr&& transport_socket,
+                                 const Network::ConnectionSocket::OptionsSharedPtr& options) {
+  // make_shared<> requires a public constructor so we can't use it here.
+  std::unique_ptr<ConfigValidateConnection> conn(new ConfigValidateConnection(
+      dispatcher, remote_address, source_address, std::move(transport_socket), options));
+  conn->transport_socket_->setTransportSocketCallbacks(*conn);
+  return conn;
+}
+
+} // namespace Network
+} // namespace Envoy

source/server/config_validation/connection.h

@@ -13,7 +13,7 @@ namespace Network {
   overridden with no-op implementations.
 */
 class ConfigValidateConnection : public Network::ClientConnectionImpl {
-public:
+private:
   ConfigValidateConnection(Event::ValidationDispatcher& dispatcher,
                            Network::Address::InstanceConstSharedPtr remote_address,
                            Network::Address::InstanceConstSharedPtr source_address,
@@ -22,6 +22,14 @@ class ConfigValidateConnection : public Network::ClientConnectionImpl {
       : Network::ClientConnectionImpl(dispatcher, remote_address, source_address,
                                       std::move(transport_socket), options) {}
 
+public:
+  static std::unique_ptr<ConfigValidateConnection>
+  create(Event::ValidationDispatcher& dispatcher,
+         const Address::InstanceConstSharedPtr& remote_address,
+         const Address::InstanceConstSharedPtr& source_address,
+         Network::TransportSocketPtr&& transport_socket,
+         const Network::ConnectionSocket::OptionsSharedPtr& options);
+
   // Unit tests may instantiate it without proper event machine and leave opened sockets.
   // Do some cleanup before invoking base class's destructor.
   virtual ~ConfigValidateConnection() { close(ConnectionCloseType::NoFlush); }

source/server/config_validation/dispatcher.cc

@@ -12,8 +12,8 @@ Network::ClientConnectionPtr ValidationDispatcher::createClientConnection(
     Network::Address::InstanceConstSharedPtr source_address,
     Network::TransportSocketPtr&& transport_socket,
     const Network::ConnectionSocket::OptionsSharedPtr& options) {
-  return std::make_unique<Network::ConfigValidateConnection>(*this, remote_address, source_address,
-                                                             std::move(transport_socket), options);
+  return Network::ConfigValidateConnection::create(*this, remote_address, source_address,
+                                                   std::move(transport_socket), options);
 }
 
 Network::DnsResolverSharedPtr ValidationDispatcher::createDnsResolver(

test/common/grpc/grpc_client_integration_test_harness.h

@@ -262,7 +262,7 @@ class GrpcClientIntegrationTest : public GrpcClientIntegrationParamTest {
   // Create a Grpc::AsyncClientImpl instance backed by enough fake/mock
   // infrastructure to initiate a loopback TCP connection to fake_upstream_.
   AsyncClientPtr createAsyncClientImpl() {
-    client_connection_ = std::make_unique<Network::ClientConnectionImpl>(
+    client_connection_ = Network::ClientConnectionImpl::createClientConnection(
         dispatcher_, fake_upstream_->localAddress(), nullptr,
         std::move(async_client_transport_socket_), nullptr);
     ON_CALL(*mock_cluster_info_, connectTimeout())

test/common/network/connection_impl_test.cc

@@ -78,10 +78,11 @@ INSTANTIATE_TEST_CASE_P(IpVersions, ConnectionImplDeathTest,
 TEST_P(ConnectionImplDeathTest, BadFd) {
   Event::SimulatedTimeSystem time_system;
   Event::DispatcherImpl dispatcher(time_system);
-  EXPECT_DEATH_LOG_TO_STDERR(
-      ConnectionImpl(dispatcher, std::make_unique<ConnectionSocketImpl>(-1, nullptr, nullptr),
-                     Network::Test::createRawBufferSocket(), false),
-      ".*assert failure: fd\\(\\) != -1.*");
+  EXPECT_DEATH_LOG_TO_STDERR(ConnectionImpl::createServerConnection(
+                                 dispatcher,
+                                 std::make_unique<ConnectionSocketImpl>(-1, nullptr, nullptr),
+                                 Network::Test::createRawBufferSocket()),
+                             ".*assert failure: fd\\(\\) != -1.*");
 }
 
 class ConnectionImplTest : public testing::TestWithParam<Address::IpVersion> {
@@ -955,9 +956,9 @@ TEST_P(ConnectionImplTest, FlushWriteCloseTest) {
 // triggered.
 TEST_P(ConnectionImplTest, FlushWriteCloseTimeoutTest) {
   ConnectionMocks mocks = createConnectionMocks();
-  auto server_connection = std::make_unique<Network::ConnectionImpl>(
+  auto server_connection = ConnectionImpl::createServerConnection(
       *mocks.dispatcher, std::make_unique<ConnectionSocketImpl>(0, nullptr, nullptr),
-      std::move(mocks.transport_socket), true);
+      std::move(mocks.transport_socket));
 
   InSequence s1;
 
@@ -1080,9 +1081,9 @@ TEST_P(ConnectionImplTest, FlushWriteAndDelayConfigDisabledTest) {
                                 std::function<void()> above_high) -> Buffer::Instance* {
         return new Buffer::WatermarkBuffer(below_low, above_high);
       }));
-  std::unique_ptr<Network::ConnectionImpl> server_connection(new Network::ConnectionImpl(
+  auto server_connection = ConnectionImpl::createServerConnection(
       dispatcher, std::make_unique<ConnectionSocketImpl>(0, nullptr, nullptr),
-      std::make_unique<NiceMock<MockTransportSocket>>(), true));
+      std::make_unique<NiceMock<MockTransportSocket>>());
 
   time_system_.setMonotonicTime(std::chrono::milliseconds(0));
 
@@ -1108,9 +1109,9 @@ TEST_P(ConnectionImplTest, FlushWriteAndDelayConfigDisabledTest) {
 // Test that tearing down the connection will disable the delayed close timer.
 TEST_P(ConnectionImplTest, DelayedCloseTimeoutDisableOnSocketClose) {
   ConnectionMocks mocks = createConnectionMocks();
-  auto server_connection = std::make_unique<Network::ConnectionImpl>(
+  auto server_connection = ConnectionImpl::createServerConnection(
       *mocks.dispatcher, std::make_unique<ConnectionSocketImpl>(0, nullptr, nullptr),
-      std::move(mocks.transport_socket), true);
+      std::move(mocks.transport_socket));
 
   InSequence s1;
 
@@ -1132,9 +1133,9 @@ TEST_P(ConnectionImplTest, DelayedCloseTimeoutDisableOnSocketClose) {
 // Test that the delayed close timeout callback is resilient to connection teardown edge cases.
 TEST_P(ConnectionImplTest, DelayedCloseTimeoutNullStats) {
   ConnectionMocks mocks = createConnectionMocks();
-  auto server_connection = std::make_unique<Network::ConnectionImpl>(
+  auto server_connection = ConnectionImpl::createServerConnection(
       *mocks.dispatcher, std::make_unique<ConnectionSocketImpl>(0, nullptr, nullptr),
-      std::move(mocks.transport_socket), true);
+      std::move(mocks.transport_socket));
 
   InSequence s1;
 
@@ -1180,9 +1181,9 @@ class MockTransportConnectionImplTest : public testing::Test {
         .WillOnce(Invoke([this](TransportSocketCallbacks& callbacks) {
           transport_socket_callbacks_ = &callbacks;
         }));
-    connection_ = std::make_unique<ConnectionImpl>(
+    connection_ = ConnectionImpl::createServerConnection(
         dispatcher_, std::make_unique<ConnectionSocketImpl>(0, nullptr, nullptr),
-        TransportSocketPtr(transport_socket_), true);
+        TransportSocketPtr(transport_socket_));
     connection_->addConnectionCallbacks(callbacks_);
   }