techdebt: clean up connection_handler_impl dependency issue

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 {

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 {

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",
     ],
 )
 

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

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());

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<RebalancedSocket>;
  * 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<Logger::Id::conn_handler> {
 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<ActiveTcpSocketPtr> sockets_;
-  absl::node_hash_map<const Network::FilterChain*, ActiveConnectionsPtr> connections_by_context_;
+  absl::flat_hash_map<const Network::FilterChain*, ActiveConnectionsPtr> connections_by_context_;
 
   // The number of connections currently active on this listener. This is typically used for
   // connection balancing across per-handler listeners.

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"))}) {

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,

source/server/connection_handler_impl.cc

@@ -38,7 +38,6 @@ void ConnectionHandlerImpl::addListener(absl::optional<uint64_t> overridden_list
       }
       NOT_REACHED_GCOVR_EXCL_LINE;
     }
-    // TODO(lambdai): Remove the dependency of ActiveTcpListener.
     auto tcp_listener = std::make_unique<ActiveTcpListener>(*this, config);
     details.typed_listener_ = *tcp_listener;
     details.listener_ = std::move(tcp_listener);
@@ -212,15 +211,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

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.

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"