diff --git a/include/envoy/init/init.h b/include/envoy/init/init.h index 824dbd01fac59..d3dee09bce2ab 100644 --- a/include/envoy/init/init.h +++ b/include/envoy/init/init.h @@ -1,49 +1,35 @@ #pragma once -#include - #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 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 diff --git a/source/common/callback/BUILD b/source/common/callback/BUILD new file mode 100644 index 0000000000000..741e60d19fdce --- /dev/null +++ b/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"], +) diff --git a/source/common/callback/callback.h b/source/common/callback/callback.h new file mode 100644 index 0000000000000..1847d92974d35 --- /dev/null +++ b/source/common/callback/callback.h @@ -0,0 +1,180 @@ +#pragma once + +#include +#include + +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 callback) { callback_ = callback; } + * void invoke() { callback_(... some event ...); } + * private: + * std::function 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; + * using EventReceiver = Callback::Receiver; + * + * 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 class Receiver; + +/** + * Caller: simple wrapper for a weak_ptr to a callback function. Copyable and movable. + */ +template 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; + Caller(std::weak_ptr> fn) : fn_(std::move(fn)) {} + + std::weak_ptr> 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 class Receiver { +public: + /** + * Default constructor for default / value initialization. + */ + Receiver() = default; + + /** + * Construct a receiver to own a callback function. + */ + Receiver(std::function fn) + : fn_(std::make_shared>(std::move(fn))) {} + + /** + * @return a new caller for this receiver. + */ + Caller caller() const { + return Caller(std::weak_ptr>(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(fn_); } + +private: + std::shared_ptr> fn_; +}; + +} // namespace Callback +} // namespace Common +} // namespace Envoy diff --git a/source/common/callback/manager.h b/source/common/callback/manager.h new file mode 100644 index 0000000000000..96a2c14197b17 --- /dev/null +++ b/source/common/callback/manager.h @@ -0,0 +1,65 @@ +#pragma once + +#include + +#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, + * 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 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 callers_; +}; + +template using Manager = ManagerT, Args...>; + +} // namespace Callback +} // namespace Common +} // namespace Envoy diff --git a/source/common/config/BUILD b/source/common/config/BUILD index 8071c1855276b..9a476b309adc7 100644 --- a/source/common/config/BUILD +++ b/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", ], ) diff --git a/source/common/config/config_provider_impl.cc b/source/common/config/config_provider_impl.cc index 541c767412aac..34fc5e301bb2e 100644 --- a/source/common/config/config_provider_impl.cc +++ b/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, diff --git a/source/common/config/config_provider_impl.h b/source/common/config/config_provider_impl.h index b865165cd4f44..0856884ab3aa7 100644 --- a/source/common/config/config_provider_impl.h +++ b/source/common/config/config_provider_impl.h @@ -15,6 +15,8 @@ #include "common/config/utility.h" #include "common/protobuf/protobuf.h" +#include "init/callback.h" + namespace Envoy { namespace Config { @@ -133,21 +135,14 @@ class MutableConfigProviderImplBase; * This class can not be instantiated directly; instead, it provides the foundation for * config subscription implementations which derive from it. */ -class ConfigSubscriptionInstanceBase : public Init::Target, - protected Logger::Loggable { +class ConfigSubscriptionInstanceBase : protected Logger::Loggable { public: struct LastConfigInfo { uint64_t last_config_hash_; std::string last_config_version_; }; - ~ConfigSubscriptionInstanceBase() override; - - // Init::Target - void initialize(std::function callback) override { - initialize_callback_ = callback; - start(); - } + virtual ~ConfigSubscriptionInstanceBase(); /** * Starts the subscription corresponding to a config source. @@ -211,9 +206,7 @@ class ConfigSubscriptionInstanceBase : public Init::Target, void runInitializeCallbackIfAny(); private: - void registerInitTarget(Init::Manager& init_manager) { - init_manager.registerTarget(*this, fmt::format("ConfigSubscriptionInstanceBase {}", name_)); - } + void registerInitTarget(Init::Manager& init_manager) { init_manager.add(init_target_receiver_); } void bindConfigProvider(MutableConfigProviderImplBase* provider); @@ -222,7 +215,6 @@ class ConfigSubscriptionInstanceBase : public Init::Target, } const std::string name_; - std::function initialize_callback_; std::unordered_set mutable_config_providers_; const uint64_t manager_identifier_; ConfigProviderManagerImplBase& config_provider_manager_; @@ -230,6 +222,13 @@ class ConfigSubscriptionInstanceBase : public Init::Target, SystemTime last_updated_; absl::optional config_info_; + Init::Caller init_caller_; + const Init::TargetReceiver init_target_receiver_{ + fmt::format("ConfigSubscriptionInstanceBase {}", name_), [this](Init::Caller caller) { + init_caller_ = std::move(caller); + start(); + }}; + // ConfigSubscriptionInstanceBase, MutableConfigProviderImplBase and ConfigProviderManagerImplBase // are tightly coupled with the current shared ownership model; use friend classes to explicitly // denote the binding between them. diff --git a/source/common/router/BUILD b/source/common/router/BUILD index b861e6eeb3037..bdfd1d858b08c 100644 --- a/source/common/router/BUILD +++ b/source/common/router/BUILD @@ -87,6 +87,7 @@ envoy_cc_library( "//source/common/config:subscription_factory_lib", "//source/common/config:utility_lib", "//source/common/protobuf:utility_lib", + "//source/init:callback_lib", "@envoy_api//envoy/admin/v2alpha:config_dump_cc", "@envoy_api//envoy/api/v2:rds_cc", "@envoy_api//envoy/config/filter/network/http_connection_manager/v2:http_connection_manager_cc", diff --git a/source/common/router/rds_impl.cc b/source/common/router/rds_impl.cc index 3908fa799cd96..055f74389050e 100644 --- a/source/common/router/rds_impl.cc +++ b/source/common/router/rds_impl.cc @@ -64,7 +64,12 @@ RdsRouteConfigSubscription::RdsRouteConfigSubscription( stats_({ALL_RDS_STATS(POOL_COUNTER(*scope_))}), route_config_provider_manager_(route_config_provider_manager), manager_identifier_(manager_identifier), time_source_(factory_context.timeSource()), - last_updated_(factory_context.timeSource().systemTime()) { + last_updated_(factory_context.timeSource().systemTime()), + init_target_receiver_(fmt::format("RdsRouteConfigSubscription {}", route_config_name_), + [this](Init::Caller caller) { + init_caller_ = std::move(caller); + subscription_->start({route_config_name_}, *this); + }) { Envoy::Config::Utility::checkLocalInfo("rds", factory_context.localInfo()); subscription_ = Envoy::Config::SubscriptionFactory::subscriptionFromConfigSource< @@ -129,15 +134,12 @@ void RdsRouteConfigSubscription::onConfigUpdateFailed(const EnvoyException*) { } void RdsRouteConfigSubscription::registerInitTarget(Init::Manager& init_manager) { - init_manager.registerTarget(*this, - fmt::format("RdsRouteConfigSubscription {}", route_config_name_)); + init_manager.add(init_target_receiver_); } void RdsRouteConfigSubscription::runInitializeCallbackIfAny() { - if (initialize_callback_) { - initialize_callback_(); - initialize_callback_ = nullptr; - } + init_caller_(); + init_caller_.reset(); } RdsRouteConfigProviderImpl::RdsRouteConfigProviderImpl( diff --git a/source/common/router/rds_impl.h b/source/common/router/rds_impl.h index 67e086258e21b..70f9d2c5de581 100644 --- a/source/common/router/rds_impl.h +++ b/source/common/router/rds_impl.h @@ -25,6 +25,8 @@ #include "common/common/logger.h" #include "common/protobuf/utility.h" +#include "init/callback.h" + namespace Envoy { namespace Router { @@ -94,18 +96,11 @@ class RdsRouteConfigProviderImpl; * RDS config providers. */ class RdsRouteConfigSubscription - : public Init::Target, - Envoy::Config::SubscriptionCallbacks, + : Envoy::Config::SubscriptionCallbacks, Logger::Loggable { public: ~RdsRouteConfigSubscription(); - // Init::Target - void initialize(std::function callback) override { - initialize_callback_ = callback; - subscription_->start({route_config_name_}, *this); - } - // Config::SubscriptionCallbacks // TODO(fredlas) deduplicate void onConfigUpdate(const ResourceVector& resources, const std::string& version_info) override; @@ -134,7 +129,6 @@ class RdsRouteConfigSubscription void runInitializeCallbackIfAny(); std::unique_ptr> subscription_; - std::function initialize_callback_; const std::string route_config_name_; Stats::ScopePtr scope_; RdsStats stats_; @@ -146,6 +140,9 @@ class RdsRouteConfigSubscription envoy::api::v2::RouteConfiguration route_config_proto_; std::unordered_set route_config_providers_; + Init::TargetReceiver init_target_receiver_; + Init::Caller init_caller_; + friend class RouteConfigProviderManagerImpl; friend class RdsRouteConfigProviderImpl; }; diff --git a/source/common/secret/BUILD b/source/common/secret/BUILD index 5a9f0f94ec7ce..bd6c3045ddca5 100644 --- a/source/common/secret/BUILD +++ b/source/common/secret/BUILD @@ -55,5 +55,6 @@ envoy_cc_library( "//source/common/protobuf:utility_lib", "//source/common/ssl:certificate_validation_context_config_impl_lib", "//source/common/ssl:tls_certificate_config_impl_lib", + "//source/init:callback_lib", ], ) diff --git a/source/common/secret/sds_api.cc b/source/common/secret/sds_api.cc index 2471ad98657f6..0fbc045e2519b 100644 --- a/source/common/secret/sds_api.cc +++ b/source/common/secret/sds_api.cc @@ -19,25 +19,27 @@ SdsApi::SdsApi(const LocalInfo::LocalInfo& local_info, Event::Dispatcher& dispat Api::Api& api) : local_info_(local_info), dispatcher_(dispatcher), random_(random), stats_(stats), cluster_manager_(cluster_manager), sds_config_(sds_config), sds_config_name_(sds_config_name), - secret_hash_(0), clean_up_(destructor_cb), api_(api) { + secret_hash_(0), clean_up_(destructor_cb), + api_(api), init_target_receiver_{ + fmt::format("SdsApi {}", sds_config_name), [this](Init::Caller caller) { + init_caller_ = std::move(caller); + subscription_ = + Envoy::Config::SubscriptionFactory::subscriptionFromConfigSource< + envoy::api::v2::auth::Secret>( + sds_config_, local_info_, dispatcher_, cluster_manager_, random_, + stats_, + "envoy.service.discovery.v2.SecretDiscoveryService.FetchSecrets", + "envoy.service.discovery.v2.SecretDiscoveryService.StreamSecrets", + api_); + + subscription_->start({sds_config_name_}, *this); + }} { Config::Utility::checkLocalInfo("sds", local_info_); // TODO(JimmyCYJ): Implement chained_init_manager, so that multiple init_manager // can be chained together to behave as one init_manager. In that way, we let // two listeners which share same SdsApi to register at separate init managers, and // each init manager has a chance to initialize its targets. - init_manager.registerTarget(*this, fmt::format("SdsApi {}", sds_config_name)); -} - -void SdsApi::initialize(std::function callback) { - initialize_callback_ = callback; - - subscription_ = Envoy::Config::SubscriptionFactory::subscriptionFromConfigSource< - envoy::api::v2::auth::Secret>( - sds_config_, local_info_, dispatcher_, cluster_manager_, random_, stats_, - "envoy.service.discovery.v2.SecretDiscoveryService.FetchSecrets", - "envoy.service.discovery.v2.SecretDiscoveryService.StreamSecrets", api_); - - subscription_->start({sds_config_name_}, *this); + init_manager.add(init_target_receiver_); } void SdsApi::onConfigUpdate(const ResourceVector& resources, const std::string&) { @@ -75,10 +77,8 @@ void SdsApi::onConfigUpdateFailed(const EnvoyException*) { } void SdsApi::runInitializeCallbackIfAny() { - if (initialize_callback_) { - initialize_callback_(); - initialize_callback_ = nullptr; - } + init_caller_(); + init_caller_.reset(); } } // namespace Secret diff --git a/source/common/secret/sds_api.h b/source/common/secret/sds_api.h index 6123159b372fe..a8414ade194db 100644 --- a/source/common/secret/sds_api.h +++ b/source/common/secret/sds_api.h @@ -7,7 +7,6 @@ #include "envoy/api/v2/core/config_source.pb.h" #include "envoy/config/subscription.h" #include "envoy/event/dispatcher.h" -#include "envoy/init/init.h" #include "envoy/local_info/local_info.h" #include "envoy/runtime/runtime.h" #include "envoy/secret/secret_callbacks.h" @@ -21,14 +20,15 @@ #include "common/ssl/certificate_validation_context_config_impl.h" #include "common/ssl/tls_certificate_config_impl.h" +#include "init/callback.h" + namespace Envoy { namespace Secret { /** * SDS API implementation that fetches secrets from SDS server via Subscription. */ -class SdsApi : public Init::Target, - public Config::SubscriptionCallbacks { +class SdsApi : public Config::SubscriptionCallbacks { public: SdsApi(const LocalInfo::LocalInfo& local_info, Event::Dispatcher& dispatcher, Runtime::RandomGenerator& random, Stats::Store& stats, @@ -36,9 +36,6 @@ class SdsApi : public Init::Target, const envoy::api::v2::core::ConfigSource& sds_config, const std::string& sds_config_name, std::function destructor_cb, Api::Api& api); - // Init::Target - void initialize(std::function callback) override; - // Config::SubscriptionCallbacks // TODO(fredlas) deduplicate void onConfigUpdate(const ResourceVector& resources, const std::string& version_info) override; @@ -68,12 +65,14 @@ class SdsApi : public Init::Target, const envoy::api::v2::core::ConfigSource sds_config_; std::unique_ptr> subscription_; - std::function initialize_callback_; const std::string sds_config_name_; uint64_t secret_hash_; Cleanup clean_up_; Api::Api& api_; + + Init::Caller init_caller_; + Init::TargetReceiver init_target_receiver_; }; class TlsCertificateSdsApi; diff --git a/source/common/upstream/BUILD b/source/common/upstream/BUILD index bd09d3f9329b1..70f40aaa79b61 100644 --- a/source/common/upstream/BUILD +++ b/source/common/upstream/BUILD @@ -386,7 +386,7 @@ envoy_cc_library( "//source/common/config:well_known_names", "//source/common/stats:isolated_store_lib", "//source/common/stats:stats_lib", - "//source/server:init_manager_lib", + "//source/init:init_lib", "//source/server:transport_socket_config_lib", "@envoy_api//envoy/api/v2/core:base_cc", "@envoy_api//envoy/api/v2/endpoint:endpoint_cc", @@ -454,7 +454,7 @@ envoy_cc_library( "//source/common/stats:isolated_store_lib", "//source/common/stats:stats_lib", "//source/extensions/clusters:well_known_names", - "//source/server:init_manager_lib", + "//source/init:init_lib", "//source/server:transport_socket_config_lib", "@envoy_api//envoy/api/v2/core:base_cc", "@envoy_api//envoy/api/v2/endpoint:endpoint_cc", diff --git a/source/common/upstream/cluster_factory_impl.h b/source/common/upstream/cluster_factory_impl.h index 6d4c8b95c62e5..3f2a1f6977dec 100644 --- a/source/common/upstream/cluster_factory_impl.h +++ b/source/common/upstream/cluster_factory_impl.h @@ -44,10 +44,10 @@ #include "common/upstream/resource_manager_impl.h" #include "common/upstream/upstream_impl.h" -#include "server/init_manager_impl.h" - #include "extensions/clusters/well_known_names.h" +#include "init/manager_impl.h" + namespace Envoy { namespace Upstream { diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index 825df6f4ad474..9880902eb639e 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -635,7 +635,9 @@ ClusterImplBase::ClusterImplBase( const envoy::api::v2::Cluster& cluster, Runtime::Loader& runtime, Server::Configuration::TransportSocketFactoryContext& factory_context, Stats::ScopePtr&& stats_scope, bool added_via_api) - : runtime_(runtime), init_manager_(fmt::format("Cluster {}", cluster.name())) { + : runtime_(runtime), + init_receiver_(fmt::format("ClusterImplBase {}", cluster.name()), [this]() { onInitDone(); }), + init_manager_(fmt::format("Cluster {}", cluster.name())) { factory_context.setInitManager(init_manager_); auto socket_factory = createTransportSocketFactory(cluster, factory_context); info_ = std::make_unique(cluster, factory_context.clusterManager().bindConfig(), @@ -705,7 +707,7 @@ void ClusterImplBase::onPreInitComplete() { initialization_started_ = true; ENVOY_LOG(debug, "initializing secondary cluster {} completed", info()->name()); - init_manager_.initialize([this]() { onInitDone(); }); + init_manager_.initialize(init_receiver_); } void ClusterImplBase::onInitDone() { diff --git a/source/common/upstream/upstream_impl.h b/source/common/upstream/upstream_impl.h index 878f8163c019b..dc31ae3998538 100644 --- a/source/common/upstream/upstream_impl.h +++ b/source/common/upstream/upstream_impl.h @@ -40,9 +40,8 @@ #include "common/upstream/outlier_detection_impl.h" #include "common/upstream/resource_manager_impl.h" -#include "server/init_manager_impl.h" - #include "absl/synchronization/mutex.h" +#include "init/manager_impl.h" namespace Envoy { namespace Upstream { @@ -662,7 +661,8 @@ class ClusterImplBase : public Cluster, protected Logger::Loggable caller, absl::string_view name, + absl::string_view receiver_name) + : caller_(std::move(caller)), name_(name), receiver_name_(receiver_name) {} + +Caller::operator bool() const { return caller_; } + +void Caller::reset() { caller_.reset(); } + +void Caller::operator()() const { + if (caller_) { + ENVOY_LOG(debug, "{} initialized, notifying {}", name_, receiver_name_); + caller_(); + } else { + ENVOY_LOG(debug, "{} initialized, but can't notify {} (unavailable)", name_, receiver_name_); + } +} + +Receiver::Receiver(absl::string_view name, std::function fn) : name_(name), receiver_(fn) {} + +Receiver::~Receiver() { + if (receiver_) { + ENVOY_LOG(debug, "{} destroyed", name_); + } +} + +Caller Receiver::caller(absl::string_view name) const { + return Caller(receiver_.caller(), name, name_); +} + +void Receiver::reset() { + if (receiver_) { + ENVOY_LOG(debug, "{} reset", name_); + name_.clear(); + receiver_.reset(); + } +} + +TargetCaller::TargetCaller(Common::Callback::Caller caller, absl::string_view name, + absl::string_view receiver_name) + : caller_(std::move(caller)), name_(name), receiver_name_(receiver_name) {} + +TargetCaller::operator bool() const { return caller_; } + +void TargetCaller::operator()(const Receiver& receiver) const { + if (caller_) { + ENVOY_LOG(debug, "{} initializing {}", name_, receiver_name_); + caller_(receiver.caller(receiver_name_)); + } else { + ENVOY_LOG(debug, "{} can't initialize {} (unavailable)", name_, receiver_name_); + } +} + +TargetReceiver::TargetReceiver(absl::string_view name, std::function fn) + : name_(fmt::format("target {}", name)), receiver_(fn) {} + +TargetReceiver::~TargetReceiver() { + if (receiver_) { + ENVOY_LOG(debug, "{} destroyed", name_); + } +} + +TargetCaller TargetReceiver::caller(absl::string_view name) const { + return TargetCaller(receiver_.caller(), name, name_); +} + +void TargetReceiver::reset() { + if (receiver_) { + ENVOY_LOG(debug, "{} reset", name_); + name_.clear(); + receiver_.reset(); + } +} + +absl::string_view TargetReceiver::name() const { return name_; } + +} // namespace Init +} // namespace Envoy diff --git a/source/init/callback.h b/source/init/callback.h new file mode 100644 index 0000000000000..30ca631e56c89 --- /dev/null +++ b/source/init/callback.h @@ -0,0 +1,89 @@ +#pragma once + +#include "common/callback/callback.h" +#include "common/common/logger.h" + +#include "absl/strings/string_view.h" + +namespace Envoy { +namespace Init { + +/** + * A Caller notifies a Receiver when something has initialized. This works at two levels: first, + * each initialization target holds a Caller to notify the init manager's Receiver when the target + * has initialized. And second, the manager holds a Caller to notify its client's Receiver when all + * of its targets have initialized. + * + * Caller and Receiver are simple wrappers for their counterparts in Common::Callback with logging + * added to help debug initialization and destruction ordering issues that occasionally arise. + */ +class Caller : Logger::Loggable { +public: + Caller() = default; + explicit operator bool() const; + void reset(); + void operator()() const; + +private: + friend class Receiver; + Caller(Common::Callback::Caller<> caller, absl::string_view name, + absl::string_view receiver_name); + + Common::Callback::Caller<> caller_; + std::string name_; + std::string receiver_name_; +}; + +class Receiver : Logger::Loggable { +public: + Receiver() = default; + Receiver(absl::string_view name, std::function fn); + ~Receiver(); + Caller caller(absl::string_view name) const; + void reset(); + +private: + std::string name_; + Common::Callback::Receiver<> receiver_; +}; + +/** + * A TargetCaller notifies a TargetReceiver when the target should start initialization. The + * TargetReceiver accepts a Caller, so that the target can notify the manager when it has + * initialized. + * + * As above, TargetCaller and TargetReceiver are simple wrappers for their counterparts in + * Common::Callback with logging added. + */ +class TargetCaller : Logger::Loggable { +public: + TargetCaller() = default; + explicit operator bool() const; + void operator()(const Receiver& receiver) const; + +private: + friend class TargetReceiver; + TargetCaller(Common::Callback::Caller caller, absl::string_view name, + absl::string_view receiver_name); + + Common::Callback::Caller caller_; + std::string name_; + std::string receiver_name_; +}; + +class TargetReceiver : Logger::Loggable { +public: + TargetReceiver() = default; + TargetReceiver(absl::string_view name, std::function fn); + ~TargetReceiver(); + TargetCaller caller(absl::string_view name) const; + void reset(); + absl::string_view name() const; + +private: + std::string name_; + Common::Callback::Receiver receiver_; +}; + +} // namespace Init +} // namespace Envoy diff --git a/source/init/manager_impl.cc b/source/init/manager_impl.cc new file mode 100644 index 0000000000000..9bea5b2746429 --- /dev/null +++ b/source/init/manager_impl.cc @@ -0,0 +1,56 @@ +#include "init/manager_impl.h" + +#include "common/common/assert.h" + +namespace Envoy { +namespace Init { + +ManagerImpl::ManagerImpl(absl::string_view name) + : name_(fmt::format("init manager {}", name)), state_(State::Uninitialized), count_(0), + receiver_(name_, [this]() { + if (--count_ == 0) { + state_ = State::Initialized; + caller_(); + } + }) {} + +Manager::State ManagerImpl::state() const { return state_; } + +void ManagerImpl::add(const TargetReceiver& target_receiver) { + ++count_; + TargetCaller target_caller(target_receiver.caller(name_)); + switch (state_) { + case State::Uninitialized: + ENVOY_LOG(debug, "added {} to {}", target_receiver.name(), name_); + targets_.add(std::move(target_caller)); + return; + case State::Initializing: + // It's important in this case that count_ was incremented before calling the target, because if + // the target calls back to the receiver synchronously, count_ must have been incremented + // before the receiver decrements and tests it. + target_caller(receiver_); + return; + case State::Initialized: + RELEASE_ASSERT( + false, fmt::format("attempted to add {} to initialized {}", target_receiver.name(), name_)); + } +} + +void ManagerImpl::initialize(const Receiver& receiver) { + RELEASE_ASSERT(state_ == State::Uninitialized, + fmt::format("attempted to initialize {} twice", name_)); + + if (count_ == 0) { + ENVOY_LOG(debug, "{} contains no targets", name_); + state_ = State::Initialized; + receiver.caller(name_)(); + } else { + ENVOY_LOG(debug, "{} initializing", name_); + state_ = State::Initializing; + caller_ = receiver.caller(name_); + targets_(receiver_); + } +} + +} // namespace Init +} // namespace Envoy diff --git a/source/init/manager_impl.h b/source/init/manager_impl.h new file mode 100644 index 0000000000000..f0166ea69db29 --- /dev/null +++ b/source/init/manager_impl.h @@ -0,0 +1,65 @@ +#pragma once + +#include "envoy/init/init.h" + +#include "common/callback/manager.h" +#include "common/common/logger.h" + +#include "absl/strings/string_view.h" +#include "init/callback.h" + +namespace Envoy { +namespace Init { + +/** + * Init::Manager coordinates asynchronous initialization of one or more "targets" using the safe + * callback mechanism defined in Envoy::Common::Callback. A target registers its need for + * initialization by passing a TargetReceiver to `add`. When `initialize` is called on the manager, + * it calls all targets, passing each a Caller to invoke when its initialization is complete. When + * all targets are finished initializing, the manager will finally notify its client. + * + * Note that it's safe for an initialization target to invoke a Caller from a destroyed manager, + * and likewise for the manager to invoke a Caller from a destroyed client. This does happen in + * practice, for example when a warming listener is destroyed before its route configuration is + * received (see issue #6116). + */ +class ManagerImpl : public Manager, Logger::Loggable { +public: + /** + * Constructs an initialization manager for a given caller. + * @param name human-readable name of the init manager for tracing. + */ + ManagerImpl(absl::string_view name); + + /** + * @return the current state of the manager. + */ + State state() const override; + + /** + * 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. + */ + void add(const TargetReceiver& target_receiver) override; + + /** + * 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. + * @param receiver callback to be invoked when initialization of all targets is complete. + */ + void initialize(const Receiver& receiver) override; + +private: + std::string name_; + State state_; + uint32_t count_; + Caller caller_; + Receiver receiver_; + Common::Callback::ManagerT targets_; +}; + +} // namespace Init +} // namespace Envoy diff --git a/source/server/BUILD b/source/server/BUILD index 3943a76618b28..249a582b038b1 100644 --- a/source/server/BUILD +++ b/source/server/BUILD @@ -138,17 +138,6 @@ envoy_cc_library( ], ) -envoy_cc_library( - name = "init_manager_lib", - srcs = ["init_manager_impl.cc"], - hdrs = ["init_manager_impl.h"], - deps = [ - "//include/envoy/init:init_interface", - "//source/common/common:assert_lib", - "//source/common/common:logger_lib", - ], -) - envoy_cc_library( name = "options_lib", srcs = ["options_impl.cc"] + select({ @@ -206,6 +195,7 @@ envoy_cc_library( "//source/common/config:subscription_factory_lib", "//source/common/config:utility_lib", "//source/common/protobuf:utility_lib", + "//source/init:callback_lib", "@envoy_api//envoy/api/v2:lds_cc", ], ) @@ -217,7 +207,6 @@ envoy_cc_library( deps = [ ":configuration_lib", ":drain_manager_lib", - ":init_manager_lib", ":lds_api_lib", ":transport_socket_config_lib", "//include/envoy/server:filter_config_interface", @@ -239,6 +228,7 @@ envoy_cc_library( "//source/extensions/transport_sockets:well_known_names", "//source/extensions/transport_sockets/tls:context_config_lib", "//source/extensions/transport_sockets/tls:context_lib", + "//source/init:init_lib", "@envoy_api//envoy/admin/v2alpha:config_dump_cc", "@envoy_api//envoy/api/v2:lds_cc", ], @@ -280,7 +270,6 @@ envoy_cc_library( ":configuration_lib", ":connection_handler_lib", ":guarddog_lib", - ":init_manager_lib", ":listener_manager_lib", ":test_hooks_lib", ":worker_lib", @@ -317,6 +306,7 @@ envoy_cc_library( "//source/common/stats:thread_local_store_lib", "//source/common/upstream:cluster_manager_lib", "//source/common/upstream:health_discovery_service_lib", + "//source/init:init_lib", "//source/server:overload_manager_lib", "//source/server/http:admin_lib", "@envoy_api//envoy/config/bootstrap/v2:bootstrap_cc", diff --git a/source/server/config_validation/server.cc b/source/server/config_validation/server.cc index 4cd901bc5df91..05c69c6e3311e 100644 --- a/source/server/config_validation/server.cc +++ b/source/server/config_validation/server.cc @@ -96,8 +96,7 @@ void ValidationInstance::initialize(const Options& options, singletonManager(), time_system_); config_.initialize(bootstrap, *this, *cluster_manager_factory_); http_context_.setTracer(config_.httpTracer()); - clusterManager().setInitializedCb( - [this]() -> void { init_manager_.initialize([]() -> void {}); }); + clusterManager().setInitializedCb([this]() -> void { init_manager_.initialize(init_receiver_); }); } void ValidationInstance::shutdown() { diff --git a/source/server/config_validation/server.h b/source/server/config_validation/server.h index 2369cda8777c5..ef458781ea188 100644 --- a/source/server/config_validation/server.h +++ b/source/server/config_validation/server.h @@ -26,6 +26,7 @@ #include "extensions/transport_sockets/tls/context_manager_impl.h" #include "absl/types/optional.h" +#include "init/manager_impl.h" namespace Envoy { namespace Server { @@ -142,7 +143,8 @@ class ValidationInstance : Logger::Loggable, // init_manager_ must come before any member that participates in initialization, and destructed // only after referencing members are gone, since initialization continuation can potentially // occur at any point during member lifetime. - InitManagerImpl init_manager_{"Validation server"}; + Init::Receiver init_receiver_{"(no-op)", []() {}}; + Init::ManagerImpl init_manager_{"Validation server"}; // secret_manager_ must come before listener_manager_, config_ and dispatcher_, and destructed // only after these members can no longer reference it, since: // - There may be active filter chains referencing it in listener_manager_. diff --git a/source/server/http/admin.cc b/source/server/http/admin.cc index 9c7da83e0e358..09b2b646fbed1 100644 --- a/source/server/http/admin.cc +++ b/source/server/http/admin.cc @@ -639,7 +639,7 @@ Http::Code AdminImpl::handlerServerInfo(absl::string_view, Http::HeaderMap& head server_info.set_version(VersionInfo::version()); switch (server_.initManager().state()) { - case Init::Manager::State::NotInitialized: + case Init::Manager::State::Uninitialized: server_info.set_state(envoy::admin::v2alpha::ServerInfo::PRE_INITIALIZING); break; case Init::Manager::State::Initializing: diff --git a/source/server/init_manager_impl.cc b/source/server/init_manager_impl.cc deleted file mode 100644 index 650d284217c1c..0000000000000 --- a/source/server/init_manager_impl.cc +++ /dev/null @@ -1,66 +0,0 @@ -#include "server/init_manager_impl.h" - -#include - -#include "common/common/assert.h" - -#define TRACE_INIT_MANAGER(fmt, ...) \ - ENVOY_LOG(debug, "InitManagerImpl({}): " fmt, description_, ##__VA_ARGS__) - -namespace Envoy { -namespace Server { - -InitManagerImpl::InitManagerImpl(absl::string_view description) : description_(description) { - TRACE_INIT_MANAGER("constructor"); -} - -InitManagerImpl::~InitManagerImpl() { TRACE_INIT_MANAGER("destructor"); } - -void InitManagerImpl::initialize(std::function callback) { - ASSERT(state_ == State::NotInitialized); - if (targets_.empty()) { - TRACE_INIT_MANAGER("empty targets, initialized"); - callback(); - state_ = State::Initialized; - } else { - TRACE_INIT_MANAGER("initializing"); - callback_ = callback; - state_ = State::Initializing; - // Target::initialize(...) method can modify the list to remove the item currently - // being initialized, so we increment the iterator before calling initialize. - for (auto iter = targets_.begin(); iter != targets_.end();) { - TargetWithDescription& target = *iter; - ++iter; - initializeTarget(target); - } - } -} - -void InitManagerImpl::initializeTarget(TargetWithDescription& target) { - TRACE_INIT_MANAGER("invoking initializeTarget {}", target.second); - target.first->initialize([this, &target]() -> void { - TRACE_INIT_MANAGER("completed initializeTarget {}", target.second); - ASSERT(std::find(targets_.begin(), targets_.end(), target) != targets_.end()); - targets_.remove(target); - if (targets_.empty()) { - TRACE_INIT_MANAGER("initialized"); - state_ = State::Initialized; - callback_(); - } - }); -} - -void InitManagerImpl::registerTarget(Init::Target& target, absl::string_view description) { - TRACE_INIT_MANAGER("registerTarget {}", description); - ASSERT(state_ != State::Initialized); - ASSERT(std::find(targets_.begin(), targets_.end(), - TargetWithDescription{&target, std::string(description)}) == targets_.end(), - "Registered duplicate Init::Target"); - targets_.emplace_back(&target, std::string(description)); - if (state_ == State::Initializing) { - initializeTarget(targets_.back()); - } -} - -} // namespace Server -} // namespace Envoy diff --git a/source/server/init_manager_impl.h b/source/server/init_manager_impl.h deleted file mode 100644 index 0f3ec0d2321f7..0000000000000 --- a/source/server/init_manager_impl.h +++ /dev/null @@ -1,39 +0,0 @@ -#pragma once - -#include - -#include "envoy/init/init.h" - -#include "common/common/logger.h" - -namespace Envoy { -namespace Server { - -/** - * Implementation of Init::Manager for use during post cluster manager init / pre listening. - * TODO(JimmyCYJ): Move InitManagerImpl into a new subdirectory in source/ called init/. - */ -class InitManagerImpl : public Init::Manager, Logger::Loggable { -public: - InitManagerImpl(absl::string_view description); - ~InitManagerImpl() override; - - void initialize(std::function callback); - - // Init::Manager - void registerTarget(Init::Target& target, absl::string_view description) override; - State state() const override { return state_; } - -private: - using TargetWithDescription = std::pair; - - void initializeTarget(TargetWithDescription& target); - - std::list targets_; - State state_{State::NotInitialized}; - std::function callback_; - std::string description_; // For debug tracing. -}; - -} // namespace Server -} // namespace Envoy diff --git a/source/server/lds_api.cc b/source/server/lds_api.cc index 2b5c18629b92d..0d045860b9905 100644 --- a/source/server/lds_api.cc +++ b/source/server/lds_api.cc @@ -20,19 +20,18 @@ LdsApiImpl::LdsApiImpl(const envoy::api::v2::core::ConfigSource& lds_config, Runtime::RandomGenerator& random, Init::Manager& init_manager, const LocalInfo::LocalInfo& local_info, Stats::Scope& scope, ListenerManager& lm, Api::Api& api) - : listener_manager_(lm), scope_(scope.createScope("listener_manager.lds.")), cm_(cm) { + : listener_manager_(lm), scope_(scope.createScope("listener_manager.lds.")), cm_(cm), + init_target_receiver_("LDS", [this](Init::Caller caller) { + init_caller_ = std::move(caller); + subscription_->start({}, *this); + }) { subscription_ = Envoy::Config::SubscriptionFactory::subscriptionFromConfigSource( lds_config, local_info, dispatcher, cm, random, *scope_, "envoy.api.v2.ListenerDiscoveryService.FetchListeners", "envoy.api.v2.ListenerDiscoveryService.StreamListeners", api); Config::Utility::checkLocalInfo("lds", local_info); - init_manager.registerTarget(*this, "LDS"); -} - -void LdsApiImpl::initialize(std::function callback) { - initialize_callback_ = callback; - subscription_->start({}, *this); + init_manager.add(init_target_receiver_); } void LdsApiImpl::onConfigUpdate(const ResourceVector& resources, const std::string& version_info) { @@ -95,10 +94,8 @@ void LdsApiImpl::onConfigUpdateFailed(const EnvoyException*) { } void LdsApiImpl::runInitializeCallbackIfAny() { - if (initialize_callback_) { - initialize_callback_(); - initialize_callback_ = nullptr; - } + init_caller_(); + init_caller_.reset(); } } // namespace Server diff --git a/source/server/lds_api.h b/source/server/lds_api.h index 713ead3f118a6..536ecde271a83 100644 --- a/source/server/lds_api.h +++ b/source/server/lds_api.h @@ -11,6 +11,8 @@ #include "common/common/logger.h" +#include "init/callback.h" + namespace Envoy { namespace Server { @@ -18,7 +20,6 @@ namespace Server { * LDS API implementation that fetches via Subscription. */ class LdsApiImpl : public LdsApi, - public Init::Target, Config::SubscriptionCallbacks, Logger::Loggable { public: @@ -30,9 +31,6 @@ class LdsApiImpl : public LdsApi, // Server::LdsApi std::string versionInfo() const override { return version_info_; } - // Init::Target - void initialize(std::function callback) override; - // Config::SubscriptionCallbacks // TODO(fredlas) deduplicate void onConfigUpdate(const ResourceVector& resources, const std::string& version_info) override; @@ -53,7 +51,9 @@ class LdsApiImpl : public LdsApi, ListenerManager& listener_manager_; Stats::ScopePtr scope_; Upstream::ClusterManager& cm_; - std::function initialize_callback_; + + Init::TargetReceiver init_target_receiver_; + Init::Caller init_caller_; }; } // namespace Server diff --git a/source/server/listener_manager_impl.cc b/source/server/listener_manager_impl.cc index ac8bd1a6c2f21..9d2f202215fef 100644 --- a/source/server/listener_manager_impl.cc +++ b/source/server/listener_manager_impl.cc @@ -155,7 +155,14 @@ ProdListenerComponentFactory::createDrainManager(envoy::api::v2::Listener::Drain ListenerImpl::ListenerImpl(const envoy::api::v2::Listener& config, const std::string& version_info, ListenerManagerImpl& parent, const std::string& name, bool modifiable, bool workers_started, uint64_t hash) - : parent_(parent), address_(Network::Address::resolveProtoAddress(config.address())), + : init_receiver_(fmt::format("ListenerImpl {}", name), + [this]() -> void { + if (!initialize_canceled_) { + parent_.onListenerWarmed(*this); + } + }), + dynamic_init_manager_(fmt::format("Listener {}", name)), parent_(parent), + address_(Network::Address::resolveProtoAddress(config.address())), socket_type_(Network::Utility::protobufAddressSocketType(config.address())), global_scope_(parent_.server_.stats().createScope("")), listener_scope_( @@ -167,7 +174,6 @@ ListenerImpl::ListenerImpl(const envoy::api::v2::Listener& config, const std::st PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, per_connection_buffer_limit_bytes, 1024 * 1024)), listener_tag_(parent_.factory_.nextListenerTag()), name_(name), modifiable_(modifiable), workers_started_(workers_started), hash_(hash), - dynamic_init_manager_(fmt::format("Listener {}", name)), local_drain_manager_(parent.factory_.createDrainManager(config.drain_type())), config_(config), version_info_(version_info), listener_filters_timeout_( @@ -631,11 +637,7 @@ void ListenerImpl::initialize() { // per listener init manager. See ~ListenerImpl() for why we gate the onListenerWarmed() call // with initialize_canceled_. if (workers_started_) { - dynamic_init_manager_.initialize([this]() -> void { - if (!initialize_canceled_) { - parent_.onListenerWarmed(*this); - } - }); + dynamic_init_manager_.initialize(init_receiver_); } } diff --git a/source/server/listener_manager_impl.h b/source/server/listener_manager_impl.h index 0c616215aa182..8c73377a84771 100644 --- a/source/server/listener_manager_impl.h +++ b/source/server/listener_manager_impl.h @@ -15,9 +15,10 @@ #include "common/network/cidr_range.h" #include "common/network/lc_trie.h" -#include "server/init_manager_impl.h" #include "server/lds_api.h" +#include "init/manager_impl.h" + namespace Envoy { namespace Server { @@ -382,6 +383,9 @@ class ListenerImpl : public Network::ListenerConfig, static bool isWildcardServerName(const std::string& name); + Init::Receiver init_receiver_; + Init::ManagerImpl dynamic_init_manager_; + // Mapping of FilterChain's configured destination ports, IPs, server names, transport protocols // and application protocols, using structures defined above. DestinationPortsMap destination_ports_map_; @@ -400,7 +404,6 @@ class ListenerImpl : public Network::ListenerConfig, const bool modifiable_; const bool workers_started_; const uint64_t hash_; - InitManagerImpl dynamic_init_manager_; bool initialize_canceled_{}; std::vector listener_filter_factories_; DrainManagerPtr local_drain_manager_; diff --git a/source/server/server.cc b/source/server/server.cc index 62e9ae99a902f..8d00372ac2c77 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -52,7 +52,13 @@ InstanceImpl::InstanceImpl(const Options& options, Event::TimeSystem& time_syste ComponentFactory& component_factory, Runtime::RandomGeneratorPtr&& random_generator, ThreadLocal::Instance& tls, Thread::ThreadFactory& thread_factory) - : secret_manager_(std::make_unique()), shutdown_(false), + : init_receiver_("InstanceImpl", + [this]() { + if (!shutdown_) { + startWorkers(); + } + }), + secret_manager_(std::make_unique()), shutdown_(false), options_(options), time_source_(time_system), restarter_(restarter), start_time_(time(nullptr)), original_start_time_(start_time_), stats_store_(store), thread_local_(tls), api_(new Api::Impl(thread_factory, store, time_system)), @@ -397,8 +403,8 @@ uint64_t InstanceImpl::numConnections() { return listener_manager_->numConnectio RunHelper::RunHelper(Instance& instance, const Options& options, Event::Dispatcher& dispatcher, Upstream::ClusterManager& cm, AccessLog::AccessLogManager& access_log_manager, - InitManagerImpl& init_manager, OverloadManager& overload_manager, - std::function workers_start_cb) { + const Init::Receiver& init_receiver, Init::ManagerImpl& init_manager, + OverloadManager& overload_manager) { // Setup signals. if (options.signalHandlingEnabled()) { @@ -430,7 +436,7 @@ RunHelper::RunHelper(Instance& instance, const Options& options, Event::Dispatch // this can fire immediately if all clusters have already initialized. Also note that we need // to guard against shutdown at two different levels since SIGTERM can come in once the run loop // starts. - cm.setInitializedCb([&instance, &init_manager, &cm, workers_start_cb]() { + cm.setInitializedCb([&instance, &init_receiver, &init_manager, &cm]() { if (instance.isShutdown()) { return; } @@ -441,16 +447,7 @@ RunHelper::RunHelper(Instance& instance, const Options& options, Event::Dispatch cm.adsMux().pause(Config::TypeUrl::get().RouteConfiguration); ENVOY_LOG(info, "all clusters initialized. initializing init manager"); - - // Note: the lambda below should not capture "this" since the RunHelper object may - // have been destructed by the time it gets executed. - init_manager.initialize([&instance, workers_start_cb]() { - if (instance.isShutdown()) { - return; - } - - workers_start_cb(); - }); + init_manager.initialize(init_receiver); // Now that we're execute all the init callbacks we can resume RDS // as we've subscribed to all the statically defined RDS resources. @@ -462,8 +459,8 @@ void InstanceImpl::run() { // We need the RunHelper to be available to call from InstanceImpl::shutdown() below, so // we save it as a member variable. run_helper_ = std::make_unique(*this, options_, *dispatcher_, clusterManager(), - access_log_manager_, init_manager_, overloadManager(), - [this]() -> void { startWorkers(); }); + access_log_manager_, init_receiver_, init_manager_, + overloadManager()); // Run the main dispatch loop waiting to exit. ENVOY_LOG(info, "starting main dispatch loop"); diff --git a/source/server/server.h b/source/server/server.h index aa026fb65ce13..b18b6145b4ef8 100644 --- a/source/server/server.h +++ b/source/server/server.h @@ -28,7 +28,6 @@ #include "server/configuration_impl.h" #include "server/http/admin.h" -#include "server/init_manager_impl.h" #include "server/listener_manager_impl.h" #include "server/overload_manager_impl.h" #include "server/test_hooks.h" @@ -38,6 +37,7 @@ #include "extensions/transport_sockets/tls/context_manager_impl.h" #include "absl/types/optional.h" +#include "init/manager_impl.h" namespace Envoy { namespace Server { @@ -123,8 +123,8 @@ class RunHelper : Logger::Loggable { public: RunHelper(Instance& instance, const Options& options, Event::Dispatcher& dispatcher, Upstream::ClusterManager& cm, AccessLog::AccessLogManager& access_log_manager, - InitManagerImpl& init_manager, OverloadManager& overload_manager, - std::function workers_start_cb); + const Init::Receiver& receiver, Init::ManagerImpl& init_manager, + OverloadManager& overload_manager); private: Event::SignalEventPtr sigterm_; @@ -203,7 +203,8 @@ class InstanceImpl : Logger::Loggable, public Instance { // init_manager_ must come before any member that participates in initialization, and destructed // only after referencing members are gone, since initialization continuation can potentially // occur at any point during member lifetime. - InitManagerImpl init_manager_{"Server"}; + Init::Receiver init_receiver_; + Init::ManagerImpl init_manager_{"Server"}; // secret_manager_ must come before listener_manager_, config_ and dispatcher_, and destructed // only after these members can no longer reference it, since: // - There may be active filter chains referencing it in listener_manager_. diff --git a/test/common/callback/BUILD b/test/common/callback/BUILD new file mode 100644 index 0000000000000..b4179daf1ae1a --- /dev/null +++ b/test/common/callback/BUILD @@ -0,0 +1,25 @@ +licenses(["notice"]) # Apache 2 + +load( + "//bazel:envoy_build_system.bzl", + "envoy_cc_test", + "envoy_package", +) + +envoy_package() + +envoy_cc_test( + name = "callback_test", + srcs = ["callback_test.cc"], + deps = [ + "//source/common/callback", + ], +) + +envoy_cc_test( + name = "manager_test", + srcs = ["manager_test.cc"], + deps = [ + "//source/common/callback:manager", + ], +) diff --git a/test/common/callback/callback_test.cc b/test/common/callback/callback_test.cc new file mode 100644 index 0000000000000..b63f633ece32f --- /dev/null +++ b/test/common/callback/callback_test.cc @@ -0,0 +1,69 @@ +#include "common/callback/callback.h" + +#include "gmock/gmock.h" +#include "gtest/gtest.h" + +using testing::_; + +namespace Envoy { +namespace Common { +namespace Callback { +namespace { + +using R = Receiver; +using C = Caller; + +struct MockClient { + MOCK_CONST_METHOD1(callback, void(uint32_t)); + R receiver_{[this](uint32_t data) { callback(data); }}; +}; + +TEST(CallbackTest, CallAvailableReceiver) { + MockClient client; + EXPECT_TRUE(client.receiver_); + + C caller(client.receiver_.caller()); + EXPECT_TRUE(caller); + + EXPECT_CALL(client, callback(123)); + caller(123); +} + +TEST(CallbackTest, CallDestroyedReceiver) { + C caller; + { + MockClient client; + caller = client.receiver_.caller(); + EXPECT_CALL(client, callback(_)).Times(0); + } + EXPECT_FALSE(caller); + caller(123); +} + +TEST(CallbackTest, CallResetReceiver) { + MockClient client; + C caller(client.receiver_.caller()); + client.receiver_.reset(); + + EXPECT_FALSE(client.receiver_); + EXPECT_FALSE(caller); + EXPECT_CALL(client, callback(_)).Times(0); + caller(123); +} + +TEST(CallbackTest, DefaultInitializedCaller) { + C caller; + EXPECT_FALSE(caller); +} + +TEST(CallbackTest, ResetCaller) { + R receiver{[](uint32_t) {}}; + C caller(receiver.caller()); + caller.reset(); + EXPECT_FALSE(caller); +} + +} // namespace +} // namespace Callback +} // namespace Common +} // namespace Envoy diff --git a/test/common/callback/manager_test.cc b/test/common/callback/manager_test.cc new file mode 100644 index 0000000000000..4f0b64d0a840c --- /dev/null +++ b/test/common/callback/manager_test.cc @@ -0,0 +1,67 @@ +#include +#include + +#include "common/callback/manager.h" + +#include "gmock/gmock.h" +#include "gtest/gtest.h" + +using testing::_; +using testing::InSequence; +using testing::Return; + +namespace Envoy { +namespace Common { +namespace Callback { +namespace { + +struct MockReceiver { + MOCK_CONST_METHOD0(boolConversionOperator, bool()); + MOCK_CONST_METHOD1(functionCallOperator, void(uint32_t)); +}; + +struct MockCaller { + MockCaller(const MockReceiver& receiver) : receiver_(receiver) {} + operator bool() const { return receiver_.boolConversionOperator(); } + void operator()(uint32_t data) const { receiver_.functionCallOperator(data); } + const MockReceiver& receiver_; +}; + +using M = ManagerT; + +TEST(ManagerTest, RemoveUnavailable) { + InSequence s; + M mgr; + + MockReceiver good_receivers[5] = {}, bad_receivers[5] = {}; + for (const auto& receiver : good_receivers) { + // "good" receivers are invoked, and remain available after invocation + EXPECT_CALL(receiver, functionCallOperator(123)); + EXPECT_CALL(receiver, boolConversionOperator()).WillOnce(Return(true)); + mgr.add(MockCaller(receiver)); + } + for (const auto& receiver : bad_receivers) { + // "bad" receivers are invoked, and become unavailable after invocation + EXPECT_CALL(receiver, functionCallOperator(123)); + EXPECT_CALL(receiver, boolConversionOperator()).WillOnce(Return(false)); + mgr.add(MockCaller(receiver)); + } + mgr(123); + + for (const auto& receiver : good_receivers) { + // "good" receivers are invoked a second time + EXPECT_CALL(receiver, functionCallOperator(456)); + EXPECT_CALL(receiver, boolConversionOperator()); + } + for (const auto& receiver : bad_receivers) { + // "bad" receivers have had their callers removed from the manager + EXPECT_CALL(receiver, functionCallOperator(_)).Times(0); + EXPECT_CALL(receiver, boolConversionOperator()).Times(0); + } + mgr(456); +} + +} // namespace +} // namespace Callback +} // namespace Common +} // namespace Envoy diff --git a/test/common/config/config_provider_impl_test.cc b/test/common/config/config_provider_impl_test.cc index f25f77a0b1dd8..3d22720220a44 100644 --- a/test/common/config/config_provider_impl_test.cc +++ b/test/common/config/config_provider_impl_test.cc @@ -235,7 +235,8 @@ test::common::config::DummyConfig parseDummyConfigFromYaml(const std::string& ya // subscriptions, config protos and data structures generated as a result of the // configurations (i.e., the ConfigProvider::Config). TEST_F(ConfigProviderImplTest, SharedOwnership) { - factory_context_.init_manager_.initialize(); + Init::Receiver receiver("(no-op)", []() {}); + factory_context_.init_manager_.initialize(receiver); envoy::api::v2::core::ApiConfigSource config_source_proto; config_source_proto.set_api_type(envoy::api::v2::core::ApiConfigSource::GRPC); diff --git a/test/common/router/BUILD b/test/common/router/BUILD index 89e87cda4ea0e..c43ec79f5b8bc 100644 --- a/test/common/router/BUILD +++ b/test/common/router/BUILD @@ -61,6 +61,7 @@ envoy_cc_test( "//source/common/http:message_lib", "//source/common/json:json_loader_lib", "//source/common/router:rds_lib", + "//source/init:callback_lib", "//source/server/http:admin_lib", "//test/mocks/init:init_mocks", "//test/mocks/local_info:local_info_mocks", diff --git a/test/common/router/rds_impl_test.cc b/test/common/router/rds_impl_test.cc index 266793ce209ad..7444ba530add5 100644 --- a/test/common/router/rds_impl_test.cc +++ b/test/common/router/rds_impl_test.cc @@ -14,6 +14,7 @@ #include "server/http/admin.h" +#include "test/mocks/common.h" #include "test/mocks/init/mocks.h" #include "test/mocks/local_info/mocks.h" #include "test/mocks/server/mocks.h" @@ -25,6 +26,7 @@ #include "gmock/gmock.h" #include "gtest/gtest.h" +#include "init/callback.h" using testing::_; using testing::InSequence; @@ -49,7 +51,15 @@ parseHttpConnectionManagerFromJson(const std::string& json_string, const Stats:: class RdsTestBase : public testing::Test { public: - RdsTestBase() : request_(&factory_context_.cluster_manager_.async_client_) {} + RdsTestBase() : request_(&factory_context_.cluster_manager_.async_client_) { + ON_CALL(factory_context_.init_manager_, add(_)) + .WillByDefault(Invoke([this](const Init::TargetReceiver& target_receiver) { + init_target_caller_ = target_receiver.caller("test"); + })); + ON_CALL(factory_context_.init_manager_, initialize(_)) + .WillByDefault( + Invoke([this](const Init::Receiver&) { init_target_caller_(init_receiver_); })); + } void expectRequest() { EXPECT_CALL(factory_context_.cluster_manager_, httpAsyncClientForCluster("foo_cluster")); @@ -75,6 +85,9 @@ class RdsTestBase : public testing::Test { Http::MockAsyncClientRequest request_; Http::AsyncClient::Callbacks* callbacks_{}; Event::MockTimer* interval_timer_{}; + ReadyWatcher initialized_; + Init::TargetCaller init_target_caller_; + Init::Receiver init_receiver_{"test", [this]() { initialized_.ready(); }}; }; class RdsImplTest : public RdsTestBase { @@ -112,12 +125,11 @@ class RdsImplTest : public RdsTestBase { EXPECT_CALL(cluster, info()); EXPECT_CALL(*cluster.info_, type()); interval_timer_ = new Event::MockTimer(&factory_context_.dispatcher_); - EXPECT_CALL(factory_context_.init_manager_, registerTarget(_, _)); rds_ = RouteConfigProviderUtil::create(parseHttpConnectionManagerFromJson(config_json, scope_), factory_context_, "foo.", *route_config_provider_manager_); expectRequest(); - factory_context_.init_manager_.initialize(); + factory_context_.init_manager_.initialize(Init::Receiver()); } NiceMock scope_; @@ -198,7 +210,7 @@ TEST_F(RdsImplTest, DestroyDuringInitialize) { InSequence s; setup(); - EXPECT_CALL(factory_context_.init_manager_.initialized_, ready()); + EXPECT_CALL(initialized_, ready()); EXPECT_CALL(request_, cancel()); rds_.reset(); } @@ -232,7 +244,7 @@ TEST_F(RdsImplTest, Basic) { Http::HeaderMapPtr{new Http::TestHeaderMapImpl{{":status", "200"}}})); message->body() = std::make_unique(response1_json); - EXPECT_CALL(factory_context_.init_manager_.initialized_, ready()); + EXPECT_CALL(initialized_, ready()); EXPECT_CALL(*interval_timer_, enableTimer(_)); callbacks_->onSuccess(std::move(message)); EXPECT_EQ(nullptr, rds_->config()->route(Http::TestHeaderMapImpl{{":authority", "foo"}}, 0)); @@ -342,7 +354,7 @@ TEST_F(RdsImplTest, Failure) { Http::HeaderMapPtr{new Http::TestHeaderMapImpl{{":status", "200"}}})); message->body() = std::make_unique(response_json); - EXPECT_CALL(factory_context_.init_manager_.initialized_, ready()); + EXPECT_CALL(initialized_, ready()); EXPECT_CALL(*interval_timer_, enableTimer(_)); callbacks_->onSuccess(std::move(message)); @@ -465,7 +477,7 @@ name: foo // Static + dynamic. setup(); expectRequest(); - factory_context_.init_manager_.initialize(); + factory_context_.init_manager_.initialize(Init::Receiver()); const std::string response1_json = R"EOF( { @@ -483,7 +495,7 @@ name: foo Http::MessagePtr message(new Http::ResponseMessageImpl( Http::HeaderMapPtr{new Http::TestHeaderMapImpl{{":status", "200"}}})); message->body() = std::make_unique(response1_json); - EXPECT_CALL(factory_context_.init_manager_.initialized_, ready()); + EXPECT_CALL(initialized_, ready()); EXPECT_CALL(*interval_timer_, enableTimer(_)); callbacks_->onSuccess(std::move(message)); message_ptr = factory_context_.admin_.config_tracker_.config_tracker_callbacks_["routes"](); @@ -519,7 +531,7 @@ name: foo TEST_F(RouteConfigProviderManagerImplTest, Basic) { Buffer::OwnedImpl data; - factory_context_.init_manager_.initialize(); + factory_context_.init_manager_.initialize(Init::Receiver()); // Get a RouteConfigProvider. This one should create an entry in the RouteConfigProviderManager. setup(); @@ -616,9 +628,9 @@ TEST_F(RouteConfigProviderManagerImplTest, ValidateFail) { TEST_F(RouteConfigProviderManagerImplTest, onConfigUpdateEmpty) { setup(); - factory_context_.init_manager_.initialize(); + factory_context_.init_manager_.initialize(Init::Receiver()); auto& provider_impl = dynamic_cast(*provider_.get()); - EXPECT_CALL(factory_context_.init_manager_.initialized_, ready()); + EXPECT_CALL(initialized_, ready()); provider_impl.subscription().onConfigUpdate({}, ""); EXPECT_EQ( 1UL, factory_context_.scope_.counter("foo_prefix.rds.foo_route_config.update_empty").value()); @@ -626,12 +638,12 @@ TEST_F(RouteConfigProviderManagerImplTest, onConfigUpdateEmpty) { TEST_F(RouteConfigProviderManagerImplTest, onConfigUpdateWrongSize) { setup(); - factory_context_.init_manager_.initialize(); + factory_context_.init_manager_.initialize(Init::Receiver()); auto& provider_impl = dynamic_cast(*provider_.get()); Protobuf::RepeatedPtrField route_configs; route_configs.Add(); route_configs.Add(); - EXPECT_CALL(factory_context_.init_manager_.initialized_, ready()); + EXPECT_CALL(initialized_, ready()); EXPECT_THROW_WITH_MESSAGE(provider_impl.subscription().onConfigUpdate(route_configs, ""), EnvoyException, "Unexpected RDS resource length: 2"); } diff --git a/test/common/secret/sds_api_test.cc b/test/common/secret/sds_api_test.cc index 87f6aacb16819..4b25c74593564 100644 --- a/test/common/secret/sds_api_test.cc +++ b/test/common/secret/sds_api_test.cc @@ -38,8 +38,12 @@ TEST_F(SdsApiTest, BasicTest) { ::testing::InSequence s; const envoy::service::discovery::v2::SdsDummy dummy; NiceMock server; - NiceMock init_manager; - EXPECT_CALL(init_manager, registerTarget(_, _)); + Init::MockManager init_manager; + Init::TargetCaller init_target_caller; + EXPECT_CALL(init_manager, add(_)) + .WillOnce(Invoke([&init_target_caller](const Init::TargetReceiver& target_receiver) { + init_target_caller = target_receiver.caller("test"); + })); envoy::api::v2::core::ConfigSource config_source; config_source.mutable_api_config_source()->set_api_type( @@ -61,8 +65,11 @@ TEST_F(SdsApiTest, BasicTest) { EXPECT_CALL(*factory, create()).WillOnce(Invoke([grpc_client] { return Grpc::AsyncClientPtr{grpc_client}; })); - EXPECT_CALL(init_manager.initialized_, ready()); - init_manager.initialize(); + + ReadyWatcher initialized; + Init::Receiver init_receiver("test", [&initialized]() { initialized.ready(); }); + EXPECT_CALL(initialized, ready()); + init_target_caller(init_receiver); } // Validate that TlsCertificateSdsApi updates secrets successfully if a good secret diff --git a/test/init/BUILD b/test/init/BUILD new file mode 100644 index 0000000000000..8e13e2f766f0c --- /dev/null +++ b/test/init/BUILD @@ -0,0 +1,17 @@ +licenses(["notice"]) # Apache 2 + +load( + "//bazel:envoy_build_system.bzl", + "envoy_cc_test", + "envoy_package", +) + +envoy_package() + +envoy_cc_test( + name = "init_test", + srcs = ["init_test.cc"], + deps = [ + "//source/init:init_lib", + ], +) diff --git a/test/init/init_test.cc b/test/init/init_test.cc new file mode 100644 index 0000000000000..df3336b50f3bc --- /dev/null +++ b/test/init/init_test.cc @@ -0,0 +1,270 @@ +#include + +#include "gmock/gmock.h" +#include "gtest/gtest.h" +#include "init/manager_impl.h" + +using testing::_; +using testing::InSequence; +using testing::Invoke; + +namespace Envoy { +namespace Init { +namespace { + +struct MockClient { + MOCK_CONST_METHOD0(callback, void()); + Receiver receiver_{"test", [this]() { callback(); }}; + + testing::internal::TypedExpectation& expectCallback() { + return EXPECT_CALL(*this, callback()); + } +}; + +struct MockTarget { + MockTarget(absl::string_view name) + : target_receiver_{name, [this](Caller caller) { initialize(caller); }} {} + MockTarget(absl::string_view name, Manager& m) : MockTarget(name) { m.add(target_receiver_); } + MOCK_CONST_METHOD1(initialize, void(Caller)); + TargetReceiver target_receiver_; + Caller caller_; + + testing::internal::TypedExpectation& expectInitialize() { + return EXPECT_CALL(*this, initialize(_)); + } + + // initialize() will complete immediately + void expectInitializeImmediate() { + expectInitialize().WillOnce(Invoke([](Caller caller) { caller(); })); + } + + // initialize() will save its caller to complete asynchronously + void expectInitializeAsync() { + expectInitialize().WillOnce(Invoke([this](Caller caller) { caller_ = caller; })); + } +}; + +void expectUninitialized(const Manager& m) { EXPECT_EQ(Manager::State::Uninitialized, m.state()); } +void expectInitializing(const Manager& m) { EXPECT_EQ(Manager::State::Initializing, m.state()); } +void expectInitialized(const Manager& m) { EXPECT_EQ(Manager::State::Initialized, m.state()); } + +TEST(ManagerTest, AddImmediateTargetsWhenUninitialized) { + InSequence s; + + ManagerImpl m("test"); + expectUninitialized(m); + + MockTarget t1("t1", m); + t1.expectInitializeImmediate(); + + MockTarget t2("t2", m); + t2.expectInitializeImmediate(); + + // initialization should complete immediately + MockClient c; + c.expectCallback(); + m.initialize(c.receiver_); + expectInitialized(m); +} + +TEST(ManagerTest, AddAsyncTargetsWhenUninitialized) { + InSequence s; + + ManagerImpl m("test"); + expectUninitialized(m); + + MockTarget t1("t1", m); + t1.expectInitializeAsync(); + + MockTarget t2("t2", m); + t2.expectInitializeAsync(); + + // initialization should begin + MockClient c; + m.initialize(c.receiver_); + expectInitializing(m); + + // should still be initializing after first target initializes + t1.caller_(); + expectInitializing(m); + + // initialization should finish after second target initializes + c.expectCallback(); + t2.caller_(); + expectInitialized(m); +} + +TEST(ManagerTest, AddMixedTargetsWhenUninitialized) { + InSequence s; + + ManagerImpl m("test"); + expectUninitialized(m); + + MockTarget t1("t1", m); + t1.expectInitializeImmediate(); + + MockTarget t2("t2", m); + t2.expectInitializeAsync(); + + // initialization should begin, and first target will initialize immediately + MockClient c; + m.initialize(c.receiver_); + expectInitializing(m); + + // initialization should finish after second target initializes + c.expectCallback(); + t2.caller_(); + expectInitialized(m); +} + +TEST(ManagerTest, AddImmediateTargetWhenInitializing) { + InSequence s; + + ManagerImpl m("test"); + expectUninitialized(m); + + // need an initial async target so initialization doesn't finish immediately + MockTarget t1("t1", m); + t1.expectInitializeAsync(); + + MockClient c; + m.initialize(c.receiver_); + expectInitializing(m); + + // adding an immediate target shouldn't finish initialization + MockTarget t2("t2"); + t2.expectInitializeImmediate(); + m.add(t2.target_receiver_); + expectInitializing(m); + + c.expectCallback(); + t1.caller_(); + expectInitialized(m); +} + +TEST(ManagerTest, AddWhenInitialized) { + InSequence s; + + ManagerImpl m("test"); + expectUninitialized(m); + + // initialize + MockTarget t1("t1", m); + t1.expectInitializeImmediate(); + + MockClient c; + c.expectCallback(); + m.initialize(c.receiver_); + expectInitialized(m); + + MockTarget t2("t2"); + EXPECT_DEATH(m.add(t2.target_receiver_), + "attempted to add target t2 to initialized init manager test"); +} + +TEST(ManagerTest, InitializeEmpty) { + InSequence s; + + ManagerImpl m("test"); + expectUninitialized(m); + + MockClient c; + c.expectCallback(); + m.initialize(c.receiver_); + expectInitialized(m); +} + +TEST(ManagerTest, InitializeWhenInitializing) { + InSequence s; + + ManagerImpl m("test"); + expectUninitialized(m); + + MockTarget t1("t1", m); + t1.expectInitializeAsync(); + + // initialization should begin + MockClient c; + m.initialize(c.receiver_); + expectInitializing(m); + + EXPECT_DEATH(m.initialize(c.receiver_), "attempted to initialize init manager test twice"); +} + +TEST(ManagerTest, InitializeWhenInitialized) { + InSequence s; + + ManagerImpl m("test"); + expectUninitialized(m); + + MockTarget t1("t1", m); + t1.expectInitializeImmediate(); + + // initialize + MockClient c; + c.expectCallback(); + m.initialize(c.receiver_); + expectInitialized(m); + + EXPECT_DEATH(m.initialize(c.receiver_), "attempted to initialize init manager test twice"); +} + +TEST(ManagerTest, UnavailableTarget) { + InSequence s; + + ManagerImpl m("test"); + expectUninitialized(m); + + MockTarget t1("t1", m); + t1.target_receiver_.reset(); + t1.expectInitialize().Times(0); + + // initialization should begin and get stuck + MockClient c; + c.expectCallback().Times(0); + m.initialize(c.receiver_); + expectInitializing(m); +} + +TEST(ManagerTest, UnavailableManager) { + InSequence s; + + auto m = new ManagerImpl("test"); + expectUninitialized(*m); + + MockTarget t1("t1", *m); + t1.expectInitializeAsync(); + + // initialization should begin + MockClient c; + m->initialize(c.receiver_); + expectInitializing(*m); + + // initialization should get stuck after init manager is destroyed + delete m; + c.expectCallback().Times(0); + t1.caller_(); +} + +TEST(ManagerTest, UnavailableClient) { + InSequence s; + + ManagerImpl m("test"); + expectUninitialized(m); + + MockTarget t1("t1", m); + t1.expectInitializeAsync(); + + // initialization should begin + auto c = new MockClient(); + m.initialize(c->receiver_); + expectInitializing(m); + + // initialization should not crash after client is destroyed + delete c; + t1.caller_(); +} + +} // namespace +} // namespace Init +} // namespace Envoy diff --git a/test/mocks/init/BUILD b/test/mocks/init/BUILD index 682c862e1a4e7..9691f6e51b23e 100644 --- a/test/mocks/init/BUILD +++ b/test/mocks/init/BUILD @@ -10,10 +10,9 @@ envoy_package() envoy_cc_mock( name = "init_mocks", - srcs = ["mocks.cc"], hdrs = ["mocks.h"], deps = [ "//include/envoy/init:init_interface", - "//test/mocks:common_lib", + "//source/init:init_lib", ], ) diff --git a/test/mocks/init/mocks.cc b/test/mocks/init/mocks.cc deleted file mode 100644 index f968ad7c290bc..0000000000000 --- a/test/mocks/init/mocks.cc +++ /dev/null @@ -1,33 +0,0 @@ -#include "mocks.h" - -#include - -#include "gmock/gmock.h" -#include "gtest/gtest.h" - -using testing::_; -using testing::Invoke; - -namespace Envoy { -namespace Init { - -MockTarget::MockTarget() { - ON_CALL(*this, initialize(_)) - .WillByDefault(Invoke([this](std::function callback) -> void { - EXPECT_EQ(nullptr, callback_); - callback_ = callback; - })); -} - -MockTarget::~MockTarget() {} - -MockManager::MockManager() { - ON_CALL(*this, registerTarget(_, _)) - .WillByDefault(Invoke( - [this](Target& target, absl::string_view) -> void { targets_.push_back(&target); })); -} - -MockManager::~MockManager() {} - -} // namespace Init -} // namespace Envoy diff --git a/test/mocks/init/mocks.h b/test/mocks/init/mocks.h index e8f6d093a8270..1ee6c62f4c157 100644 --- a/test/mocks/init/mocks.h +++ b/test/mocks/init/mocks.h @@ -1,44 +1,18 @@ #pragma once -#include -#include - #include "envoy/init/init.h" -#include "test/mocks/common.h" - #include "gmock/gmock.h" +#include "init/manager_impl.h" namespace Envoy { namespace Init { -class MockTarget : public Target { -public: - MockTarget(); - ~MockTarget(); - - MOCK_METHOD1(initialize, void(std::function callback)); - - std::function callback_; -}; - class MockManager : public Manager { public: - MockManager(); - ~MockManager(); - - void initialize() { - for (auto target : targets_) { - target->initialize([this]() -> void { initialized_.ready(); }); - } - } - - // Init::Manager - MOCK_METHOD2(registerTarget, void(Target& target, absl::string_view description)); - MOCK_CONST_METHOD0(state, State()); - - std::list targets_; - ReadyWatcher initialized_; + MOCK_CONST_METHOD0(state, Manager::State()); + MOCK_METHOD1(add, void(const TargetReceiver&)); + MOCK_METHOD1(initialize, void(const Receiver&)); }; } // namespace Init diff --git a/test/server/BUILD b/test/server/BUILD index 1c64006680419..8cae70848f762 100644 --- a/test/server/BUILD +++ b/test/server/BUILD @@ -80,16 +80,6 @@ envoy_cc_test( ], ) -envoy_cc_test( - name = "init_manager_impl_test", - srcs = ["init_manager_impl_test.cc"], - deps = [ - "//source/server:init_manager_lib", - "//test/mocks:common_lib", - "//test/mocks/init:init_mocks", - ], -) - envoy_cc_test( name = "guarddog_impl_test", srcs = ["guarddog_impl_test.cc"], diff --git a/test/server/http/admin_test.cc b/test/server/http/admin_test.cc index 76723982e4565..b4b9670c098af 100644 --- a/test/server/http/admin_test.cc +++ b/test/server/http/admin_test.cc @@ -1190,7 +1190,7 @@ TEST_P(AdminInstanceTest, GetRequest) { Http::HeaderMapImpl response_headers; std::string body; - ON_CALL(initManager, state()).WillByDefault(Return(Init::Manager::State::NotInitialized)); + ON_CALL(initManager, state()).WillByDefault(Return(Init::Manager::State::Uninitialized)); EXPECT_EQ(Http::Code::OK, admin_.request("/server_info", "GET", response_headers, body)); envoy::admin::v2alpha::ServerInfo server_info_proto; EXPECT_THAT(std::string(response_headers.ContentType()->value().getStringView()), diff --git a/test/server/init_manager_impl_test.cc b/test/server/init_manager_impl_test.cc deleted file mode 100644 index 964db18551670..0000000000000 --- a/test/server/init_manager_impl_test.cc +++ /dev/null @@ -1,69 +0,0 @@ -#include "server/init_manager_impl.h" - -#include "test/mocks/common.h" -#include "test/mocks/init/mocks.h" - -#include "gmock/gmock.h" - -using testing::_; -using testing::InSequence; -using testing::Invoke; - -namespace Envoy { -namespace Server { -namespace { - -class InitManagerImplTest : public testing::Test { -public: - InitManagerImpl manager_{"test"}; - ReadyWatcher initialized_; -}; - -TEST_F(InitManagerImplTest, NoTargets) { - EXPECT_CALL(initialized_, ready()); - manager_.initialize([&]() -> void { initialized_.ready(); }); -} - -TEST_F(InitManagerImplTest, Targets) { - InSequence s; - Init::MockTarget target; - - manager_.registerTarget(target, ""); - EXPECT_CALL(target, initialize(_)); - manager_.initialize([&]() -> void { initialized_.ready(); }); - EXPECT_CALL(initialized_, ready()); - target.callback_(); -} - -TEST_F(InitManagerImplTest, TargetRemoveWhileInitializing) { - InSequence s; - Init::MockTarget target; - - manager_.registerTarget(target, ""); - EXPECT_CALL(target, initialize(_)).WillOnce(Invoke([](std::function callback) -> void { - callback(); - })); - EXPECT_CALL(initialized_, ready()); - manager_.initialize([&]() -> void { initialized_.ready(); }); -} - -TEST_F(InitManagerImplTest, TargetAfterInitializing) { - InSequence s; - Init::MockTarget target1; - Init::MockTarget target2; - - manager_.registerTarget(target1, ""); - EXPECT_CALL(target1, initialize(_)); - manager_.initialize([&]() -> void { initialized_.ready(); }); - - EXPECT_CALL(target2, initialize(_)); - manager_.registerTarget(target2, ""); - - target2.callback_(); - EXPECT_CALL(initialized_, ready()); - target1.callback_(); -} - -} // namespace -} // namespace Server -} // namespace Envoy diff --git a/test/server/lds_api_test.cc b/test/server/lds_api_test.cc index 7c73374a1d986..3ba5bd805525e 100644 --- a/test/server/lds_api_test.cc +++ b/test/server/lds_api_test.cc @@ -25,7 +25,12 @@ namespace { class LdsApiTest : public testing::Test { public: - LdsApiTest() : request_(&cluster_manager_.async_client_), api_(Api::createApiForTest(store_)) {} + LdsApiTest() : request_(&cluster_manager_.async_client_), api_(Api::createApiForTest(store_)) { + ON_CALL(init_, add(_)) + .WillByDefault(Invoke([this](const Init::TargetReceiver& target_receiver) { + init_target_caller_ = target_receiver.caller("test"); + })); + } void setup() { const std::string config_json = R"EOF( @@ -50,12 +55,12 @@ class LdsApiTest : public testing::Test { EXPECT_CALL(cluster, info()); EXPECT_CALL(*cluster.info_, type()); interval_timer_ = new Event::MockTimer(&dispatcher_); - EXPECT_CALL(init_, registerTarget(_, _)); + EXPECT_CALL(init_, add(_)); lds_ = std::make_unique(lds_config, cluster_manager_, dispatcher_, random_, init_, local_info_, store_, listener_manager_, *api_); expectRequest(); - init_.initialize(); + init_target_caller_(init_receiver_); } void expectAdd(const std::string& listener_name, absl::optional version, @@ -121,7 +126,6 @@ class LdsApiTest : public testing::Test { NiceMock cluster_manager_; Event::MockDispatcher dispatcher_; NiceMock random_; - Init::MockManager init_; NiceMock local_info_; Stats::IsolatedStoreImpl store_; MockListenerManager listener_manager_; @@ -131,6 +135,11 @@ class LdsApiTest : public testing::Test { Http::AsyncClient::Callbacks* callbacks_{}; Api::ApiPtr api_; + Init::MockManager init_; + ReadyWatcher initialized_; + Init::TargetCaller init_target_caller_; + Init::Receiver init_receiver_{"test", [this]() { initialized_.ready(); }}; + private: std::list> listeners_; }; @@ -191,7 +200,7 @@ TEST_F(LdsApiTest, MisconfiguredListenerNameIsPresentInException) { EXPECT_CALL(listener_manager_, addOrUpdateListener(_, _, true)) .WillOnce(Throw(EnvoyException("something is wrong"))); - EXPECT_CALL(init_.initialized_, ready()); + EXPECT_CALL(initialized_, ready()); EXPECT_THROW_WITH_MESSAGE( lds_->onConfigUpdate(listeners, ""), EnvoyException, @@ -209,7 +218,7 @@ TEST_F(LdsApiTest, EmptyListenersUpdate) { EXPECT_CALL(listener_manager_, listeners()).WillOnce(Return(existing_listeners)); - EXPECT_CALL(init_.initialized_, ready()); + EXPECT_CALL(initialized_, ready()); EXPECT_CALL(request_, cancel()); lds_->onConfigUpdate(listeners, ""); @@ -237,7 +246,7 @@ TEST_F(LdsApiTest, ListenerCreationContinuesEvenAfterException) { .WillOnce(Return(true)) .WillOnce(Throw(EnvoyException("something else is wrong"))); - EXPECT_CALL(init_.initialized_, ready()); + EXPECT_CALL(initialized_, ready()); EXPECT_THROW_WITH_MESSAGE(lds_->onConfigUpdate(listeners, ""), EnvoyException, "Error adding/updating listener(s) invalid-listener-1: something is " @@ -324,7 +333,7 @@ TEST_F(LdsApiTest, Basic) { makeListenersAndExpectCall({}); expectAdd("listener1", "0", true); expectAdd("listener2", "0", true); - EXPECT_CALL(init_.initialized_, ready()); + EXPECT_CALL(initialized_, ready()); EXPECT_CALL(*interval_timer_, enableTimer(_)); callbacks_->onSuccess(std::move(message)); @@ -397,7 +406,7 @@ TEST_F(LdsApiTest, TlsConfigWithoutCaCert) { makeListenersAndExpectCall({"listener0"}); expectAdd("listener0", {}, true); - EXPECT_CALL(init_.initialized_, ready()); + EXPECT_CALL(initialized_, ready()); EXPECT_CALL(*interval_timer_, enableTimer(_)); callbacks_->onSuccess(std::move(message)); @@ -474,7 +483,7 @@ TEST_F(LdsApiTest, Failure) { Http::HeaderMapPtr{new Http::TestHeaderMapImpl{{":status", "200"}}})); message->body() = std::make_unique(response_json); - EXPECT_CALL(init_.initialized_, ready()); + EXPECT_CALL(initialized_, ready()); EXPECT_CALL(*interval_timer_, enableTimer(_)); callbacks_->onSuccess(std::move(message)); @@ -524,7 +533,7 @@ TEST_F(LdsApiTest, ReplacingListenerWithSameAddress) { makeListenersAndExpectCall({}); expectAdd("listener1", "0", true); expectAdd("listener2", "0", true); - EXPECT_CALL(init_.initialized_, ready()); + EXPECT_CALL(initialized_, ready()); EXPECT_CALL(*interval_timer_, enableTimer(_)); callbacks_->onSuccess(std::move(message)); diff --git a/test/server/server_test.cc b/test/server/server_test.cc index d9b8c54284d00..041e1842b7658 100644 --- a/test/server/server_test.cc +++ b/test/server/server_test.cc @@ -79,7 +79,7 @@ class RunHelperTest : public testing::Test { NiceMock cm_; NiceMock access_log_manager_; NiceMock overload_manager_; - InitManagerImpl init_manager_{""}; + Init::ManagerImpl init_manager_{""}; ReadyWatcher start_workers_; std::unique_ptr helper_; std::function cm_init_callback_; diff --git a/tools/spelling_dictionary.txt b/tools/spelling_dictionary.txt index 34a034702ad1a..507420a4d731e 100644 --- a/tools/spelling_dictionary.txt +++ b/tools/spelling_dictionary.txt @@ -154,6 +154,7 @@ NUL Nilsson OCSP OK +OO OOM OOMs OS @@ -365,6 +366,7 @@ deletable deleter delim dereference +dereferenced dereferences dereferencing deregistered