Init manager checking unready targets with target-aware watchers

include/envoy/init/target.h

@@ -25,6 +25,12 @@ struct TargetHandle {
    * @return true if the target received this call, false if the target was already destroyed.
    */
   virtual bool initialize(const Watcher& watcher) const PURE;
+
+  /**
+   * @return a human-readable target name, for logging / debugging / tracking target names.
+   * The target name has to be unique.
+   */
+  virtual absl::string_view name() const PURE;
 };
 using TargetHandlePtr = std::unique_ptr<TargetHandle>;
 

source/common/init/manager_impl.cc

@@ -1,19 +1,23 @@
 #include "common/init/manager_impl.h"
 
+#include <functional>
+
 #include "common/common/assert.h"
+#include "common/init/watcher_impl.h"
 
 namespace Envoy {
 namespace Init {
 
 ManagerImpl::ManagerImpl(absl::string_view name)
     : name_(fmt::format("init manager {}", name)), state_(State::Uninitialized), count_(0),
-      watcher_(name_, [this]() { onTargetReady(); }) {}
+      watcher_(name_, [this](absl::string_view target_name) { onTargetReady(target_name); }) {}
 
 Manager::State ManagerImpl::state() const { return state_; }
 
 void ManagerImpl::add(const Target& target) {
   ++count_;
   TargetHandlePtr target_handle(target.createHandle(name_));
+  ++target_names_count_[target.name()];
   switch (state_) {
   case State::Uninitialized:
     // If the manager isn't initialized yet, save the target handle to be initialized later.
@@ -53,15 +57,26 @@ void ManagerImpl::initialize(const Watcher& watcher) {
     // completed immediately.
     for (const auto& target_handle : target_handles_) {
       if (!target_handle->initialize(watcher_)) {
-        onTargetReady();
+        onTargetReady(target_handle->name());
       }
     }
   }
 }
 
-void ManagerImpl::onTargetReady() {
+const absl::flat_hash_map<std::string, uint32_t>& ManagerImpl::unreadyTargets() const {
+  return target_names_count_;
+}
+
+void ManagerImpl::onTargetReady(absl::string_view target_name) {
   // If there are no remaining targets and one mysteriously calls us back, this manager is haunted.
-  ASSERT(count_ != 0, fmt::format("{} called back by target after initialization complete"));
+  ASSERT(count_ != 0,
+         fmt::format("{} called back by target after initialization complete", target_name));
+
+  // Decrease target_name count by 1.
+  ASSERT(target_names_count_.find(target_name) != target_names_count_.end());
+  if (--target_names_count_[target_name] == 0) {
+    target_names_count_.erase(target_name);
+  }
 
   // If there are no uninitialized targets remaining when called back by a target, that means it was
   // the last. Signal `ready` to the handle we saved in `initialize`.

source/common/init/manager_impl.h

@@ -7,6 +7,8 @@
 #include "common/common/logger.h"
 #include "common/init/watcher_impl.h"
 
+#include "absl/container/flat_hash_map.h"
+
 namespace Envoy {
 namespace Init {
 
@@ -35,27 +37,39 @@ class ManagerImpl : public Manager, Logger::Loggable<Logger::Id::init> {
   void add(const Target& target) override;
   void initialize(const Watcher& watcher) override;
 
+  // Expose the const reference of target_names_count_ hash map to public.
+  const absl::flat_hash_map<std::string, uint32_t>& unreadyTargets() const;
+
 private:
-  void onTargetReady();
+  // Callback function with an additional target_name parameter, decrease unready targets count by
+  // 1, update target_names_count_ hash map.
+  void onTargetReady(absl::string_view target_name);
+
   void ready();
 
-  // Human-readable name for logging
+  // Human-readable name for logging.
   const std::string name_;
 
-  // Current state
+  // Current state.
   State state_;
 
-  // Current number of registered targets that have not yet initialized
+  // Current number of registered targets that have not yet initialized.
   uint32_t count_;
 
-  // Handle to the watcher passed in `initialize`, to be called when initialization completes
+  // Handle to the watcher passed in `initialize`, to be called when initialization completes.
   WatcherHandlePtr watcher_handle_;
 
-  // Watcher to receive ready notifications from each target
+  // Watcher to receive ready notifications from each target. We restrict the watcher_ inside
+  // ManagerImpl to be constructed with the 'TargetAwareReadyFn' fn so that the init manager will
+  // get target name information when the watcher_ calls 'onTargetSendName(target_name)' For any
+  // other purpose, a watcher can be constructed with either TargetAwareReadyFn or ReadyFn.
   const WatcherImpl watcher_;
 
-  // All registered targets
+  // All registered targets.
   std::list<TargetHandlePtr> target_handles_;
+
+  // Count of target_name of unready targets.
+  absl::flat_hash_map<std::string, uint32_t> target_names_count_;
 };
 
 } // namespace Init

source/common/init/target_impl.cc

@@ -22,6 +22,8 @@ bool TargetHandleImpl::initialize(const Watcher& watcher) const {
   }
 }
 
+absl::string_view TargetHandleImpl::name() const { return name_; }
+
 TargetImpl::TargetImpl(absl::string_view name, InitializeFn fn)
     : name_(fmt::format("target {}", name)),
       fn_(std::make_shared<InternalInitalizeFn>([this, fn](WatcherHandlePtr watcher_handle) {

source/common/init/target_impl.h

@@ -38,6 +38,8 @@ class TargetHandleImpl : public TargetHandle, Logger::Loggable<Logger::Id::init>
   // Init::TargetHandle
   bool initialize(const Watcher& watcher) const override;
 
+  absl::string_view name() const override;
+
 private:
   // Name of the handle (almost always the name of the ManagerImpl calling the target)
   const std::string handle_name_;

source/common/init/watcher_impl.cc

@@ -4,34 +4,38 @@ namespace Envoy {
 namespace Init {
 
 WatcherHandleImpl::WatcherHandleImpl(absl::string_view handle_name, absl::string_view name,
-                                     std::weak_ptr<ReadyFn> fn)
+                                     std::weak_ptr<TargetAwareReadyFn> fn)
     : handle_name_(handle_name), name_(name), fn_(std::move(fn)) {}
 
 bool WatcherHandleImpl::ready() const {
   auto locked_fn(fn_.lock());
   if (locked_fn) {
     // If we can "lock" a shared pointer to the watcher's callback function, call it.
     ENVOY_LOG(debug, "{} initialized, notifying {}", handle_name_, name_);
-    (*locked_fn)();
+    (*locked_fn)(handle_name_);
     return true;
   } else {
     // If not, the watcher was already destroyed.
-    ENVOY_LOG(debug, "{} initialized, but can't notify {} (unavailable)", handle_name_, name_);
+    ENVOY_LOG(debug, "{} initialized, but can't notify {}", handle_name_, name_);
     return false;
   }
 }
 
 WatcherImpl::WatcherImpl(absl::string_view name, ReadyFn fn)
-    : name_(name), fn_(std::make_shared<ReadyFn>(std::move(fn))) {}
+    : name_(name), fn_(std::make_shared<TargetAwareReadyFn>(
+                       [callback = std::move(fn)](absl::string_view) { callback(); })) {}
+
+WatcherImpl::WatcherImpl(absl::string_view name, TargetAwareReadyFn fn)
+    : name_(name), fn_(std::make_shared<TargetAwareReadyFn>(std::move(fn))) {}
 
 WatcherImpl::~WatcherImpl() { ENVOY_LOG(debug, "{} destroyed", name_); }
 
 absl::string_view WatcherImpl::name() const { return name_; }
 
 WatcherHandlePtr WatcherImpl::createHandle(absl::string_view handle_name) const {
-  // Note: can't use std::make_unique because WatcherHandleImpl ctor is private
+  // Note: can't use std::make_unique because WatcherHandleImpl ctor is private.
   return std::unique_ptr<WatcherHandle>(
-      new WatcherHandleImpl(handle_name, name_, std::weak_ptr<ReadyFn>(fn_)));
+      new WatcherHandleImpl(handle_name, name_, std::weak_ptr<TargetAwareReadyFn>(fn_)));
 }
 
 } // namespace Init

source/common/init/watcher_impl.h

@@ -14,6 +14,7 @@ namespace Init {
  * initialization completes.
  */
 using ReadyFn = std::function<void()>;
+using TargetAwareReadyFn = std::function<void(absl::string_view)>;
 
 /**
  * A WatcherHandleImpl functions as a weak reference to a Watcher. It is how a TargetImpl safely
@@ -25,22 +26,22 @@ class WatcherHandleImpl : public WatcherHandle, Logger::Loggable<Logger::Id::ini
 private:
   friend class WatcherImpl;
   WatcherHandleImpl(absl::string_view handle_name, absl::string_view name,
-                    std::weak_ptr<ReadyFn> fn);
+                    std::weak_ptr<TargetAwareReadyFn> fn);
 
 public:
-  // Init::WatcherHandle
+  // Init::WatcherHandle.
   bool ready() const override;
 
 private:
   // Name of the handle (either the name of the target calling the manager, or the name of the
-  // manager calling the client)
+  // manager calling the client).
   const std::string handle_name_;
 
-  // Name of the watcher (either the name of the manager, or the name of the client)
+  // Name of the watcher (either the name of the manager, or the name of the client).
   const std::string name_;
 
-  // The watcher's callback function, only called if the weak pointer can be "locked"
-  const std::weak_ptr<ReadyFn> fn_;
+  // The watcher's callback function, only called if the weak pointer can be "locked".
+  const std::weak_ptr<TargetAwareReadyFn> fn_;
 };
 
 /**
@@ -51,22 +52,23 @@ class WatcherHandleImpl : public WatcherHandle, Logger::Loggable<Logger::Id::ini
 class WatcherImpl : public Watcher, Logger::Loggable<Logger::Id::init> {
 public:
   /**
-   * @param name a human-readable watcher name, for logging / debugging
-   * @param fn a callback function to invoke when `ready` is called on the handle
+   * @param name a human-readable watcher name, for logging / debugging.
+   * @param fn a callback function to invoke when `ready` is called on the handle.
    */
   WatcherImpl(absl::string_view name, ReadyFn fn);
+  WatcherImpl(absl::string_view name, TargetAwareReadyFn fn);
   ~WatcherImpl() override;
 
-  // Init::Watcher
+  // Init::Watcher.
   absl::string_view name() const override;
   WatcherHandlePtr createHandle(absl::string_view handle_name) const override;
 
 private:
-  // Human-readable name for logging
+  // Human-readable name for logging.
   const std::string name_;
 
-  // The callback function, called via WatcherHandleImpl by either the target or the manager
-  const std::shared_ptr<ReadyFn> fn_;
+  // The callback function, called via WatcherHandleImpl by either the target or the manager.
+  const std::shared_ptr<TargetAwareReadyFn> fn_;
 };
 
 } // namespace Init

test/integration/stats_integration_test.cc

@@ -286,6 +286,7 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithFakeSymbolTable) {
   // 2020/07/20  11559    44747       46000   stats: add histograms for request/response headers
   //                                          and body sizes.
   // 2020/07/21  12034    44811       46000   Add configurable histogram buckets.
+  // 2020/07/31  12035    45002       46000   Init manager store unready targets in hash map.
 
   // Note: when adjusting this value: EXPECT_MEMORY_EQ is active only in CI
   // 'release' builds, where we control the platform and tool-chain. So you
@@ -303,8 +304,7 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithFakeSymbolTable) {
   // We only run the exact test for ipv6 because ipv4 in some cases may allocate a
   // different number of bytes. We still run the approximate test.
   if (ip_version_ != Network::Address::IpVersion::v6) {
-    // https://github.com/envoyproxy/envoy/issues/12209
-    // EXPECT_MEMORY_EQ(m_per_cluster, 44811);
+    EXPECT_MEMORY_EQ(m_per_cluster, 45002);
   }
   EXPECT_MEMORY_LE(m_per_cluster, 46000); // Round up to allow platform variations.
 }
@@ -362,6 +362,7 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithRealSymbolTable) {
   // 2020/07/20  11559    36859       38000   stats: add histograms for request/response headers
   //                                          and body sizes.
   // 2020/07/21  12034    36923       38000   Add configurable histogram buckets.
+  // 2020/07/31  12035    37114       38000   Init manager store unready targets in hash map.
 
   // Note: when adjusting this value: EXPECT_MEMORY_EQ is active only in CI
   // 'release' builds, where we control the platform and tool-chain. So you
@@ -379,8 +380,7 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithRealSymbolTable) {
   // We only run the exact test for ipv6 because ipv4 in some cases may allocate a
   // different number of bytes. We still run the approximate test.
   if (ip_version_ != Network::Address::IpVersion::v6) {
-    // https://github.com/envoyproxy/envoy/issues/12209
-    // EXPECT_MEMORY_EQ(m_per_cluster, 36923);
+    EXPECT_MEMORY_EQ(m_per_cluster, 37114);
   }
   EXPECT_MEMORY_LE(m_per_cluster, 38000); // Round up to allow platform variations.
 }