wip: introduce safe callbacks and use in rewriting init manager

include/envoy/init/init.h

@@ -1,49 +1,35 @@
 #pragma once
 
-#include <functional>
-
 #include "envoy/common/pure.h"
 
-#include "absl/strings/string_view.h"
-
 namespace Envoy {
 namespace Init {
 
 /**
- * A single initialization target.
+ * Implementation-defined representations of initialization callbacks (see e.g.
+ * /source/init/callback.h). A TargetReceiver is called by the init manager to signal the target
+ * should begin initialization, and a Receiver is called by the init manager when initialization of
+ * all targets is complete.
  */
-class Target {
-public:
-  virtual ~Target() {}
-
-  /**
-   * Called when the target should begin its own initialization.
-   * @param callback supplies the callback to invoke when the target has completed its
-   *        initialization.
-   */
-  virtual void initialize(std::function<void()> callback) PURE;
-};
+class TargetReceiver;
+class Receiver;
 
 /**
- * A manager that initializes multiple targets.
+ * Init::Manager coordinates initialization of one or more "targets." A target registers its need
+ * for initialization by passing a TargetReceiver to `add`. When `initialize` is called on the
+ * manager, it notifies all targets to initialize.
  */
-class Manager {
-public:
-  virtual ~Manager() {}
+struct Manager {
+  virtual ~Manager() = default;
 
   /**
-   * Register a target to be initialized in the future. The manager will call initialize() on each
-   * target at some point in the future. It is an error to register the same target more than once.
-   * @param target the Target to initialize.
-   * @param description a human-readable description of target used for logging and debugging.
+   * The manager's state, used e.g. for reporting in the admin server.
    */
-  virtual void registerTarget(Target& target, absl::string_view description) PURE;
-
   enum class State {
     /**
      * Targets have not been initialized.
      */
-    NotInitialized,
+    Uninitialized,
     /**
      * Targets are currently being initialized.
      */
@@ -55,9 +41,24 @@ class Manager {
   };
 
   /**
-   * Returns the current state of the init manager.
+   * @return the current state of the manager.
    */
   virtual State state() const PURE;
+
+  /**
+   * Register an initialization target. If the manager's current state is uninitialized, the target
+   * will be saved for invocation later, when `initialize` is called. If the current state is
+   * initializing, the target will be invoked immediately. It is an error to register a target with
+   * a manager that is already in initialized state.
+   * @param target_receiver the target to be invoked when initialization begins.
+   */
+  virtual void add(const TargetReceiver& target_receiver) PURE;
+
+  /**
+   * Start initialization of all previously registered targets. It is an error to call initialize
+   * on a manager that is already in initializing or initialized state.
+   */
+  virtual void initialize(const Receiver& receiver) PURE;
 };
 
 } // namespace Init

source/common/callback/BUILD

@@ -0,0 +1,20 @@
+licenses(["notice"])  # Apache 2
+
+load(
+    "//bazel:envoy_build_system.bzl",
+    "envoy_cc_library",
+    "envoy_package",
+)
+
+envoy_package()
+
+envoy_cc_library(
+    name = "callback",
+    hdrs = ["callback.h"],
+)
+
+envoy_cc_library(
+    name = "manager",
+    hdrs = ["manager.h"],
+    deps = ["//source/common/callback"],
+)

source/common/callback/callback.h

@@ -0,0 +1,180 @@
+#pragma once
+
+#include <functional>
+#include <memory>
+
+namespace Envoy {
+namespace Common {
+
+/**
+ * The Callback::Caller and Callback::Receiver classes address a memory safety issue with callbacks
+ * in C++. Typically, an "event consumer" (a.k.a. handler, listener, observer) might register
+ * interest with an "event producer" (a.k.a. manager, subject) either by implementing an OO-style
+ * callback interface like:
+ *
+ *   struct EventConsumer {
+ *     virtual void onEvent(const Event& e) = 0;
+ *   };
+ *
+ *   class MyEventConsumer : public EventConsumer {
+ *   public:
+ *     MyEventConsumer(EventProducer& producer) { producer.attach(this); }
+ *     void onEvent(const Event& e) override { ... handle event ... }
+ *   };
+ *
+ *   class EventProducer {
+ *   public:
+ *     void attach(EventConsumer* consumer) { consumer_ = consumer; }
+ *     void invoke() { consumer_.onEvent(... some event ...); }
+ *   private:
+ *     EventConsumer* consumer_;
+ *   };
+ *
+ * ... or by passing a functional-style callback like:
+ *
+ *   class MyEventConsumer {
+ *   public:
+ *     MyEventConsumer(EventProducer& producer) {
+ *       producer.attach([this]() { ... handle event ... });
+ *     }
+ *   };
+ *
+ *   class EventProducer {
+ *   public:
+ *     void attach(std::function<void(const Event&)> callback) { callback_ = callback; }
+ *     void invoke() { callback_(... some event ...); }
+ *   private:
+ *     std::function<void(const Event&)> callback_;
+ *   };
+ *
+ *
+ * These approaches are equivalent, and they both have the same issue: the event producer
+ * references the event consumer, either directly or via the lambda's captures, but doesn't manage
+ * its lifetime. If the event consumer is destroyed before the event producer calls it, this is a
+ * use-after-free.
+ *
+ * In some cases, it's straightforward enough to implement "cancelation" by allowing an event
+ * consumer to be detached from any event producers it is currently attached to, but that requires
+ * holding references to all such event producers. That may be impractical or, again, unsafe in the
+ * case where the event consumer outlives its producers.
+ *
+ * Caller and Receiver provide some additional safety. A Receiver owns a callback function, and
+ * can produce Callers which function as weak references to it. The receiver's callback function
+ * can only be invoked via its callers. If the receiver is destroyed, invoking its callers has
+ * no effect, so none of the callback's captures can be unsafely dereferenced.
+ *
+ * When implementing this pattern, an event consumer would own a Receiver and an event producer
+ * would own the corresponding Caller. For example:
+ *
+ *   using EventCaller = Callback::Caller<const Event&>;
+ *   using EventReceiver = Callback::Receiver<const Event&>;
+ *
+ *   class MyEventConsumer {
+ *   public:
+ *     MyEventConsumer(EventProducer& producer) {
+ *       producer.attach(receiver_.caller());
+ *     }
+ *   private:
+ *     EventReceiver receiver_([this]() { ... handle event ... });
+ *   };
+ *
+ *   class EventProducer {
+ *   public:
+ *     void attach(EventCaller caller) { caller_ = caller; }
+ *     void invoke() { caller_(... some event ... ); }
+ *   private:
+ *     EventCaller caller_;
+ *   };
+ */
+namespace Callback {
+
+// Forward-declaration for Caller's friend declaration.
+template <typename... Args> class Receiver;
+
+/**
+ * Caller: simple wrapper for a weak_ptr to a callback function. Copyable and movable.
+ */
+template <typename... Args> class Caller {
+public:
+  /**
+   * Default constructor for default / value initialization.
+   */
+  Caller() = default;
+
+  /**
+   * Implicit conversion to bool, to test whether the corresponding Receiver is still available.
+   * @return true if the corresponding Receiver is still available, false otherwise.
+   */
+  operator bool() const { return !fn_.expired(); }
+
+  /**
+   * Reset this caller to not reference a Receiver.
+   */
+  void reset() { fn_.reset(); }
+
+  /**
+   * Invoke the corresponding Receiver's callback, if it is still available. If the receiver has
+   * been destroyed or reset, this has no effect.
+   * @param args the arguments, if any, to pass to the receiver's callback function.
+   */
+  void operator()(Args... args) const {
+    auto locked_fn(fn_.lock());
+    if (locked_fn) {
+      (*locked_fn)(args...);
+    }
+  }
+
+private:
+  /**
+   * Can only be constructed by a Receiver
+   */
+  friend Receiver<Args...>;
+  Caller(std::weak_ptr<std::function<void(Args...)>> fn) : fn_(std::move(fn)) {}
+
+  std::weak_ptr<std::function<void(Args...)>> fn_;
+};
+
+/**
+ * Receiver: simple wrapper for a shared_ptr to a callback function. Copyable and movable, but
+ * typically should be owned uniquely by the owner of any pointers and references captured by its
+ * handler function. For example, if `this` is captured by the handler function, `this` should
+ * probably also own the Receiver.
+ */
+template <typename... Args> class Receiver {
+public:
+  /**
+   * Default constructor for default / value initialization.
+   */
+  Receiver() = default;
+
+  /**
+   * Construct a receiver to own a callback function.
+   */
+  Receiver(std::function<void(Args...)> fn)
+      : fn_(std::make_shared<std::function<void(Args...)>>(std::move(fn))) {}
+
+  /**
+   * @return a new caller for this receiver.
+   */
+  Caller<Args...> caller() const {
+    return Caller<Args...>(std::weak_ptr<std::function<void(Args...)>>(fn_));
+  }
+
+  /**
+   * Reset this receiver, such that any callers previously created will not be able to invoke it.
+   */
+  void reset() { fn_.reset(); }
+
+  /**
+   * Explicit conversion to bool, to test whether the receiver contains a callback function.
+   * @return true if the corresponding Receiver contains a callback, false otherwise.
+   */
+  explicit operator bool() const { return static_cast<bool>(fn_); }
+
+private:
+  std::shared_ptr<std::function<void(Args...)>> fn_;
+};
+
+} // namespace Callback
+} // namespace Common
+} // namespace Envoy

source/common/callback/manager.h

@@ -0,0 +1,65 @@
+#pragma once
+
+#include <list>
+
+#include "common/callback/callback.h"
+
+namespace Envoy {
+namespace Common {
+namespace Callback {
+
+/**
+ * Callback::Manager is a fairly typical implementation of the "Observer" pattern
+ * (https://en.wikipedia.org/wiki/Observer_pattern), made safe by using Callback::Caller. Any number
+ * of Callers may be added to a Manager. Resetting or destroying a Caller's corresponding Receiver
+ * will remove it from the Manager's list when it is invoked.
+ *
+ * Manager is actually a type alias (below) to this class template, ManagerT, which is parameterized
+ * on an additional type C, representing the caller type. This will typically be Caller<Args...>,
+ * but can be anything that behaves like a function. See examples in manager_test.cc (where the
+ * caller type is a mock), and Init::ManagerImpl (where the caller does some extra logging).
+ */
+template <typename C, typename... Args> class ManagerT {
+public:
+  /**
+   * Adds a Caller to the callback manager, such that its corresponding Receiver will be invoked
+   * by subsequent calls to call() as long as it remains available.
+   * @param caller the caller to add to the callback manager
+   */
+  ManagerT& add(C caller) {
+    callers_.push_back(std::move(caller));
+    return *this;
+  }
+
+  /**
+   * Invokes all callers previously provided to add(). Any corresponding receivers that are no
+   * longer available after invocation (which may have happened as a side-effect of invoking them)
+   * will be removed from the callback manager.
+   * @param args the arguments, if any, to pass to all callers.
+   */
+  void operator()(Args... args) {
+    for (auto it = callers_.begin(); it != callers_.end(); /* incremented below */) {
+      // First, invoke the caller whether or not it references an available receiver.
+      const auto& caller = *it;
+      caller(args...);
+
+      // The caller may reference an unavailable receiver, either because the receiver was already
+      // unavailable beforehand, or because it was reset or destroyed as a side-effect of invoking
+      // it. If the receiver is unavailable, remove it so we don't try to call it again.
+      if (caller) {
+        ++it;
+      } else {
+        it = callers_.erase(it);
+      }
+    }
+  }
+
+private:
+  std::list<C> callers_;
+};
+
+template <typename... Args> using Manager = ManagerT<Caller<Args...>, Args...>;
+
+} // namespace Callback
+} // namespace Common
+} // namespace Envoy

source/common/config/BUILD

@@ -397,5 +397,6 @@ envoy_cc_library(
         "//include/envoy/singleton:instance_interface",
         "//include/envoy/thread_local:thread_local_interface",
         "//source/common/protobuf",
+        "//source/init:callback_lib",
     ],
 )

source/common/config/config_provider_impl.cc

@@ -21,10 +21,8 @@ ConfigSubscriptionInstanceBase::~ConfigSubscriptionInstanceBase() {
 }
 
 void ConfigSubscriptionInstanceBase::runInitializeCallbackIfAny() {
-  if (initialize_callback_) {
-    initialize_callback_();
-    initialize_callback_ = nullptr;
-  }
+  init_caller_();
+  init_caller_.reset();
 }
 
 bool ConfigSubscriptionInstanceBase::checkAndApplyConfig(const Protobuf::Message& config_proto,