From 351541789ca28a73e39dd900a72ca6a01c6495a6 Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Thu, 1 Apr 2021 09:13:45 -0700 Subject: [PATCH 1/6] techdebt: clean up connection_handler_impl dependency issue Signed-off-by: Yuchen Dai --- source/common/quic/envoy_quic_dispatcher.h | 1 + source/common/quic/envoy_quic_proof_source.h | 1 + source/server/BUILD | 26 ++------ source/server/active_listener_base.h | 63 ++++++++++++++++++++ source/server/active_tcp_listener.cc | 52 ++++++++-------- source/server/active_tcp_listener.h | 8 ++- source/server/active_udp_listener.cc | 2 +- source/server/active_udp_listener.h | 5 +- source/server/connection_handler_impl.cc | 10 ---- source/server/connection_handler_impl.h | 45 -------------- 10 files changed, 102 insertions(+), 111 deletions(-) create mode 100644 source/server/active_listener_base.h diff --git a/source/common/quic/envoy_quic_dispatcher.h b/source/common/quic/envoy_quic_dispatcher.h index d59307f415ecc..85a0a009d7137 100644 --- a/source/common/quic/envoy_quic_dispatcher.h +++ b/source/common/quic/envoy_quic_dispatcher.h @@ -18,6 +18,7 @@ #include "envoy/network/listener.h" #include "server/connection_handler_impl.h" +#include "server/active_listener_base.h" namespace Envoy { namespace Quic { diff --git a/source/common/quic/envoy_quic_proof_source.h b/source/common/quic/envoy_quic_proof_source.h index 7642e26c2039a..97c2120870b75 100644 --- a/source/common/quic/envoy_quic_proof_source.h +++ b/source/common/quic/envoy_quic_proof_source.h @@ -3,6 +3,7 @@ #include "common/quic/envoy_quic_proof_source_base.h" #include "common/quic/quic_transport_socket_factory.h" +#include "server/active_listener_base.h" #include "server/connection_handler_impl.h" namespace Envoy { diff --git a/source/server/BUILD b/source/server/BUILD index bd7727db6d216..dd46e4e446c00 100644 --- a/source/server/BUILD +++ b/source/server/BUILD @@ -77,6 +77,7 @@ envoy_cc_library( "connection_handler_impl.h", ], deps = [ + ":active_tcp_listener", "//include/envoy/common:time_interface", "//include/envoy/event:deferred_deletable", "//include/envoy/event:dispatcher_interface", @@ -92,38 +93,19 @@ envoy_cc_library( "//source/common/event:deferred_task", "//source/common/network:connection_lib", "//source/common/stream_info:stream_info_lib", - "//source/server:active_tcp_listener_header", ], ) envoy_cc_library( - # Currently both `active_tcp_listener` and `connection_handler_impl` need below headers - # while addressing https://github.com/envoyproxy/envoy/issues/15126 - # TODO(lambdai): Remove the definition of ActiveTcpListener from dependency of ConnectionHandlerImpl - # and delete this target. - name = "active_tcp_listener_header", + name = "active_listener_base", hdrs = [ - "active_tcp_listener.h", - "connection_handler_impl.h", + "active_listener_base.h", ], deps = [ - "//include/envoy/common:time_interface", - "//include/envoy/event:deferred_deletable", - "//include/envoy/event:dispatcher_interface", - "//include/envoy/event:timer_interface", "//include/envoy/network:connection_handler_interface", - "//include/envoy/network:connection_interface", - "//include/envoy/network:filter_interface", - "//include/envoy/network:listen_socket_interface", "//include/envoy/network:listener_interface", - "//include/envoy/server:listener_manager_interface", "//include/envoy/stats:timespan_interface", - "//source/common/common:linked_object", - "//source/common/common:non_copyable", - "//source/common/event:deferred_task", - "//source/common/network:connection_lib", "//source/common/stats:timespan_lib", - "//source/common/stream_info:stream_info_lib", ], ) @@ -153,7 +135,7 @@ envoy_cc_library( "//source/common/stats:timespan_lib", "//source/common/stream_info:stream_info_lib", "//source/extensions/transport_sockets:well_known_names", - "//source/server:active_tcp_listener_header", + "//source/server:active_listener_base", ], ) diff --git a/source/server/active_listener_base.h b/source/server/active_listener_base.h new file mode 100644 index 0000000000000..5cfdbe3c56690 --- /dev/null +++ b/source/server/active_listener_base.h @@ -0,0 +1,63 @@ +#pragma once + +#include "envoy/network/connection_handler.h" +#include "envoy/network/listener.h" +#include "envoy/stats/scope.h" + +namespace Envoy { +namespace Server { + +#define ALL_LISTENER_STATS(COUNTER, GAUGE, HISTOGRAM) \ + COUNTER(downstream_cx_destroy) \ + COUNTER(downstream_cx_overflow) \ + COUNTER(downstream_cx_total) \ + COUNTER(downstream_cx_overload_reject) \ + COUNTER(downstream_global_cx_overflow) \ + COUNTER(downstream_pre_cx_timeout) \ + COUNTER(no_filter_chain_match) \ + GAUGE(downstream_cx_active, Accumulate) \ + GAUGE(downstream_pre_cx_active, Accumulate) \ + HISTOGRAM(downstream_cx_length_ms, Milliseconds) + +/** + * Wrapper struct for listener stats. @see stats_macros.h + */ +struct ListenerStats { + ALL_LISTENER_STATS(GENERATE_COUNTER_STRUCT, GENERATE_GAUGE_STRUCT, GENERATE_HISTOGRAM_STRUCT) +}; + +#define ALL_PER_HANDLER_LISTENER_STATS(COUNTER, GAUGE) \ + COUNTER(downstream_cx_total) \ + GAUGE(downstream_cx_active, Accumulate) + +/** + * Wrapper struct for per-handler listener stats. @see stats_macros.h + */ +struct PerHandlerListenerStats { + ALL_PER_HANDLER_LISTENER_STATS(GENERATE_COUNTER_STRUCT, GENERATE_GAUGE_STRUCT) +}; + +/** + * Wrapper for an active listener owned by this handler. + */ +class ActiveListenerImplBase : public virtual Network::ConnectionHandler::ActiveListener { +public: + ActiveListenerImplBase(Network::ConnectionHandler& parent, Network::ListenerConfig* config) + : stats_({ALL_LISTENER_STATS(POOL_COUNTER(config->listenerScope()), + POOL_GAUGE(config->listenerScope()), + POOL_HISTOGRAM(config->listenerScope()))}), + per_worker_stats_({ALL_PER_HANDLER_LISTENER_STATS( + POOL_COUNTER_PREFIX(config->listenerScope(), parent.statPrefix()), + POOL_GAUGE_PREFIX(config->listenerScope(), parent.statPrefix()))}), + config_(config) {} + + // Network::ConnectionHandler::ActiveListener. + uint64_t listenerTag() override { return config_->listenerTag(); } + + ListenerStats stats_; + PerHandlerListenerStats per_worker_stats_; + Network::ListenerConfig* config_{}; +}; + +} // namespace Server +} // namespace Envoy \ No newline at end of file diff --git a/source/server/active_tcp_listener.cc b/source/server/active_tcp_listener.cc index c522aa0e01105..7a5f0a64edccc 100644 --- a/source/server/active_tcp_listener.cc +++ b/source/server/active_tcp_listener.cc @@ -12,8 +12,6 @@ #include "common/network/utility.h" #include "common/stats/timespan_impl.h" -#include "server/connection_handler_impl.h" - #include "extensions/transport_sockets/well_known_names.h" namespace Envoy { @@ -28,25 +26,6 @@ void emitLogs(Network::ListenerConfig& config, StreamInfo::StreamInfo& stream_in } } // namespace -void ActiveTcpListener::removeConnection(ActiveTcpConnection& connection) { - ENVOY_CONN_LOG(debug, "adding to cleanup list", *connection.connection_); - ActiveConnections& active_connections = connection.active_connections_; - ActiveTcpConnectionPtr removed = connection.removeFromList(active_connections.connections_); - parent_.dispatcher().deferredDelete(std::move(removed)); - // Delete map entry only iff connections becomes empty. - if (active_connections.connections_.empty()) { - auto iter = connections_by_context_.find(&active_connections.filter_chain_); - ASSERT(iter != connections_by_context_.end()); - // To cover the lifetime of every single connection, Connections need to be deferred deleted - // because the previously contained connection is deferred deleted. - parent_.dispatcher().deferredDelete(std::move(iter->second)); - // The erase will break the iteration over the connections_by_context_ during the deletion. - if (!is_deleting_) { - connections_by_context_.erase(iter); - } - } -} - ActiveTcpListener::ActiveTcpListener(Network::TcpConnectionHandler& parent, Network::ListenerConfig& config) : ActiveTcpListener( @@ -64,12 +43,6 @@ ActiveTcpListener::ActiveTcpListener(Network::TcpConnectionHandler& parent, config.connectionBalancer().registerHandler(*this); } -void ActiveTcpListener::updateListenerConfig(Network::ListenerConfig& config) { - ENVOY_LOG(trace, "replacing listener ", config_->listenerTag(), " by ", config.listenerTag()); - ASSERT(&config_->connectionBalancer() == &config.connectionBalancer()); - config_ = &config; -} - ActiveTcpListener::~ActiveTcpListener() { is_deleting_ = true; config_->connectionBalancer().unregisterHandler(*this); @@ -99,6 +72,31 @@ ActiveTcpListener::~ActiveTcpListener() { ASSERT(num_listener_connections_ == 0); } +void ActiveTcpListener::removeConnection(ActiveTcpConnection& connection) { + ENVOY_CONN_LOG(debug, "adding to cleanup list", *connection.connection_); + ActiveConnections& active_connections = connection.active_connections_; + ActiveTcpConnectionPtr removed = connection.removeFromList(active_connections.connections_); + parent_.dispatcher().deferredDelete(std::move(removed)); + // Delete map entry only iff connections becomes empty. + if (active_connections.connections_.empty()) { + auto iter = connections_by_context_.find(&active_connections.filter_chain_); + ASSERT(iter != connections_by_context_.end()); + // To cover the lifetime of every single connection, Connections need to be deferred deleted + // because the previously contained connection is deferred deleted. + parent_.dispatcher().deferredDelete(std::move(iter->second)); + // The erase will break the iteration over the connections_by_context_ during the deletion. + if (!is_deleting_) { + connections_by_context_.erase(iter); + } + } +} + +void ActiveTcpListener::updateListenerConfig(Network::ListenerConfig& config) { + ENVOY_LOG(trace, "replacing listener ", config_->listenerTag(), " by ", config.listenerTag()); + ASSERT(&config_->connectionBalancer() == &config.connectionBalancer()); + config_ = &config; +} + void ActiveTcpSocket::onTimeout() { listener_.stats_.downstream_pre_cx_timeout_.inc(); ASSERT(inserted()); diff --git a/source/server/active_tcp_listener.h b/source/server/active_tcp_listener.h index fd4a17160aab4..01cf11b72408a 100644 --- a/source/server/active_tcp_listener.h +++ b/source/server/active_tcp_listener.h @@ -1,11 +1,13 @@ #pragma once +#include "envoy/event/dispatcher.h" +#include "envoy/event/timer.h" #include "envoy/stats/timespan.h" #include "common/common/linked_object.h" #include "common/stream_info/stream_info_impl.h" -#include "server/connection_handler_impl.h" +#include "server/active_listener_base.h" namespace Envoy { namespace Server { @@ -29,7 +31,7 @@ using RebalancedSocketSharedPtr = std::shared_ptr; * Wrapper for an active tcp listener owned by this handler. */ class ActiveTcpListener : public Network::TcpListenerCallbacks, - public ConnectionHandlerImpl::ActiveListenerImplBase, + public ActiveListenerImplBase, public Network::BalancedConnectionHandler, Logger::Loggable { public: @@ -104,7 +106,7 @@ class ActiveTcpListener : public Network::TcpListenerCallbacks, const std::chrono::milliseconds listener_filters_timeout_; const bool continue_on_listener_filters_timeout_; std::list sockets_; - absl::node_hash_map connections_by_context_; + absl::flat_hash_map connections_by_context_; // The number of connections currently active on this listener. This is typically used for // connection balancing across per-handler listeners. diff --git a/source/server/active_udp_listener.cc b/source/server/active_udp_listener.cc index 7b712069025c4..ceb01bbcd14e9 100644 --- a/source/server/active_udp_listener.cc +++ b/source/server/active_udp_listener.cc @@ -15,7 +15,7 @@ ActiveUdpListenerBase::ActiveUdpListenerBase(uint32_t worker_index, uint32_t con Network::Socket& listen_socket, Network::UdpListenerPtr&& listener, Network::ListenerConfig* config) - : ConnectionHandlerImpl::ActiveListenerImplBase(parent, config), worker_index_(worker_index), + : ActiveListenerImplBase(parent, config), worker_index_(worker_index), concurrency_(concurrency), parent_(parent), listen_socket_(listen_socket), udp_listener_(std::move(listener)), udp_stats_({ALL_UDP_LISTENER_STATS(POOL_COUNTER_PREFIX(config->listenerScope(), "udp"))}) { diff --git a/source/server/active_udp_listener.h b/source/server/active_udp_listener.h index 645d37e4c8f6b..e122b65d2e42e 100644 --- a/source/server/active_udp_listener.h +++ b/source/server/active_udp_listener.h @@ -8,8 +8,7 @@ #include "envoy/network/listen_socket.h" #include "envoy/network/listener.h" -// TODO(lambdai): remove connection_handler_impl after ActiveListenerImplBase is extracted from it. -#include "server/connection_handler_impl.h" +#include "server/active_listener_base.h" namespace Envoy { namespace Server { @@ -23,7 +22,7 @@ struct UdpListenerStats { ALL_UDP_LISTENER_STATS(GENERATE_COUNTER_STRUCT) }; -class ActiveUdpListenerBase : public ConnectionHandlerImpl::ActiveListenerImplBase, +class ActiveUdpListenerBase : public ActiveListenerImplBase, public Network::ConnectionHandler::ActiveUdpListener { public: ActiveUdpListenerBase(uint32_t worker_index, uint32_t concurrency, diff --git a/source/server/connection_handler_impl.cc b/source/server/connection_handler_impl.cc index 37983bc08331c..882a9fd776123 100644 --- a/source/server/connection_handler_impl.cc +++ b/source/server/connection_handler_impl.cc @@ -212,15 +212,5 @@ ConnectionHandlerImpl::getBalancedHandlerByAddress(const Network::Address::Insta : absl::nullopt; } -ConnectionHandlerImpl::ActiveListenerImplBase::ActiveListenerImplBase( - Network::ConnectionHandler& parent, Network::ListenerConfig* config) - : stats_({ALL_LISTENER_STATS(POOL_COUNTER(config->listenerScope()), - POOL_GAUGE(config->listenerScope()), - POOL_HISTOGRAM(config->listenerScope()))}), - per_worker_stats_({ALL_PER_HANDLER_LISTENER_STATS( - POOL_COUNTER_PREFIX(config->listenerScope(), parent.statPrefix()), - POOL_GAUGE_PREFIX(config->listenerScope(), parent.statPrefix()))}), - config_(config) {} - } // namespace Server } // namespace Envoy diff --git a/source/server/connection_handler_impl.h b/source/server/connection_handler_impl.h index 0d2e8a52f65df..4d73ba79fe219 100644 --- a/source/server/connection_handler_impl.h +++ b/source/server/connection_handler_impl.h @@ -19,36 +19,6 @@ namespace Envoy { namespace Server { -#define ALL_LISTENER_STATS(COUNTER, GAUGE, HISTOGRAM) \ - COUNTER(downstream_cx_destroy) \ - COUNTER(downstream_cx_overflow) \ - COUNTER(downstream_cx_total) \ - COUNTER(downstream_cx_overload_reject) \ - COUNTER(downstream_global_cx_overflow) \ - COUNTER(downstream_pre_cx_timeout) \ - COUNTER(no_filter_chain_match) \ - GAUGE(downstream_cx_active, Accumulate) \ - GAUGE(downstream_pre_cx_active, Accumulate) \ - HISTOGRAM(downstream_cx_length_ms, Milliseconds) - -/** - * Wrapper struct for listener stats. @see stats_macros.h - */ -struct ListenerStats { - ALL_LISTENER_STATS(GENERATE_COUNTER_STRUCT, GENERATE_GAUGE_STRUCT, GENERATE_HISTOGRAM_STRUCT) -}; - -#define ALL_PER_HANDLER_LISTENER_STATS(COUNTER, GAUGE) \ - COUNTER(downstream_cx_total) \ - GAUGE(downstream_cx_active, Accumulate) - -/** - * Wrapper struct for per-handler listener stats. @see stats_macros.h - */ -struct PerHandlerListenerStats { - ALL_PER_HANDLER_LISTENER_STATS(GENERATE_COUNTER_STRUCT, GENERATE_GAUGE_STRUCT) -}; - class ActiveUdpListenerBase; class ActiveTcpListener; @@ -93,21 +63,6 @@ class ConnectionHandlerImpl : public Network::TcpConnectionHandler, // Network::UdpConnectionHandler Network::UdpListenerCallbacksOptRef getUdpListenerCallbacks(uint64_t listener_tag) override; - /** - * Wrapper for an active listener owned by this handler. - */ - class ActiveListenerImplBase : public virtual Network::ConnectionHandler::ActiveListener { - public: - ActiveListenerImplBase(Network::ConnectionHandler& parent, Network::ListenerConfig* config); - - // Network::ConnectionHandler::ActiveListener. - uint64_t listenerTag() override { return config_->listenerTag(); } - - ListenerStats stats_; - PerHandlerListenerStats per_worker_stats_; - Network::ListenerConfig* config_{}; - }; - private: struct ActiveListenerDetails { // Strong pointer to the listener, whether TCP, UDP, QUIC, etc. From 734f4f425ab1b601e743ab059e191b78d35339aa Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Thu, 1 Apr 2021 20:33:29 +0000 Subject: [PATCH 2/6] fix build Signed-off-by: Yuchen Dai --- source/server/active_tcp_listener.h | 1 - test/common/quic/envoy_quic_server_stream_test.cc | 1 + 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/source/server/active_tcp_listener.h b/source/server/active_tcp_listener.h index 01cf11b72408a..0b1c6250cc04d 100644 --- a/source/server/active_tcp_listener.h +++ b/source/server/active_tcp_listener.h @@ -6,7 +6,6 @@ #include "common/common/linked_object.h" #include "common/stream_info/stream_info_impl.h" - #include "server/active_listener_base.h" namespace Envoy { diff --git a/test/common/quic/envoy_quic_server_stream_test.cc b/test/common/quic/envoy_quic_server_stream_test.cc index fc7bcfbf85214..a967251b2fbf7 100644 --- a/test/common/quic/envoy_quic_server_stream_test.cc +++ b/test/common/quic/envoy_quic_server_stream_test.cc @@ -15,6 +15,7 @@ #include "common/event/libevent_scheduler.h" #include "common/http/headers.h" +#include "server/active_listener_base.h" #include "common/quic/envoy_quic_alarm_factory.h" #include "common/quic/envoy_quic_connection_helper.h" From 0c88b62e151fc42d059b1ceff9cdda224d920492 Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Thu, 1 Apr 2021 20:35:41 +0000 Subject: [PATCH 3/6] clean 1 TODO Signed-off-by: Yuchen Dai --- source/server/connection_handler_impl.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/source/server/connection_handler_impl.cc b/source/server/connection_handler_impl.cc index 882a9fd776123..043fbdb20b539 100644 --- a/source/server/connection_handler_impl.cc +++ b/source/server/connection_handler_impl.cc @@ -38,7 +38,6 @@ void ConnectionHandlerImpl::addListener(absl::optional overridden_list } NOT_REACHED_GCOVR_EXCL_LINE; } - // TODO(lambdai): Remove the dependency of ActiveTcpListener. auto tcp_listener = std::make_unique(*this, config); details.typed_listener_ = *tcp_listener; details.listener_ = std::move(tcp_listener); From 17957455e215501f7320093f35a046f30cd80de3 Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Fri, 2 Apr 2021 12:00:02 -0700 Subject: [PATCH 4/6] fix format pre Signed-off-by: Yuchen Dai --- source/server/active_listener_base.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/server/active_listener_base.h b/source/server/active_listener_base.h index 5cfdbe3c56690..6981900983c89 100644 --- a/source/server/active_listener_base.h +++ b/source/server/active_listener_base.h @@ -60,4 +60,4 @@ class ActiveListenerImplBase : public virtual Network::ConnectionHandler::Active }; } // namespace Server -} // namespace Envoy \ No newline at end of file +} // namespace Envoy From ae6b275be74126dfad4b846456a731996e8cde0d Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Fri, 2 Apr 2021 15:33:21 -0700 Subject: [PATCH 5/6] clang-format-11 Signed-off-by: Yuchen Dai --- source/common/quic/platform/quic_logging_impl.h | 2 +- source/server/active_tcp_listener.h | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/source/common/quic/platform/quic_logging_impl.h b/source/common/quic/platform/quic_logging_impl.h index 74966040510d1..91504224d1eb9 100644 --- a/source/common/quic/platform/quic_logging_impl.h +++ b/source/common/quic/platform/quic_logging_impl.h @@ -147,7 +147,7 @@ enum { // DFATAL is FATAL in debug mode, ERROR in release mode. #ifdef NDEBUG LogLevelDFATAL = LogLevelERROR, -#else // NDEBUG +#else // NDEBUG LogLevelDFATAL = LogLevelFATAL, #endif // NDEBUG }; diff --git a/source/server/active_tcp_listener.h b/source/server/active_tcp_listener.h index 0b1c6250cc04d..01cf11b72408a 100644 --- a/source/server/active_tcp_listener.h +++ b/source/server/active_tcp_listener.h @@ -6,6 +6,7 @@ #include "common/common/linked_object.h" #include "common/stream_info/stream_info_impl.h" + #include "server/active_listener_base.h" namespace Envoy { From ca8f46cda578a1ecb9c4c4ad00670907ac9e4d11 Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Fri, 2 Apr 2021 22:25:42 -0700 Subject: [PATCH 6/6] antother format fix Signed-off-by: Yuchen Dai --- source/common/quic/platform/quic_logging_impl.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/quic/platform/quic_logging_impl.h b/source/common/quic/platform/quic_logging_impl.h index 91504224d1eb9..74966040510d1 100644 --- a/source/common/quic/platform/quic_logging_impl.h +++ b/source/common/quic/platform/quic_logging_impl.h @@ -147,7 +147,7 @@ enum { // DFATAL is FATAL in debug mode, ERROR in release mode. #ifdef NDEBUG LogLevelDFATAL = LogLevelERROR, -#else // NDEBUG +#else // NDEBUG LogLevelDFATAL = LogLevelFATAL, #endif // NDEBUG };