listener: add listen() error handling

source/common/init/target_impl.cc

@@ -45,9 +45,11 @@ bool TargetImpl::ready() {
   if (watcher_handle_) {
     // If we have a handle for the ManagerImpl's watcher, signal it and then reset so it can't be
     // accidentally signaled again.
-    const bool result = watcher_handle_->ready();
-    watcher_handle_.reset();
-    return result;
+    // NOTE: We must move watcher_handle_ to a local to avoid the scenario in which as a result of
+    // calling ready() this target is destroyed. This is possible in practice, for example when
+    // a listener is deleted as a result of a failure in the context of the ready() call.
+    auto local_watcher_handle = std::move(watcher_handle_);
+    return local_watcher_handle->ready();
   }
   return false;
 }
@@ -75,12 +77,14 @@ TargetHandlePtr SharedTargetImpl::createHandle(absl::string_view handle_name) co
 
 bool SharedTargetImpl::ready() {
   initialized_ = true;
-  bool all_notified = !watcher_handles_.empty();
-  for (auto& watcher_handle : watcher_handles_) {
+  // NOTE: We must move watcher_handles_ to a local to avoid the scenario in which as a result of
+  // calling ready() this target is destroyed. This is possible in practice, for example when
+  // a listener is deleted as a result of a failure in the context of the ready() call.
+  auto local_watcher_handles = std::move(watcher_handles_);
+  bool all_notified = !local_watcher_handles.empty();
+  for (auto& watcher_handle : local_watcher_handles) {
     all_notified = watcher_handle->ready() && all_notified;
   }
-  // save heap and avoid repeatedly invoke
-  watcher_handles_.clear();
   return all_notified;
 }
 

source/server/admin/admin.cc

@@ -127,8 +127,8 @@ void AdminImpl::startHttpListener(const std::list<AccessLog::InstanceSharedPtr>&
   }
   null_overload_manager_.start();
   socket_ = std::make_shared<Network::TcpListenSocket>(address, socket_options, true);
-  // TODO(mattklein123): We lost error handling along the way for the listen() call. Add it back.
-  socket_->ioHandle().listen(ENVOY_TCP_BACKLOG_SIZE);
+  RELEASE_ASSERT(0 == socket_->ioHandle().listen(ENVOY_TCP_BACKLOG_SIZE).return_value_,
+                 "listen() failed on admin listener");
   socket_factory_ = std::make_unique<AdminListenSocketFactory>(socket_);
   listener_ = std::make_unique<AdminListener>(*this, std::move(listener_scope));
   ENVOY_LOG(info, "admin address: {}", socket().addressProvider().localAddress()->asString());

source/server/listener_impl.cc

@@ -169,10 +169,16 @@ void ListenSocketFactoryImpl::doFinalPreWorkerInit() {
   }
 
   for (auto& socket : sockets_) {
-    // TODO(mattklein123): At some point we lost error handling on this call which I think can
-    // technically fail (at least according to lingering code comments). Add error handling on this
-    // in a follow up.
-    socket->ioHandle().listen(tcp_backlog_size_);
+    const auto rc = socket->ioHandle().listen(tcp_backlog_size_);
+#ifndef WIN32
+    if (rc.return_value_ != 0) {
+      throw EnvoyException(fmt::format("cannot listen() errno={}", rc.errno_));
+    }
+#else
+    // TODO(davinci26): listen() error handling and generally listening on multiple workers
+    // is broken right now. This needs follow up to do something better on Windows.
+    UNREFERENCED_PARAMETER(rc);
+#endif
 
     if (!Network::Socket::applyOptions(socket->options(), *socket,
                                        envoy::config::core::v3::SocketOption::STATE_LISTENING)) {

test/common/quic/envoy_quic_writer_test.cc

@@ -5,16 +5,17 @@
 
 #include "source/common/network/address_impl.h"
 #include "source/common/network/io_socket_error_impl.h"
+#include "source/common/network/io_socket_handle_impl.h"
 #include "source/common/network/udp_packet_writer_handler_impl.h"
 #include "source/common/quic/envoy_quic_packet_writer.h"
 
 #include "test/mocks/api/mocks.h"
-#include "test/mocks/network/mocks.h"
 #include "test/test_common/threadsafe_singleton_injector.h"
 
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
+using testing::_;
 using testing::Return;
 
 namespace Envoy {
@@ -23,7 +24,7 @@ namespace Quic {
 class EnvoyQuicWriterTest : public ::testing::Test {
 public:
   EnvoyQuicWriterTest()
-      : envoy_quic_writer_(std::make_unique<Network::UdpDefaultWriter>(socket_.ioHandle())) {
+      : envoy_quic_writer_(std::make_unique<Network::UdpDefaultWriter>(io_handle_)) {
     self_address_.FromString("::");
     quic::QuicIpAddress peer_ip;
     peer_ip.FromString("::1");
@@ -50,7 +51,7 @@ class EnvoyQuicWriterTest : public ::testing::Test {
 protected:
   testing::NiceMock<Api::MockOsSysCalls> os_sys_calls_;
   TestThreadsafeSingletonInjector<Api::OsSysCallsImpl> os_calls_{&os_sys_calls_};
-  testing::NiceMock<Network::MockListenSocket> socket_;
+  Network::IoSocketHandleImpl io_handle_;
   quic::QuicIpAddress self_address_;
   quic::QuicSocketAddress peer_address_;
   EnvoyQuicPacketWriter envoy_quic_writer_;

test/common/quic/quic_io_handle_wrapper_test.cc

@@ -4,6 +4,7 @@
 #include "envoy/common/platform.h"
 
 #include "source/common/network/address_impl.h"
+#include "source/common/network/io_socket_handle_impl.h"
 #include "source/common/quic/quic_io_handle_wrapper.h"
 
 #include "test/mocks/api/mocks.h"
@@ -16,19 +17,23 @@
 
 using testing::ByMove;
 using testing::Return;
+using testing::ReturnRef;
 
 namespace Envoy {
 namespace Quic {
 
 class QuicIoHandleWrapperTest : public testing::Test {
 public:
-  QuicIoHandleWrapperTest() : wrapper_(std::make_unique<QuicIoHandleWrapper>(socket_.ioHandle())) {
+  QuicIoHandleWrapperTest() {
+    real_io_handle_ = std::make_unique<Network::IoSocketHandleImpl>();
+    ON_CALL(socket_, ioHandle()).WillByDefault(ReturnRef(*real_io_handle_));
+    wrapper_ = std::make_unique<QuicIoHandleWrapper>(socket_.ioHandle());
     EXPECT_TRUE(wrapper_->isOpen());
     EXPECT_FALSE(socket_.ioHandle().isOpen());
   }
-  ~QuicIoHandleWrapperTest() override = default;
 
 protected:
+  Network::IoHandlePtr real_io_handle_;
   testing::NiceMock<Network::MockListenSocket> socket_;
   std::unique_ptr<QuicIoHandleWrapper> wrapper_;
   testing::StrictMock<Envoy::Api::MockOsSysCalls> os_sys_calls_;

test/mocks/network/BUILD

@@ -47,6 +47,7 @@ envoy_cc_mock(
     hdrs = ["mocks.h"],
     deps = [
         ":connection_mocks",
+        ":io_handle_mocks",
         ":transport_socket_mocks",
         "//envoy/buffer:buffer_interface",
         "//envoy/network:connection_interface",

test/mocks/network/mocks.cc

@@ -132,7 +132,7 @@ MockFilterChainFactory::MockFilterChainFactory() {
 MockFilterChainFactory::~MockFilterChainFactory() = default;
 
 MockListenSocket::MockListenSocket()
-    : io_handle_(std::make_unique<IoSocketHandleImpl>()),
+    : io_handle_(std::make_unique<NiceMock<MockIoHandle>>()),
       address_provider_(std::make_shared<SocketAddressSetterImpl>(
           std::make_shared<Address::Ipv4Instance>(80), nullptr)) {
   ON_CALL(*this, options()).WillByDefault(ReturnRef(options_));

test/mocks/network/mocks.h

@@ -25,6 +25,7 @@
 
 #include "test/mocks/event/mocks.h"
 #include "test/mocks/network/connection.h"
+#include "test/mocks/network/io_handle.h"
 #include "test/mocks/stream_info/mocks.h"
 #include "test/test_common/printers.h"
 
@@ -270,7 +271,7 @@ class MockListenSocket : public Socket {
               (unsigned long, void*, unsigned long, void*, unsigned long, unsigned long*));
   MOCK_METHOD(Api::SysCallIntResult, setBlockingForTest, (bool));
 
-  IoHandlePtr io_handle_;
+  std::unique_ptr<MockIoHandle> io_handle_;
   Network::SocketAddressSetterSharedPtr address_provider_;
   OptionsSharedPtr options_;
   bool socket_is_open_ = true;

test/mocks/server/BUILD

@@ -121,6 +121,7 @@ envoy_cc_mock(
     deps = [
         "//envoy/server:drain_manager_interface",
         "//envoy/server:listener_manager_interface",
+        "//test/mocks/network:io_handle_mocks",
         "//test/mocks/network:network_mocks",
         "@envoy_api//envoy/config/core/v3:pkg_cc_proto",
         "@envoy_api//envoy/config/listener/v3:pkg_cc_proto",

test/mocks/tcp/BUILD

@@ -16,6 +16,7 @@ envoy_cc_mock(
         "//envoy/buffer:buffer_interface",
         "//envoy/tcp:conn_pool_interface",
         "//test/mocks:common_lib",
+        "//test/mocks/network:io_handle_mocks",
         "//test/mocks/network:network_mocks",
         "//test/mocks/upstream:host_mocks",
     ],

test/server/listener_manager_impl_test.cc

@@ -1837,6 +1837,41 @@ TEST_F(ListenerManagerImplTest, NotSupportedDatagramUds) {
                             "socket type SocketType::Datagram not supported for pipes");
 }
 
+// TODO(davinci26): See ListenSocketFactoryImpl::doFinalPreWorkerInit() for why this test is
+// not run on Windows.
+#ifndef WIN32
+TEST_F(ListenerManagerImplTest, CantListen) {
+  InSequence s;
+
+  EXPECT_CALL(*worker_, start(_, _));
+  manager_->startWorkers(guard_dog_, callback_.AsStdFunction());
+
+  const std::string listener_foo_yaml = R"EOF(
+name: foo
+address:
+  socket_address:
+    address: 127.0.0.1
+    port_value: 1234
+filter_chains:
+- filters: []
+  )EOF";
+
+  ListenerHandle* listener_foo = expectListenerCreate(true, true);
+  EXPECT_CALL(listener_factory_, createListenSocket(_, _, _, default_bind_type, 0));
+  EXPECT_CALL(listener_foo->target_, initialize());
+  manager_->addOrUpdateListener(parseListenerFromV3Yaml(listener_foo_yaml), "", true);
+
+  EXPECT_CALL(*listener_factory_.socket_->io_handle_, listen(_))
+      .WillOnce(Return(Api::SysCallIntResult{-1, 100}));
+  EXPECT_CALL(*listener_foo, onDestroy());
+  listener_foo->target_.ready();
+
+  EXPECT_EQ(
+      1UL,
+      server_.stats_store_.counterFromString("listener_manager.listener_create_failure").value());
+}
+#endif
+
 TEST_F(ListenerManagerImplTest, CantBindSocket) {
   time_system_.setSystemTime(std::chrono::milliseconds(1001001001001));
   InSequence s;