From e7abe65b1c2481daec90e5a7dfc66dfe37c4c4c1 Mon Sep 17 00:00:00 2001 From: Cerek Hillen Date: Tue, 9 Feb 2021 06:08:17 -0800 Subject: [PATCH] python: normalize callback structs (#1240) Description: Because there was no separate callbacks struct, `c_on_engine_running` and `c_on_exit` were namespaced under `Engine`. This PR moves the `std::function` closures to a dedicated `EngineCallbacks` and moves `c_on_engine_running` and `c_on_exit` into an anonymous namespace. This PR also normalizes the callback structure for both `EngineCallbacks` and `StreamCallbacks`, so they're constructed & passed around the same. Risk Level: Low, none of this code is built into a production target. Testing: Pending Signed-off-by: Cerek Hillen Signed-off-by: JP Simard --- mobile/library/cc/engine.cc | 40 +++++++------ mobile/library/cc/engine.h | 15 +++-- mobile/library/cc/engine_builder.cc | 6 +- mobile/library/cc/engine_builder.h | 2 +- mobile/library/cc/stream.cc | 4 +- mobile/library/cc/stream.h | 4 +- mobile/library/cc/stream_callbacks.cc | 81 +++++++++++++-------------- mobile/library/cc/stream_callbacks.h | 19 +------ mobile/library/cc/stream_prototype.cc | 5 +- 9 files changed, 83 insertions(+), 93 deletions(-) diff --git a/mobile/library/cc/engine.cc b/mobile/library/cc/engine.cc index 14ec03bd0ea8..58d96188da9e 100644 --- a/mobile/library/cc/engine.cc +++ b/mobile/library/cc/engine.cc @@ -6,16 +6,31 @@ namespace Envoy { namespace Platform { +namespace { + +void c_on_engine_running(void* context) { + EngineCallbacks* engine_callbacks = static_cast(context); + engine_callbacks->on_engine_running(); +} + +void c_on_exit(void* context) { + // NOTE: this function is intentionally empty + // as we don't actually do any post-processing on exit. + (void)context; +} + +} // namespace + Engine::Engine(envoy_engine_t engine, const std::string& configuration, LogLevel log_level, - std::function on_engine_running) - : engine_(engine), on_engine_running_(on_engine_running) { - envoy_engine_callbacks callbacks{ - .on_engine_running = &Engine::c_on_engine_running, - .on_exit = &Engine::c_on_exit, - .context = this, + EngineCallbacksSharedPtr callbacks) + : engine_(engine), callbacks_(callbacks) { + envoy_engine_callbacks envoy_callbacks{ + .on_engine_running = &c_on_engine_running, + .on_exit = &c_on_exit, + .context = &this->callbacks_, }; - run_engine(this->engine_, callbacks, configuration.c_str(), + run_engine(this->engine_, envoy_callbacks, configuration.c_str(), log_level_to_string(log_level).c_str()); this->stream_client_ = std::make_shared(this->engine_); @@ -27,16 +42,5 @@ Engine::~Engine() { terminate_engine(this->engine_); } StreamClientSharedPtr Engine::stream_client() { return this->stream_client_; } PulseClientSharedPtr Engine::pulse_client() { return this->pulse_client_; } -void Engine::c_on_engine_running(void* context) { - Engine* engine = static_cast(context); - engine->on_engine_running_(); -} - -void Engine::c_on_exit(void* context) { - // NOTE: this function is intentionally empty - // as we don't actually do any post-processing on exit. - (void)context; -} - } // namespace Platform } // namespace Envoy diff --git a/mobile/library/cc/engine.h b/mobile/library/cc/engine.h index 1f31870bddd7..9c9ff03de382 100644 --- a/mobile/library/cc/engine.h +++ b/mobile/library/cc/engine.h @@ -15,6 +15,14 @@ namespace Platform { // - change context from Engine ptr to EngineCallbacks ptr // - move c_on_(...) from private static fn to static fn in anonymous namespace +struct EngineCallbacks { + std::function on_engine_running; + // unused: + // std::function on_exit; +}; + +using EngineCallbacksSharedPtr = std::shared_ptr; + class Engine { public: ~Engine(); @@ -24,15 +32,12 @@ class Engine { private: Engine(envoy_engine_t engine, const std::string& configuration, LogLevel log_level, - std::function on_engine_running); - - static void c_on_engine_running(void* context); - static void c_on_exit(void* context); + EngineCallbacksSharedPtr callbacks); friend class EngineBuilder; envoy_engine_t engine_; - std::function on_engine_running_; + EngineCallbacksSharedPtr callbacks_; StreamClientSharedPtr stream_client_; PulseClientSharedPtr pulse_client_; }; diff --git a/mobile/library/cc/engine_builder.cc b/mobile/library/cc/engine_builder.cc index e8810f63935c..3084ad057b11 100644 --- a/mobile/library/cc/engine_builder.cc +++ b/mobile/library/cc/engine_builder.cc @@ -9,11 +9,12 @@ EngineBuilder::EngineBuilder() {} EngineBuilder& EngineBuilder::add_log_level(LogLevel log_level) { this->log_level_ = log_level; + this->callbacks_ = std::make_shared(); return *this; } EngineBuilder& EngineBuilder::set_on_engine_running(std::function closure) { - this->on_engine_running_ = closure; + this->callbacks_->on_engine_running = closure; return *this; } @@ -87,8 +88,7 @@ EngineSharedPtr EngineBuilder::build() { } } - Engine* engine = - new Engine(init_engine(), config_str, this->log_level_, this->on_engine_running_); + Engine* engine = new Engine(init_engine(), config_str, this->log_level_, this->callbacks_); return EngineSharedPtr(engine); } diff --git a/mobile/library/cc/engine_builder.h b/mobile/library/cc/engine_builder.h index a618644789a8..aa4f357b9e38 100644 --- a/mobile/library/cc/engine_builder.h +++ b/mobile/library/cc/engine_builder.h @@ -35,7 +35,7 @@ class EngineBuilder { private: LogLevel log_level_ = LogLevel::info; - std::function on_engine_running_; + EngineCallbacksSharedPtr callbacks_; std::string stats_domain_ = "0.0.0.0"; int connect_timeout_seconds_ = 30; diff --git a/mobile/library/cc/stream.cc b/mobile/library/cc/stream.cc index 8e3c43460df3..2ab31a101857 100644 --- a/mobile/library/cc/stream.cc +++ b/mobile/library/cc/stream.cc @@ -9,8 +9,8 @@ namespace Envoy { namespace Platform { -Stream::Stream(envoy_stream_t handle, EnvoyHttpCallbacksAdapterSharedPtr adapter) - : handle_(handle), adapter_(adapter) {} +Stream::Stream(envoy_stream_t handle, StreamCallbacksSharedPtr callbacks) + : handle_(handle), callbacks_(callbacks) {} Stream& Stream::send_headers(RequestHeadersSharedPtr headers, bool end_stream) { envoy_headers raw_headers = raw_header_map_as_envoy_headers(headers->all_headers()); diff --git a/mobile/library/cc/stream.h b/mobile/library/cc/stream.h index 5de4e1b4079b..8d873ee14765 100644 --- a/mobile/library/cc/stream.h +++ b/mobile/library/cc/stream.h @@ -12,7 +12,7 @@ namespace Platform { class Stream { public: - Stream(envoy_stream_t handle, EnvoyHttpCallbacksAdapterSharedPtr adapter); + Stream(envoy_stream_t handle, StreamCallbacksSharedPtr callbacks); Stream& send_headers(RequestHeadersSharedPtr headers, bool end_stream); Stream& send_data(envoy_data data); @@ -22,7 +22,7 @@ class Stream { private: envoy_stream_t handle_; - EnvoyHttpCallbacksAdapterSharedPtr adapter_; + StreamCallbacksSharedPtr callbacks_; }; using StreamSharedPtr = std::shared_ptr; diff --git a/mobile/library/cc/stream_callbacks.cc b/mobile/library/cc/stream_callbacks.cc index d822a331b11d..c07bbe55ba05 100644 --- a/mobile/library/cc/stream_callbacks.cc +++ b/mobile/library/cc/stream_callbacks.cc @@ -7,27 +7,11 @@ namespace Envoy { namespace Platform { -EnvoyHttpCallbacksAdapter::EnvoyHttpCallbacksAdapter(StreamCallbacksSharedPtr callbacks) - : stream_callbacks_(callbacks) {} +namespace { -envoy_http_callbacks EnvoyHttpCallbacksAdapter::as_envoy_http_callbacks() { - envoy_http_callbacks callbacks{ - .on_headers = &EnvoyHttpCallbacksAdapter::c_on_headers, - .on_data = &EnvoyHttpCallbacksAdapter::c_on_data, - // on_metadata is not used - .on_trailers = &EnvoyHttpCallbacksAdapter::c_on_trailers, - .on_error = &EnvoyHttpCallbacksAdapter::c_on_error, - .on_complete = &EnvoyHttpCallbacksAdapter::c_on_complete, - .on_cancel = &EnvoyHttpCallbacksAdapter::c_on_cancel, - .context = this, - }; - return callbacks; -} - -void* EnvoyHttpCallbacksAdapter::c_on_headers(envoy_headers headers, bool end_stream, - void* context) { - auto self = static_cast(context); - if (self->stream_callbacks_->on_headers.has_value()) { +void* c_on_headers(envoy_headers headers, bool end_stream, void* context) { + auto stream_callbacks = static_cast(context); + if (stream_callbacks->on_headers.has_value()) { auto raw_headers = envoy_headers_as_raw_header_map(headers); ResponseHeadersBuilder builder; for (const auto& pair : raw_headers) { @@ -36,61 +20,76 @@ void* EnvoyHttpCallbacksAdapter::c_on_headers(envoy_headers headers, bool end_st } builder.set(pair.first, pair.second); } - self->stream_callbacks_->on_headers.value()(builder.build(), end_stream); + stream_callbacks->on_headers.value()(builder.build(), end_stream); } return context; } -void* EnvoyHttpCallbacksAdapter::c_on_data(envoy_data data, bool end_stream, void* context) { - auto self = static_cast(context); - if (self->stream_callbacks_->on_error.has_value()) { - self->stream_callbacks_->on_data.value()(data, end_stream); +void* c_on_data(envoy_data data, bool end_stream, void* context) { + auto stream_callbacks = static_cast(context); + if (stream_callbacks->on_error.has_value()) { + stream_callbacks->on_data.value()(data, end_stream); } return context; } -void* EnvoyHttpCallbacksAdapter::c_on_trailers(envoy_headers metadata, void* context) { - auto self = static_cast(context); - if (self->stream_callbacks_->on_trailers.has_value()) { +void* c_on_trailers(envoy_headers metadata, void* context) { + auto stream_callbacks = static_cast(context); + if (stream_callbacks->on_trailers.has_value()) { auto raw_headers = envoy_headers_as_raw_header_map(metadata); ResponseTrailersBuilder builder; for (const auto& pair : raw_headers) { builder.set(pair.first, pair.second); } - self->stream_callbacks_->on_trailers.value()(builder.build()); + stream_callbacks->on_trailers.value()(builder.build()); } return context; } -void* EnvoyHttpCallbacksAdapter::c_on_error(envoy_error raw_error, void* context) { - auto self = static_cast(context); - if (self->stream_callbacks_->on_error.has_value()) { +void* c_on_error(envoy_error raw_error, void* context) { + auto stream_callbacks = static_cast(context); + if (stream_callbacks->on_error.has_value()) { EnvoyErrorSharedPtr error = std::make_shared(); error->error_code = raw_error.error_code; // TODO(crockeo): go back and convert from raw_error.message // when doing so won't cause merge conflicts with other PRs. error->message = ""; error->attempt_count = absl::optional(raw_error.attempt_count); - self->stream_callbacks_->on_error.value()(error); + stream_callbacks->on_error.value()(error); } return context; } -void* EnvoyHttpCallbacksAdapter::c_on_complete(void* context) { - auto self = static_cast(context); - if (self->stream_callbacks_->on_complete.has_value()) { - self->stream_callbacks_->on_complete.value(); +void* c_on_complete(void* context) { + auto stream_callbacks = static_cast(context); + if (stream_callbacks->on_complete.has_value()) { + stream_callbacks->on_complete.value(); } return context; } -void* EnvoyHttpCallbacksAdapter::c_on_cancel(void* context) { - auto self = static_cast(context); - if (self->stream_callbacks_->on_cancel.has_value()) { - self->stream_callbacks_->on_cancel.value(); +void* c_on_cancel(void* context) { + auto stream_callbacks = static_cast(context); + if (stream_callbacks->on_cancel.has_value()) { + stream_callbacks->on_cancel.value(); } return context; } +} // namespace + +envoy_http_callbacks StreamCallbacks::as_envoy_http_callbacks() { + return envoy_http_callbacks{ + .on_headers = &c_on_headers, + .on_data = &c_on_data, + // on_metadata is not used + .on_trailers = &c_on_trailers, + .on_error = &c_on_error, + .on_complete = &c_on_complete, + .on_cancel = &c_on_cancel, + .context = this, + }; +} + } // namespace Platform } // namespace Envoy diff --git a/mobile/library/cc/stream_callbacks.h b/mobile/library/cc/stream_callbacks.h index 0d2c50e90d8b..d4ed06288e29 100644 --- a/mobile/library/cc/stream_callbacks.h +++ b/mobile/library/cc/stream_callbacks.h @@ -27,28 +27,11 @@ struct StreamCallbacks { absl::optional on_error; absl::optional on_complete; absl::optional on_cancel; -}; - -using StreamCallbacksSharedPtr = std::shared_ptr; - -class EnvoyHttpCallbacksAdapter { -public: - EnvoyHttpCallbacksAdapter(StreamCallbacksSharedPtr callbacks); envoy_http_callbacks as_envoy_http_callbacks(); - -private: - static void* c_on_headers(envoy_headers headers, bool end_stream, void* context); - static void* c_on_data(envoy_data data, bool end_stream, void* context); - static void* c_on_trailers(envoy_headers metadata, void* context); - static void* c_on_error(envoy_error raw_error, void* context); - static void* c_on_complete(void* context); - static void* c_on_cancel(void* context); - - StreamCallbacksSharedPtr stream_callbacks_; }; -using EnvoyHttpCallbacksAdapterSharedPtr = std::shared_ptr; +using StreamCallbacksSharedPtr = std::shared_ptr; } // namespace Platform } // namespace Envoy diff --git a/mobile/library/cc/stream_prototype.cc b/mobile/library/cc/stream_prototype.cc index 804747884e15..ba6c2d4add5a 100644 --- a/mobile/library/cc/stream_prototype.cc +++ b/mobile/library/cc/stream_prototype.cc @@ -11,10 +11,9 @@ StreamPrototype::StreamPrototype(envoy_engine_t engine) : engine_(engine) { StreamSharedPtr StreamPrototype::start() { auto stream = init_stream(this->engine_); - auto adapter = std::make_shared(this->callbacks_); - start_stream(stream, adapter->as_envoy_http_callbacks()); + start_stream(stream, this->callbacks_->as_envoy_http_callbacks()); - return std::make_shared(stream, adapter); + return std::make_shared(stream, this->callbacks_); } StreamPrototype& StreamPrototype::set_on_headers(OnHeadersCallback closure) {