From f3c86124083f9464a5c1271597bd59c0e86a3d70 Mon Sep 17 00:00:00 2001 From: Mike Schore Date: Fri, 11 Oct 2019 09:17:12 -0700 Subject: [PATCH 01/11] compiles Signed-off-by: Mike Schore --- library/common/BUILD | 2 + library/common/engine.cc | 94 ++++++++++++++++++++++++++++++++ library/common/engine.h | 42 ++++++++++++++ library/common/main_interface.cc | 93 ++++--------------------------- 4 files changed, 148 insertions(+), 83 deletions(-) create mode 100644 library/common/engine.cc create mode 100644 library/common/engine.h diff --git a/library/common/BUILD b/library/common/BUILD index 6c2fa16ce4..37239b9f89 100644 --- a/library/common/BUILD +++ b/library/common/BUILD @@ -9,6 +9,8 @@ envoy_cc_library( srcs = [ "certificates.inc", "config_template.cc", + "engine.h", + "engine.cc", "main_interface.cc", ], hdrs = ["main_interface.h"], diff --git a/library/common/engine.cc b/library/common/engine.cc new file mode 100644 index 0000000000..b507d725f3 --- /dev/null +++ b/library/common/engine.cc @@ -0,0 +1,94 @@ +#include "library/common/engine.h" + +#include "common/common/lock_guard.h" + +namespace Envoy { + +// As a server, Envoy's static factory registration happens when main is run. However, when compiled as a library, there is no guarantee that such registration will happen before the names are needed. The following calls ensure that registration happens before the entities are needed. Note that as more registrations are needed, explicit initialization calls will need to be added here. +static void registerFactories() { + Envoy::Extensions::Clusters::DynamicForwardProxy::forceRegisterClusterFactory(); + Envoy::Extensions::HttpFilters::DynamicForwardProxy:: + forceRegisterDynamicForwardProxyFilterFactory(); + Envoy::Extensions::HttpFilters::RouterFilter::forceRegisterRouterFilterConfig(); + Envoy::Extensions::NetworkFilters::HttpConnectionManager:: + forceRegisterHttpConnectionManagerFilterConfigFactory(); + Envoy::Extensions::TransportSockets::RawBuffer::forceRegisterDownstreamRawBufferSocketFactory(); + Envoy::Extensions::TransportSockets::RawBuffer::forceRegisterUpstreamRawBufferSocketFactory(); + Envoy::Extensions::TransportSockets::Tls::forceRegisterUpstreamSslSocketFactory(); + Envoy::Upstream::forceRegisterLogicalDnsClusterFactory(); +} + +Engine::Engine(const char* config, const char* log_level, std::atomic& preferred_network) { + // Ensure static factory registration occurs on time. + absl::call_once(register_once_, registerFactories); + + // Create the Http::Dispatcher first since it contains initial queueing logic. + // TODO: consider centralizing initial queueing in this class. + http_dispatcher_ = std::make_unique(preferred_network); + + const char* envoy_argv[] = {strdup("envoy"), strdup("--config-yaml"), strdup(config), + strdup("-l"), strdup(log_level), nullptr}; + + // Start the Envoy on a dedicated thread. + main_thread_ = std::thread(&Engine::run, this, envoy_argv); +} + +envoy_status_t Engine::run(const char** envoy_argv) { + { + Thread::LockGuard lock(mutex_); + try { + main_common_ = std::make_unique(5, envoy_argv); + cv_.notifyOne(); + } catch (const Envoy::NoServingException& e) { + return ENVOY_SUCCESS; + } catch (const Envoy::MalformedArgvException& e) { + std::cerr << e.what() << std::endl; + return ENVOY_FAILURE; + } catch (const Envoy::EnvoyException& e) { + std::cerr << e.what() << std::endl; + return ENVOY_FAILURE; + } + + // Note: We're waiting longer than we might otherwise to drain to the main thread's dispatcher. + // This is because we're not simply waiting for its availability and for it to have started, but + // also because we're waiting for clusters to have done their first attempt at DNS resolution. + // When we improve synchronous failure handling and/or move to dynamic forwarding, we only need + // to wait until the dispatcher is running (and can drain by enqueueing a drain callback on it, + // as we did previously). + auto server = main_common_->server(); + stageone_callback_handler_ = main_common_->server()->lifecycleNotifier().registerCallback( + Envoy::Server::ServerLifecycleNotifier::Stage::PostInit, [this, server]() -> void { + http_dispatcher_->ready(server->dispatcher(), server->clusterManager()); + } + ); + } // mutex_ + + // The main run loop must run without holding the mutex, so that the destructor can acquire it. + return TS_UNCHECKED_READ(main_common_)->run() ? ENVOY_SUCCESS : ENVOY_FAILURE; +} + +Engine::~Engine() { + // If we're already on the main thread, it should be safe to simply destruct. + if (!main_thread_.joinable()) { + return; + } + + // If we're not on the main thread, we need to be sure that MainCommon is finished being constructed so we can dispatch shutdown. + { + Thread::LockGuard lock(mutex_); + if (!main_common_) { + cv_.wait(mutex_); + } + ASSERT(main_common_); + main_common_->server()->shutdown(); + } // _mutex + + // Now we wait for the main thread to wrap things up. + main_thread_.join(); +} + +Http::Dispatcher& Engine::httpDispatcher() { + return *http_dispatcher_; +} + +} // namespace Envoy diff --git a/library/common/engine.h b/library/common/engine.h new file mode 100644 index 0000000000..fca489682b --- /dev/null +++ b/library/common/engine.h @@ -0,0 +1,42 @@ +#pragma once + +#include "absl/base/call_once.h" + +#include "library/common/http/dispatcher.h" +#include "library/common/types/c_types.h" + +#include "common/upstream/logical_dns_cluster.h" + +#include "envoy/server/lifecycle_notifier.h" + +#include "exe/main_common.h" + +#include "extensions/clusters/dynamic_forward_proxy/cluster.h" +#include "extensions/filters/http/dynamic_forward_proxy/config.h" +#include "extensions/filters/http/router/config.h" +#include "extensions/filters/network/http_connection_manager/config.h" +#include "extensions/transport_sockets/raw_buffer/config.h" +#include "extensions/transport_sockets/tls/config.h" + +namespace Envoy { + +class Engine { +public: + Engine(const char* config, const char* log_level, std::atomic& preferred_network); + + ~Engine(); + + envoy_status_t run(const char** argv); + + Http::Dispatcher& httpDispatcher(); +private: + static absl::once_flag register_once_; + Thread::MutexBasicLockable mutex_; + Thread::CondVar cv_; + std::thread main_thread_; + std::unique_ptr http_dispatcher_; + std::unique_ptr main_common_ GUARDED_BY(mutex_); + Envoy::Server::ServerLifecycleNotifier::HandlePtr stageone_callback_handler_; +}; + +} // namespace Envoy diff --git a/library/common/main_interface.cc b/library/common/main_interface.cc index 24630f07aa..6bd8f0a78d 100644 --- a/library/common/main_interface.cc +++ b/library/common/main_interface.cc @@ -1,59 +1,41 @@ #include "library/common/main_interface.h" #include -#include -#include "envoy/server/lifecycle_notifier.h" - -#include "common/upstream/logical_dns_cluster.h" - -#include "exe/main_common.h" - -#include "extensions/clusters/dynamic_forward_proxy/cluster.h" -#include "extensions/filters/http/dynamic_forward_proxy/config.h" -#include "extensions/filters/http/router/config.h" -#include "extensions/filters/network/http_connection_manager/config.h" -#include "extensions/transport_sockets/raw_buffer/config.h" -#include "extensions/transport_sockets/tls/config.h" - -#include "library/common/buffer/utility.h" +#include "library/common/engine.h" #include "library/common/http/dispatcher.h" -#include "library/common/http/header_utility.h" // NOLINT(namespace-envoy) -static std::unique_ptr main_common_; -static std::unique_ptr http_dispatcher_; -static Envoy::Server::ServerLifecycleNotifier::HandlePtr stageone_callback_handler_; +static std::unique_ptr engine_; static std::atomic current_stream_handle_{0}; static std::atomic preferred_network_{ENVOY_NET_GENERIC}; envoy_stream_t init_stream(envoy_engine_t) { return current_stream_handle_++; } envoy_status_t start_stream(envoy_stream_t stream, envoy_http_callbacks callbacks) { - http_dispatcher_->startStream(stream, callbacks); + engine_->httpDispatcher().startStream(stream, callbacks); return ENVOY_SUCCESS; } envoy_status_t send_headers(envoy_stream_t stream, envoy_headers headers, bool end_stream) { - return http_dispatcher_->sendHeaders(stream, headers, end_stream); + return engine_->httpDispatcher().sendHeaders(stream, headers, end_stream); } envoy_status_t send_data(envoy_stream_t stream, envoy_data data, bool end_stream) { - return http_dispatcher_->sendData(stream, data, end_stream); + return engine_->httpDispatcher().sendData(stream, data, end_stream); } // TODO: implement. envoy_status_t send_metadata(envoy_stream_t, envoy_headers) { return ENVOY_FAILURE; } envoy_status_t send_trailers(envoy_stream_t stream, envoy_headers trailers) { - return http_dispatcher_->sendTrailers(stream, trailers); + return engine_->httpDispatcher().sendTrailers(stream, trailers); } -envoy_status_t reset_stream(envoy_stream_t stream) { return http_dispatcher_->resetStream(stream); } +envoy_status_t reset_stream(envoy_stream_t stream) { return engine_->httpDispatcher().resetStream(stream); } envoy_engine_t init_engine() { - http_dispatcher_ = std::make_unique(preferred_network_); // TODO(goaway): return new handle once multiple engine support is in place. // https://github.com/lyft/envoy-mobile/issues/332 return 1; @@ -67,62 +49,7 @@ envoy_status_t set_preferred_network(envoy_network_t network) { /** * External entrypoint for library. */ -envoy_status_t run_engine(const char* config, const char* log_level) { - char* envoy_argv[] = {strdup("envoy"), strdup("--config-yaml"), strdup(config), - strdup("-l"), strdup(log_level), nullptr}; - - // Ensure static factory registration occurs on time. - // Envoy's static factory registration happens when main is run. - // However, when compiled as a library, there is no guarantee that such registration will happen - // before the names are needed. - // The following calls ensure that registration happens before the entities are needed. - // Note that as more registrations are needed, explicit initialization calls will need to be added - // here. - Envoy::Extensions::Clusters::DynamicForwardProxy::forceRegisterClusterFactory(); - Envoy::Extensions::HttpFilters::DynamicForwardProxy:: - forceRegisterDynamicForwardProxyFilterFactory(); - Envoy::Extensions::HttpFilters::RouterFilter::forceRegisterRouterFilterConfig(); - Envoy::Extensions::NetworkFilters::HttpConnectionManager:: - forceRegisterHttpConnectionManagerFilterConfigFactory(); - Envoy::Extensions::TransportSockets::RawBuffer::forceRegisterDownstreamRawBufferSocketFactory(); - Envoy::Extensions::TransportSockets::RawBuffer::forceRegisterUpstreamRawBufferSocketFactory(); - Envoy::Extensions::TransportSockets::Tls::forceRegisterUpstreamSslSocketFactory(); - Envoy::Upstream::forceRegisterLogicalDnsClusterFactory(); - - // Initialize the server's main context under a try/catch loop and simply - // return EXIT_FAILURE as needed. Whatever code in the initialization path - // that fails is expected to log an error message so the user can diagnose. - // Note that in the Android examples logging will not be seen. - // This is a known problem, and will be addressed by: - // https://github.com/lyft/envoy-mobile/issues/34 - try { - main_common_ = std::make_unique(5, envoy_argv); - - // Note: init_engine must have been called prior to calling run_engine or the creation of the - // envoy runner thread. - - // Note: We're waiting longer than we might otherwise to drain to the main thread's dispatcher. - // This is because we're not simply waiting for its availability and for it to have started, but - // also because we're waiting for clusters to have done their first attempt at DNS resolution. - // When we improve synchronous failure handling and/or move to dynamic forwarding, we only need - // to wait until the dispatcher is running (and can drain by enqueueing a drain callback on it, - // as we did previously). - stageone_callback_handler_ = main_common_->server()->lifecycleNotifier().registerCallback( - Envoy::Server::ServerLifecycleNotifier::Stage::PostInit, []() -> void { - http_dispatcher_->ready(main_common_->server()->dispatcher(), - main_common_->server()->clusterManager()); - }); - } catch (const Envoy::NoServingException& e) { - return ENVOY_SUCCESS; - } catch (const Envoy::MalformedArgvException& e) { - std::cerr << e.what() << std::endl; - return ENVOY_FAILURE; - } catch (const Envoy::EnvoyException& e) { - std::cerr << e.what() << std::endl; - return ENVOY_FAILURE; - } - - // Run the server listener loop outside try/catch blocks, so that unexpected - // exceptions show up as a core-dumps for easier diagnostics. - return main_common_->run() ? ENVOY_SUCCESS : ENVOY_FAILURE; +envoy_status_t run_engine(envoy_engine_t, const char* config, const char* log_level) { + engine_ = std::make_unique(config, log_level, preferred_network_); + return ENVOY_SUCCESS; } From 952f2bcc611fb1198a64be9d5508f00e32fc8b1e Mon Sep 17 00:00:00 2001 From: Mike Schore Date: Fri, 11 Oct 2019 14:38:43 -0700 Subject: [PATCH 02/11] fixes Signed-off-by: Mike Schore --- library/common/engine.cc | 2 ++ library/common/main_interface.h | 2 +- library/objective-c/EnvoyEngineImpl.m | 2 +- 3 files changed, 4 insertions(+), 2 deletions(-) diff --git a/library/common/engine.cc b/library/common/engine.cc index b507d725f3..5aa9050fbd 100644 --- a/library/common/engine.cc +++ b/library/common/engine.cc @@ -18,6 +18,8 @@ static void registerFactories() { Envoy::Upstream::forceRegisterLogicalDnsClusterFactory(); } +absl::once_flag Engine::register_once_; + Engine::Engine(const char* config, const char* log_level, std::atomic& preferred_network) { // Ensure static factory registration occurs on time. absl::call_once(register_once_, registerFactories); diff --git a/library/common/main_interface.h b/library/common/main_interface.h index a53233cfab..86349b27fb 100644 --- a/library/common/main_interface.h +++ b/library/common/main_interface.h @@ -97,7 +97,7 @@ envoy_status_t set_preferred_network(envoy_network_t network); * @param log_level, the logging level to run envoy with. * @return envoy_status_t, the resulting status of the operation. */ -envoy_status_t run_engine(const char* config, const char* log_level); +envoy_status_t run_engine(envoy_engine_t engine, const char* config, const char* log_level); #ifdef __cplusplus } // functions diff --git a/library/objective-c/EnvoyEngineImpl.m b/library/objective-c/EnvoyEngineImpl.m index d47d8c3d32..1adc6fff5e 100644 --- a/library/objective-c/EnvoyEngineImpl.m +++ b/library/objective-c/EnvoyEngineImpl.m @@ -32,7 +32,7 @@ - (int)runWithConfigYAML:(NSString *)configYAML logLevel:(NSString *)logLevel { // Envoy exceptions will only be caught here when compiled for 64-bit arches. // https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/Exceptions/Articles/Exceptions64Bit.html @try { - return (int)run_engine(configYAML.UTF8String, logLevel.UTF8String); + return (int)run_engine(_engineHandle, configYAML.UTF8String, logLevel.UTF8String); } @catch (...) { NSLog(@"Envoy exception caught."); [NSNotificationCenter.defaultCenter postNotificationName:@"EnvoyException" object:self]; From 51eaa99fd07bfedf6ef5ece612626081e29db955 Mon Sep 17 00:00:00 2001 From: Mike Schore Date: Fri, 11 Oct 2019 14:47:28 -0700 Subject: [PATCH 03/11] format Signed-off-by: Mike Schore --- library/common/BUILD | 2 +- library/common/engine.cc | 26 ++++++++++++++------------ library/common/engine.h | 17 +++++++++-------- library/common/main_interface.cc | 4 +++- 4 files changed, 27 insertions(+), 22 deletions(-) diff --git a/library/common/BUILD b/library/common/BUILD index 37239b9f89..b767c06e01 100644 --- a/library/common/BUILD +++ b/library/common/BUILD @@ -9,8 +9,8 @@ envoy_cc_library( srcs = [ "certificates.inc", "config_template.cc", - "engine.h", "engine.cc", + "engine.h", "main_interface.cc", ], hdrs = ["main_interface.h"], diff --git a/library/common/engine.cc b/library/common/engine.cc index 5aa9050fbd..c735fcca0c 100644 --- a/library/common/engine.cc +++ b/library/common/engine.cc @@ -4,7 +4,10 @@ namespace Envoy { -// As a server, Envoy's static factory registration happens when main is run. However, when compiled as a library, there is no guarantee that such registration will happen before the names are needed. The following calls ensure that registration happens before the entities are needed. Note that as more registrations are needed, explicit initialization calls will need to be added here. +// As a server, Envoy's static factory registration happens when main is run. However, when compiled +// as a library, there is no guarantee that such registration will happen before the names are +// needed. The following calls ensure that registration happens before the entities are needed. Note +// that as more registrations are needed, explicit initialization calls will need to be added here. static void registerFactories() { Envoy::Extensions::Clusters::DynamicForwardProxy::forceRegisterClusterFactory(); Envoy::Extensions::HttpFilters::DynamicForwardProxy:: @@ -20,7 +23,8 @@ static void registerFactories() { absl::once_flag Engine::register_once_; -Engine::Engine(const char* config, const char* log_level, std::atomic& preferred_network) { +Engine::Engine(const char* config, const char* log_level, + std::atomic& preferred_network) { // Ensure static factory registration occurs on time. absl::call_once(register_once_, registerFactories); @@ -29,7 +33,7 @@ Engine::Engine(const char* config, const char* log_level, std::atomic(preferred_network); const char* envoy_argv[] = {strdup("envoy"), strdup("--config-yaml"), strdup(config), - strdup("-l"), strdup(log_level), nullptr}; + strdup("-l"), strdup(log_level), nullptr}; // Start the Envoy on a dedicated thread. main_thread_ = std::thread(&Engine::run, this, envoy_argv); @@ -50,7 +54,7 @@ envoy_status_t Engine::run(const char** envoy_argv) { std::cerr << e.what() << std::endl; return ENVOY_FAILURE; } - + // Note: We're waiting longer than we might otherwise to drain to the main thread's dispatcher. // This is because we're not simply waiting for its availability and for it to have started, but // also because we're waiting for clusters to have done their first attempt at DNS resolution. @@ -59,10 +63,9 @@ envoy_status_t Engine::run(const char** envoy_argv) { // as we did previously). auto server = main_common_->server(); stageone_callback_handler_ = main_common_->server()->lifecycleNotifier().registerCallback( - Envoy::Server::ServerLifecycleNotifier::Stage::PostInit, [this, server]() -> void { - http_dispatcher_->ready(server->dispatcher(), server->clusterManager()); - } - ); + Envoy::Server::ServerLifecycleNotifier::Stage::PostInit, [this, server]() -> void { + http_dispatcher_->ready(server->dispatcher(), server->clusterManager()); + }); } // mutex_ // The main run loop must run without holding the mutex, so that the destructor can acquire it. @@ -75,7 +78,8 @@ Engine::~Engine() { return; } - // If we're not on the main thread, we need to be sure that MainCommon is finished being constructed so we can dispatch shutdown. + // If we're not on the main thread, we need to be sure that MainCommon is finished being + // constructed so we can dispatch shutdown. { Thread::LockGuard lock(mutex_); if (!main_common_) { @@ -89,8 +93,6 @@ Engine::~Engine() { main_thread_.join(); } -Http::Dispatcher& Engine::httpDispatcher() { - return *http_dispatcher_; -} +Http::Dispatcher& Engine::httpDispatcher() { return *http_dispatcher_; } } // namespace Envoy diff --git a/library/common/engine.h b/library/common/engine.h index fca489682b..04c6010488 100644 --- a/library/common/engine.h +++ b/library/common/engine.h @@ -1,14 +1,9 @@ #pragma once -#include "absl/base/call_once.h" - -#include "library/common/http/dispatcher.h" -#include "library/common/types/c_types.h" +#include "envoy/server/lifecycle_notifier.h" #include "common/upstream/logical_dns_cluster.h" -#include "envoy/server/lifecycle_notifier.h" - #include "exe/main_common.h" #include "extensions/clusters/dynamic_forward_proxy/cluster.h" @@ -18,19 +13,25 @@ #include "extensions/transport_sockets/raw_buffer/config.h" #include "extensions/transport_sockets/tls/config.h" +#include "absl/base/call_once.h" +#include "library/common/http/dispatcher.h" +#include "library/common/types/c_types.h" + namespace Envoy { class Engine { public: - Engine(const char* config, const char* log_level, std::atomic& preferred_network); + Engine(const char* config, const char* log_level, + std::atomic& preferred_network); ~Engine(); envoy_status_t run(const char** argv); Http::Dispatcher& httpDispatcher(); + private: - static absl::once_flag register_once_; + static absl::once_flag register_once_; Thread::MutexBasicLockable mutex_; Thread::CondVar cv_; std::thread main_thread_; diff --git a/library/common/main_interface.cc b/library/common/main_interface.cc index 6bd8f0a78d..8858ba3c2b 100644 --- a/library/common/main_interface.cc +++ b/library/common/main_interface.cc @@ -33,7 +33,9 @@ envoy_status_t send_trailers(envoy_stream_t stream, envoy_headers trailers) { return engine_->httpDispatcher().sendTrailers(stream, trailers); } -envoy_status_t reset_stream(envoy_stream_t stream) { return engine_->httpDispatcher().resetStream(stream); } +envoy_status_t reset_stream(envoy_stream_t stream) { + return engine_->httpDispatcher().resetStream(stream); +} envoy_engine_t init_engine() { // TODO(goaway): return new handle once multiple engine support is in place. From dab5e45890da5cd5dc88a6fc9b804c52b047d592 Mon Sep 17 00:00:00 2001 From: Mike Schore Date: Mon, 14 Oct 2019 14:36:01 -0700 Subject: [PATCH 04/11] make envoy_argv non-const Signed-off-by: Mike Schore --- library/common/engine.cc | 4 ++-- library/common/engine.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/library/common/engine.cc b/library/common/engine.cc index c735fcca0c..7bde4bea2b 100644 --- a/library/common/engine.cc +++ b/library/common/engine.cc @@ -32,14 +32,14 @@ Engine::Engine(const char* config, const char* log_level, // TODO: consider centralizing initial queueing in this class. http_dispatcher_ = std::make_unique(preferred_network); - const char* envoy_argv[] = {strdup("envoy"), strdup("--config-yaml"), strdup(config), + char* envoy_argv[] = {strdup("envoy"), strdup("--config-yaml"), strdup(config), strdup("-l"), strdup(log_level), nullptr}; // Start the Envoy on a dedicated thread. main_thread_ = std::thread(&Engine::run, this, envoy_argv); } -envoy_status_t Engine::run(const char** envoy_argv) { +envoy_status_t Engine::run(char** envoy_argv) { { Thread::LockGuard lock(mutex_); try { diff --git a/library/common/engine.h b/library/common/engine.h index 04c6010488..14e4cb959e 100644 --- a/library/common/engine.h +++ b/library/common/engine.h @@ -26,7 +26,7 @@ class Engine { ~Engine(); - envoy_status_t run(const char** argv); + envoy_status_t run(char** argv); Http::Dispatcher& httpDispatcher(); From 372c4a1d59cec358a9325c0716928331eb160e6c Mon Sep 17 00:00:00 2001 From: Mike Schore Date: Mon, 14 Oct 2019 14:38:32 -0700 Subject: [PATCH 05/11] comment out call_once Signed-off-by: Mike Schore --- library/common/engine.cc | 5 +++-- library/common/engine.h | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/library/common/engine.cc b/library/common/engine.cc index 7bde4bea2b..f72201e853 100644 --- a/library/common/engine.cc +++ b/library/common/engine.cc @@ -21,12 +21,13 @@ static void registerFactories() { Envoy::Upstream::forceRegisterLogicalDnsClusterFactory(); } -absl::once_flag Engine::register_once_; +// absl::once_flag Engine::register_once_; Engine::Engine(const char* config, const char* log_level, std::atomic& preferred_network) { // Ensure static factory registration occurs on time. - absl::call_once(register_once_, registerFactories); + // absl::call_once(register_once_, registerFactories); + registerFactories(); // Create the Http::Dispatcher first since it contains initial queueing logic. // TODO: consider centralizing initial queueing in this class. diff --git a/library/common/engine.h b/library/common/engine.h index 14e4cb959e..20bb94851a 100644 --- a/library/common/engine.h +++ b/library/common/engine.h @@ -31,7 +31,7 @@ class Engine { Http::Dispatcher& httpDispatcher(); private: - static absl::once_flag register_once_; + // static absl::once_flag register_once_; Thread::MutexBasicLockable mutex_; Thread::CondVar cv_; std::thread main_thread_; From c177ab4e7ef4da7f4f351d31edee5b8230ff8589 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Tue, 15 Oct 2019 13:30:49 -0700 Subject: [PATCH 06/11] clean exit Signed-off-by: Jose Nino --- library/common/engine.cc | 22 ++++++++++++---------- library/common/engine.h | 6 +++--- 2 files changed, 15 insertions(+), 13 deletions(-) diff --git a/library/common/engine.cc b/library/common/engine.cc index f72201e853..f3045cd1bc 100644 --- a/library/common/engine.cc +++ b/library/common/engine.cc @@ -21,30 +21,29 @@ static void registerFactories() { Envoy::Upstream::forceRegisterLogicalDnsClusterFactory(); } -// absl::once_flag Engine::register_once_; - Engine::Engine(const char* config, const char* log_level, std::atomic& preferred_network) { // Ensure static factory registration occurs on time. - // absl::call_once(register_once_, registerFactories); + // TODO: ensure this is only called one time once multiple Engine objects can be allocated. registerFactories(); // Create the Http::Dispatcher first since it contains initial queueing logic. // TODO: consider centralizing initial queueing in this class. http_dispatcher_ = std::make_unique(preferred_network); - char* envoy_argv[] = {strdup("envoy"), strdup("--config-yaml"), strdup(config), - strdup("-l"), strdup(log_level), nullptr}; - // Start the Envoy on a dedicated thread. - main_thread_ = std::thread(&Engine::run, this, envoy_argv); + main_thread_ = std::thread(&Engine::run, this, std::string(config), std::string(log_level)); } -envoy_status_t Engine::run(char** envoy_argv) { +envoy_status_t Engine::run(std::string config, std::string log_level) { { Thread::LockGuard lock(mutex_); try { + char* envoy_argv[] = {strdup("envoy"), strdup("--config-yaml"), strdup(config.c_str()), + strdup("-l"), strdup(log_level.c_str()), nullptr}; + main_common_ = std::make_unique(5, envoy_argv); + event_dispatcher_ = &main_common_->server()->dispatcher(); cv_.notifyOne(); } catch (const Envoy::NoServingException& e) { return ENVOY_SUCCESS; @@ -63,7 +62,7 @@ envoy_status_t Engine::run(char** envoy_argv) { // to wait until the dispatcher is running (and can drain by enqueueing a drain callback on it, // as we did previously). auto server = main_common_->server(); - stageone_callback_handler_ = main_common_->server()->lifecycleNotifier().registerCallback( + postinit_callback_handler_ = main_common_->server()->lifecycleNotifier().registerCallback( Envoy::Server::ServerLifecycleNotifier::Stage::PostInit, [this, server]() -> void { http_dispatcher_->ready(server->dispatcher(), server->clusterManager()); }); @@ -87,7 +86,10 @@ Engine::~Engine() { cv_.wait(mutex_); } ASSERT(main_common_); - main_common_->server()->shutdown(); + // Gracefully shutdown the running envoy instance by resetting the main_common_ unique_ptr. + // Destroying MainCommon's member variables shutsdown things in the correct order and + // gracefully. + event_dispatcher_->post([this]() -> void { TS_UNCHECKED_READ(main_common_).reset(); }); } // _mutex // Now we wait for the main thread to wrap things up. diff --git a/library/common/engine.h b/library/common/engine.h index 20bb94851a..4b807db260 100644 --- a/library/common/engine.h +++ b/library/common/engine.h @@ -26,18 +26,18 @@ class Engine { ~Engine(); - envoy_status_t run(char** argv); + envoy_status_t run(std::string config, std::string log_level); Http::Dispatcher& httpDispatcher(); private: - // static absl::once_flag register_once_; Thread::MutexBasicLockable mutex_; Thread::CondVar cv_; std::thread main_thread_; std::unique_ptr http_dispatcher_; std::unique_ptr main_common_ GUARDED_BY(mutex_); - Envoy::Server::ServerLifecycleNotifier::HandlePtr stageone_callback_handler_; + Envoy::Server::ServerLifecycleNotifier::HandlePtr postinit_callback_handler_; + Event::Dispatcher* event_dispatcher_; }; } // namespace Envoy From 9a1130a1007c64b97b35671a4a18b40cd028a1a9 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Tue, 15 Oct 2019 17:13:40 -0700 Subject: [PATCH 07/11] attach thread android Signed-off-by: Jose Nino --- library/common/jni_interface.cc | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/library/common/jni_interface.cc b/library/common/jni_interface.cc index c5e07fdd7e..a691f88b57 100644 --- a/library/common/jni_interface.cc +++ b/library/common/jni_interface.cc @@ -121,6 +121,11 @@ static JNIEnv* get_env() { int get_env_res = static_jvm->GetEnv((void**)&env, JNI_VERSION); if (get_env_res == JNI_EDETACHED) { __android_log_write(ANDROID_LOG_ERROR, "jni_lib", "equals JNI_EDETACHED"); + // Note: the only thread that should need to be attached is Envoy's engine std::thread. + // TODO: harden this piece of code to make sure that we are only needing to attach Envoy + // engine's std::thread, and that we detach it successfully. + static_jvm->AttachCurrentThread(&env, NULL); + static_jvm->GetEnv((void**)&env, JNI_VERSION); } return env; } From 7ecafebaa1719651fc0bb6c928dc54cfb3901b00 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Wed, 16 Oct 2019 09:32:26 -0700 Subject: [PATCH 08/11] signatures Signed-off-by: Jose Nino --- library/common/jni_interface.cc | 8 +++----- .../io/envoyproxy/envoymobile/engine/EnvoyEngineImpl.java | 4 ++-- .../src/io/envoyproxy/envoymobile/engine/JniLibrary.java | 7 +++++-- test/performance/test_binary_size.cc | 2 +- 4 files changed, 11 insertions(+), 10 deletions(-) diff --git a/library/common/jni_interface.cc b/library/common/jni_interface.cc index a691f88b57..7f6deb48b1 100644 --- a/library/common/jni_interface.cc +++ b/library/common/jni_interface.cc @@ -30,11 +30,9 @@ extern "C" JNIEXPORT jlong JNICALL Java_io_envoyproxy_envoymobile_engine_JniLibr return init_engine(); } -extern "C" JNIEXPORT jint JNICALL -Java_io_envoyproxy_envoymobile_engine_JniLibrary_runEngine(JNIEnv* env, - jclass, // class - jstring config, jstring log_level) { - return run_engine(env->GetStringUTFChars(config, nullptr), +extern "C" JNIEXPORT jint JNICALL Java_io_envoyproxy_envoymobile_engine_JniLibrary_runEngine( + JNIEnv* env, jclass, jlong engine, jstring config, jstring log_level) { + return run_engine(engine, env->GetStringUTFChars(config, nullptr), env->GetStringUTFChars(log_level, nullptr)); } diff --git a/library/java/src/io/envoyproxy/envoymobile/engine/EnvoyEngineImpl.java b/library/java/src/io/envoyproxy/envoymobile/engine/EnvoyEngineImpl.java index 265174b270..0864da5a39 100644 --- a/library/java/src/io/envoyproxy/envoymobile/engine/EnvoyEngineImpl.java +++ b/library/java/src/io/envoyproxy/envoymobile/engine/EnvoyEngineImpl.java @@ -32,7 +32,7 @@ public EnvoyHTTPStream startStream(EnvoyHTTPCallbacks callbacks) { @Override public int runWithConfig(String configurationYAML, String logLevel) { try { - return JniLibrary.runEngine(configurationYAML, logLevel); + return JniLibrary.runEngine(this.engineHandle, configurationYAML, logLevel); } catch (Throwable throwable) { // TODO: Need to have a way to log the exception somewhere return 1; @@ -43,7 +43,7 @@ public int runWithConfig(String configurationYAML, String logLevel) { * Run the Envoy engine with the provided envoyConfiguration and log level. * * @param envoyConfiguration The EnvoyConfiguration used to start Envoy. - * @param logLevel The log level to use when starting Envoy. + * @param logLevel The log level to use when starting Envoy. * @return int A status indicating if the action was successful. */ @Override diff --git a/library/java/src/io/envoyproxy/envoymobile/engine/JniLibrary.java b/library/java/src/io/envoyproxy/envoymobile/engine/JniLibrary.java index 08916f30dd..55c32fe32b 100644 --- a/library/java/src/io/envoyproxy/envoymobile/engine/JniLibrary.java +++ b/library/java/src/io/envoyproxy/envoymobile/engine/JniLibrary.java @@ -35,7 +35,9 @@ public static void load() { // dependencies are loaded and initialized at most once. private static class JavaLoader { - private JavaLoader() { System.loadLibrary(ENVOY_JNI); } + private JavaLoader() { + System.loadLibrary(ENVOY_JNI); + } } /** @@ -131,11 +133,12 @@ private static class JavaLoader { /** * External entry point for library. * + * @param engine, the engine to run. * @param config, the configuration blob to run envoy with. * @param logLevel, the logging level to run envoy with. * @return int, the resulting status of the operation. */ - protected static native int runEngine(String config, String logLevel); + protected static native int runEngine(long engine, String config, String logLevel); // Other native methods diff --git a/test/performance/test_binary_size.cc b/test/performance/test_binary_size.cc index 863af73ac2..d49fa644dc 100644 --- a/test/performance/test_binary_size.cc +++ b/test/performance/test_binary_size.cc @@ -5,4 +5,4 @@ // This binary is used to perform stripped down binary size investigations of the Envoy codebase. // Please refer to the development docs for more information: // https://envoy-mobile.github.io/docs/envoy-mobile/latest/development/performance/binary_size.html -int main() { return run_engine(nullptr, nullptr); } +int main() { return run_engine(0, nullptr, nullptr); } From 5750ba216505ac959e46bb83def558c5a0b05f86 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Wed, 16 Oct 2019 09:36:02 -0700 Subject: [PATCH 09/11] docstring Signed-off-by: Jose Nino --- library/common/main_interface.h | 1 + 1 file changed, 1 insertion(+) diff --git a/library/common/main_interface.h b/library/common/main_interface.h index 86349b27fb..ccd0690377 100644 --- a/library/common/main_interface.h +++ b/library/common/main_interface.h @@ -93,6 +93,7 @@ envoy_status_t set_preferred_network(envoy_network_t network); /** * External entry point for library. + * @param engine, handle to the engine to run. * @param config, the configuration blob to run envoy with. * @param log_level, the logging level to run envoy with. * @return envoy_status_t, the resulting status of the operation. From 258efb402fcc17a99504f0aa549d955f3599e576 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Wed, 16 Oct 2019 09:47:47 -0700 Subject: [PATCH 10/11] fmt Signed-off-by: Jose Nino --- .../java/src/io/envoyproxy/envoymobile/engine/JniLibrary.java | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/library/java/src/io/envoyproxy/envoymobile/engine/JniLibrary.java b/library/java/src/io/envoyproxy/envoymobile/engine/JniLibrary.java index 55c32fe32b..fe9fa311e2 100644 --- a/library/java/src/io/envoyproxy/envoymobile/engine/JniLibrary.java +++ b/library/java/src/io/envoyproxy/envoymobile/engine/JniLibrary.java @@ -35,9 +35,7 @@ public static void load() { // dependencies are loaded and initialized at most once. private static class JavaLoader { - private JavaLoader() { - System.loadLibrary(ENVOY_JNI); - } + private JavaLoader() { System.loadLibrary(ENVOY_JNI); } } /** From 1cf968a3eb0dfbe6262609f8c918308f5ead3e6c Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Wed, 16 Oct 2019 14:42:05 -0700 Subject: [PATCH 11/11] comments Signed-off-by: Jose Nino --- library/common/engine.cc | 2 +- library/common/main_interface.cc | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/library/common/engine.cc b/library/common/engine.cc index f3045cd1bc..ef4af54811 100644 --- a/library/common/engine.cc +++ b/library/common/engine.cc @@ -46,7 +46,7 @@ envoy_status_t Engine::run(std::string config, std::string log_level) { event_dispatcher_ = &main_common_->server()->dispatcher(); cv_.notifyOne(); } catch (const Envoy::NoServingException& e) { - return ENVOY_SUCCESS; + return ENVOY_FAILURE; } catch (const Envoy::MalformedArgvException& e) { std::cerr << e.what() << std::endl; return ENVOY_FAILURE; diff --git a/library/common/main_interface.cc b/library/common/main_interface.cc index 8858ba3c2b..48ccec99c1 100644 --- a/library/common/main_interface.cc +++ b/library/common/main_interface.cc @@ -52,6 +52,8 @@ envoy_status_t set_preferred_network(envoy_network_t network) { * External entrypoint for library. */ envoy_status_t run_engine(envoy_engine_t, const char* config, const char* log_level) { + // This will change once multiple engine support is in place. + // https://github.com/lyft/envoy-mobile/issues/332 engine_ = std::make_unique(config, log_level, preferred_network_); return ENVOY_SUCCESS; }