diff --git a/source/common/init/target_impl.cc b/source/common/init/target_impl.cc index 1c8ddfdea5cec..5e41e62f409c2 100644 --- a/source/common/init/target_impl.cc +++ b/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; } diff --git a/source/server/admin/admin.cc b/source/server/admin/admin.cc index 314e2c298bf86..b54e9c8c4077e 100644 --- a/source/server/admin/admin.cc +++ b/source/server/admin/admin.cc @@ -127,8 +127,8 @@ void AdminImpl::startHttpListener(const std::list& } null_overload_manager_.start(); socket_ = std::make_shared(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(socket_); listener_ = std::make_unique(*this, std::move(listener_scope)); ENVOY_LOG(info, "admin address: {}", socket().addressProvider().localAddress()->asString()); diff --git a/source/server/listener_impl.cc b/source/server/listener_impl.cc index 92d10db186dc8..cd36b26627376 100644 --- a/source/server/listener_impl.cc +++ b/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)) { diff --git a/test/common/quic/envoy_quic_writer_test.cc b/test/common/quic/envoy_quic_writer_test.cc index a0ee8b3ed5edd..3908fb82ba568 100644 --- a/test/common/quic/envoy_quic_writer_test.cc +++ b/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(socket_.ioHandle())) { + : envoy_quic_writer_(std::make_unique(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 os_sys_calls_; TestThreadsafeSingletonInjector os_calls_{&os_sys_calls_}; - testing::NiceMock socket_; + Network::IoSocketHandleImpl io_handle_; quic::QuicIpAddress self_address_; quic::QuicSocketAddress peer_address_; EnvoyQuicPacketWriter envoy_quic_writer_; diff --git a/test/common/quic/quic_io_handle_wrapper_test.cc b/test/common/quic/quic_io_handle_wrapper_test.cc index 899f7903d5892..e9c7d566207ed 100644 --- a/test/common/quic/quic_io_handle_wrapper_test.cc +++ b/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(socket_.ioHandle())) { + QuicIoHandleWrapperTest() { + real_io_handle_ = std::make_unique(); + ON_CALL(socket_, ioHandle()).WillByDefault(ReturnRef(*real_io_handle_)); + wrapper_ = std::make_unique(socket_.ioHandle()); EXPECT_TRUE(wrapper_->isOpen()); EXPECT_FALSE(socket_.ioHandle().isOpen()); } - ~QuicIoHandleWrapperTest() override = default; protected: + Network::IoHandlePtr real_io_handle_; testing::NiceMock socket_; std::unique_ptr wrapper_; testing::StrictMock os_sys_calls_; diff --git a/test/mocks/network/BUILD b/test/mocks/network/BUILD index 38cd2003b09d7..3e3757f1ea2f0 100644 --- a/test/mocks/network/BUILD +++ b/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", diff --git a/test/mocks/network/mocks.cc b/test/mocks/network/mocks.cc index f07685c37f246..f4f20e1359b97 100644 --- a/test/mocks/network/mocks.cc +++ b/test/mocks/network/mocks.cc @@ -132,7 +132,7 @@ MockFilterChainFactory::MockFilterChainFactory() { MockFilterChainFactory::~MockFilterChainFactory() = default; MockListenSocket::MockListenSocket() - : io_handle_(std::make_unique()), + : io_handle_(std::make_unique>()), address_provider_(std::make_shared( std::make_shared(80), nullptr)) { ON_CALL(*this, options()).WillByDefault(ReturnRef(options_)); diff --git a/test/mocks/network/mocks.h b/test/mocks/network/mocks.h index 1145db10d1460..ce971d0b92cd3 100644 --- a/test/mocks/network/mocks.h +++ b/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 io_handle_; Network::SocketAddressSetterSharedPtr address_provider_; OptionsSharedPtr options_; bool socket_is_open_ = true; diff --git a/test/mocks/server/BUILD b/test/mocks/server/BUILD index c26d734bad79f..669071ba8c8d3 100644 --- a/test/mocks/server/BUILD +++ b/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", diff --git a/test/mocks/tcp/BUILD b/test/mocks/tcp/BUILD index b3cbd6903f68f..f61c5af7d0d68 100644 --- a/test/mocks/tcp/BUILD +++ b/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", ], diff --git a/test/server/listener_manager_impl_test.cc b/test/server/listener_manager_impl_test.cc index 8a8139fa33275..c2c61c695c03c 100644 --- a/test/server/listener_manager_impl_test.cc +++ b/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;