diff --git a/.github/workflows/mobile-release.yml b/.github/workflows/mobile-release.yml index 007ac2b6b819..d49a0c9aa28c 100644 --- a/.github/workflows/mobile-release.yml +++ b/.github/workflows/mobile-release.yml @@ -24,8 +24,7 @@ jobs: packages: read if: >- ${{ - (github.repository == 'envoyproxy/envoy' - || vars.ENVOY_CI) + github.repository == 'envoyproxy/envoy' && (github.event.schedule || !contains(github.actor, '[bot]')) }} diff --git a/envoy/ssl/BUILD b/envoy/ssl/BUILD index 4d2a2c077a47..beb3102764a0 100644 --- a/envoy/ssl/BUILD +++ b/envoy/ssl/BUILD @@ -88,6 +88,7 @@ envoy_cc_library( "//envoy/network:connection_interface", "//envoy/network:post_io_action_interface", "//envoy/protobuf:message_validator_interface", + "//envoy/server:lifecycle_notifier_interface", "//envoy/server:options_interface", "//envoy/singleton:manager_interface", ], diff --git a/envoy/ssl/handshaker.h b/envoy/ssl/handshaker.h index 534ca4e2a1f9..3cd4972c8508 100644 --- a/envoy/ssl/handshaker.h +++ b/envoy/ssl/handshaker.h @@ -5,6 +5,7 @@ #include "envoy/network/connection.h" #include "envoy/network/post_io_action.h" #include "envoy/protobuf/message_validator.h" +#include "envoy/server/lifecycle_notifier.h" #include "envoy/server/options.h" #include "envoy/singleton/manager.h" @@ -90,6 +91,11 @@ class HandshakerFactoryContext { * The list of supported protocols exposed via ALPN, from ContextConfig. */ virtual absl::string_view alpnProtocols() const PURE; + + /** + * @return reference to the server lifecycle notifier + */ + virtual Server::ServerLifecycleNotifier& lifecycleNotifier() PURE; }; struct HandshakerCapabilities { diff --git a/mobile/.bazelrc b/mobile/.bazelrc index f768d026a42f..45fc7e138421 100644 --- a/mobile/.bazelrc +++ b/mobile/.bazelrc @@ -50,7 +50,7 @@ build:rules_xcodeproj --define=apple.experimental.tree_artifact_outputs=0 # Disable global index store to work around https://github.com/buildbuddy-io/rules_xcodeproj/issues/1878 build:rules_xcodeproj --features=-swift.use_global_index_store -# Override PGV validation with NOP functions +# Override PGV validation with NOP functions for binary size savings. build --@com_envoyproxy_protoc_gen_validate//bazel:template-flavor=nop build:mobile-dbg-common --compilation_mode=dbg @@ -81,6 +81,8 @@ build:mobile-test-android --define=static_extension_registration=disabled # enabled for //test/kotlin/integration:xds_test and # //test/java/io/envoyproxy/envoymobile/engine:envoy_configuration_test build:mobile-test-android --define=envoy_mobile_xds=enabled +# TODO(alyssar) fix +build:mobile-test-android --define=envoy_yaml=enabled # Default flags for iOS tests. build:mobile-test-ios --config=ios @@ -264,7 +266,6 @@ test:mobile-remote-ci-android --config=mobile-remote-ci build:mobile-remote-ci-cc --config=mobile-remote-ci test:mobile-remote-ci-cc --action_env=LD_LIBRARY_PATH -test:mobile-remote-ci-cc --copt=-DUSE_API_LISTENER build:mobile-remote-ci-cc-no-yaml --config=mobile-remote-ci build:mobile-remote-ci-cc-no-yaml --define=envoy_yaml=disabled @@ -279,18 +280,15 @@ build:mobile-remote-ci-cc-no-exceptions --copt=-fno-exceptions build:mobile-remote-ci-cc-test --config=mobile-remote-ci test:mobile-remote-ci-cc-test --test_output=all test:mobile-remote-ci-cc-test --config=mobile-remote-ci -test:mobile-remote-ci-cc-test --@com_envoyproxy_protoc_gen_validate//bazel:template-flavor= test:mobile-remote-ci-cc-test --define=envoy_yaml=disabled build:mobile-remote-ci-macos-kotlin --config=mobile-remote-ci-macos build:mobile-remote-ci-macos-kotlin --fat_apk_cpu=x86_64 build:mobile-remote-ci-macos-kotlin --define=envoy_yaml=disabled -build:mobile-remote-ci-macos-kotlin --@com_envoyproxy_protoc_gen_validate//bazel:template-flavor= build:mobile-remote-ci-macos-swift --config=mobile-remote-ci-macos build:mobile-remote-ci-macos-swift --config=mobile-test-ios build:mobile-remote-ci-macos-swift --@envoy//bazel:http3=False -build:mobile-remote-ci-macos-swift --@com_envoyproxy_protoc_gen_validate//bazel:template-flavor= build:mobile-remote-ci-core --config=mobile-remote-ci test:mobile-remote-ci-core --build_tests_only diff --git a/mobile/library/cc/engine_builder.cc b/mobile/library/cc/engine_builder.cc index 1509fcba9ece..e1d4526674a3 100644 --- a/mobile/library/cc/engine_builder.cc +++ b/mobile/library/cc/engine_builder.cc @@ -48,6 +48,21 @@ namespace { // https://source.chromium.org/chromium/chromium/src/+/main:net/quic/quic_context.h;drc=ccfe61524368c94b138ddf96ae8121d7eb7096cf;l=87 constexpr int32_t SocketReceiveBufferSize = 1024 * 1024; // 1MB +std::string nativeNameToConfig(absl::string_view name) { +#ifndef ENVOY_ENABLE_YAML + return absl::StrCat("[type.googleapis.com/" + "envoymobile.extensions.filters.http.platform_bridge.PlatformBridge] {" + "platform_filter_name: \"", + name, "\" }"); +#else + return absl::StrCat( + "{'@type': " + "type.googleapis.com/envoymobile.extensions.filters.http.platform_bridge.PlatformBridge, " + "platform_filter_name: ", + name, "}"); +#endif +} + } // namespace #ifdef ENVOY_MOBILE_XDS @@ -171,6 +186,11 @@ EngineBuilder& EngineBuilder::setOnEngineRunning(std::function closure) return *this; } +EngineBuilder& EngineBuilder::setEventTracker(std::unique_ptr event_tracker) { + event_tracker_ = std::move(event_tracker); + return *this; +} + EngineBuilder& EngineBuilder::addConnectTimeoutSeconds(int connect_timeout_seconds) { connect_timeout_seconds_ = connect_timeout_seconds; return *this; @@ -380,13 +400,7 @@ EngineBuilder& EngineBuilder::addNativeFilter(std::string name, std::string type } EngineBuilder& EngineBuilder::addPlatformFilter(const std::string& name) { - addNativeFilter( - "envoy.filters.http.platform_bridge", - absl::StrCat( - "{'@type': " - "type.googleapis.com/envoymobile.extensions.filters.http.platform_bridge.PlatformBridge, " - "platform_filter_name: ", - name, "}")); + addNativeFilter("envoy.filters.http.platform_bridge", nativeNameToConfig(name)); return *this; } @@ -450,13 +464,16 @@ std::unique_ptr EngineBuilder::generate for (auto filter = native_filter_chain_.rbegin(); filter != native_filter_chain_.rend(); ++filter) { -#ifdef ENVOY_ENABLE_YAML auto* native_filter = hcm->add_http_filters(); +#ifdef ENVOY_ENABLE_YAML native_filter->set_name((*filter).name_); MessageUtil::loadFromYaml((*filter).typed_config_, *native_filter->mutable_typed_config(), ProtobufMessage::getStrictValidationVisitor()); #else - IS_ENVOY_BUG("native filter chains can not be added when YAML is compiled out."); + Protobuf::TextFormat::ParseFromString((*filter).typed_config_, + native_filter->mutable_typed_config()); + RELEASE_ASSERT(!native_filter->typed_config().DebugString().empty(), + "Failed to parse" + (*filter).typed_config_); #endif } @@ -904,10 +921,8 @@ std::unique_ptr EngineBuilder::generate } EngineSharedPtr EngineBuilder::build() { - envoy_event_tracker null_tracker{}; - InternalEngine* envoy_engine = - new InternalEngine(std::move(callbacks_), std::move(logger_), null_tracker); + new InternalEngine(std::move(callbacks_), std::move(logger_), std::move(event_tracker_)); for (const auto& [name, store] : key_value_stores_) { // TODO(goaway): This leaks, but it's tied to the life of the engine. diff --git a/mobile/library/cc/engine_builder.h b/mobile/library/cc/engine_builder.h index 4f65764c81f7..0a9de6841055 100644 --- a/mobile/library/cc/engine_builder.h +++ b/mobile/library/cc/engine_builder.h @@ -131,6 +131,7 @@ class EngineBuilder { EngineBuilder& setEngineCallbacks(std::unique_ptr callbacks); [[deprecated("Use EngineBuilder::setEngineCallbacks instead")]] EngineBuilder& setOnEngineRunning(std::function closure); + EngineBuilder& setEventTracker(std::unique_ptr event_tracker); EngineBuilder& addConnectTimeoutSeconds(int connect_timeout_seconds); EngineBuilder& addDnsRefreshSeconds(int dns_refresh_seconds); EngineBuilder& addDnsFailureRefreshSeconds(int base, int max); @@ -219,6 +220,7 @@ class EngineBuilder { Logger::Logger::Levels log_level_ = Logger::Logger::Levels::info; std::unique_ptr logger_{nullptr}; std::unique_ptr callbacks_; + std::unique_ptr event_tracker_{nullptr}; int connect_timeout_seconds_ = 30; int dns_refresh_seconds_ = 60; diff --git a/mobile/library/common/BUILD b/mobile/library/common/BUILD index 9e6dd4aefeb5..2194e028a72b 100644 --- a/mobile/library/common/BUILD +++ b/mobile/library/common/BUILD @@ -82,6 +82,8 @@ envoy_cc_library( ], repository = "@envoy", deps = [ + "@com_google_absl//absl/container:flat_hash_map", + "@com_google_absl//absl/strings", "@envoy//source/common/common:base_logger_lib", ], ) diff --git a/mobile/library/common/engine_types.h b/mobile/library/common/engine_types.h index 75d0bc76588e..8ccfe8414bb1 100644 --- a/mobile/library/common/engine_types.h +++ b/mobile/library/common/engine_types.h @@ -15,6 +15,9 @@ #include "source/common/common/base_logger.h" +#include "absl/container/flat_hash_map.h" +#include "absl/strings/string_view.h" + namespace Envoy { /** The callbacks for the Envoy Engine. */ @@ -30,4 +33,13 @@ struct EnvoyLogger { std::function on_exit = [] {}; }; +inline constexpr absl::string_view ENVOY_EVENT_TRACKER_API_NAME = "event_tracker_api"; + +/** The callbacks for Envoy Event Tracker. */ +struct EnvoyEventTracker { + std::function&)> on_track = + [](const absl::flat_hash_map&) {}; + std::function on_exit = [] {}; +}; + } // namespace Envoy diff --git a/mobile/library/common/internal_engine.cc b/mobile/library/common/internal_engine.cc index 69c4bd6db7ce..eabb2ffc022b 100644 --- a/mobile/library/common/internal_engine.cc +++ b/mobile/library/common/internal_engine.cc @@ -5,8 +5,6 @@ #include "source/common/runtime/runtime_features.h" #include "absl/synchronization/notification.h" -#include "library/common/bridge/utility.h" -#include "library/common/data/utility.h" #include "library/common/stats/utility.h" namespace Envoy { @@ -17,17 +15,14 @@ envoy_stream_t InternalEngine::initStream() { return current_stream_handle_++; } InternalEngine::InternalEngine(std::unique_ptr callbacks, std::unique_ptr logger, - envoy_event_tracker event_tracker, + std::unique_ptr event_tracker, Thread::PosixThreadFactoryPtr thread_factory) : thread_factory_(std::move(thread_factory)), callbacks_(std::move(callbacks)), - logger_(std::move(logger)), event_tracker_(event_tracker), + logger_(std::move(logger)), event_tracker_(std::move(event_tracker)), dispatcher_(std::make_unique()) { ExtensionRegistry::registerFactories(); - // TODO(Augustyniak): Capturing an address of event_tracker_ and registering it in the API - // registry may lead to crashes at Engine shutdown. To be figured out as part of - // https://github.com/envoyproxy/envoy-mobile/issues/332 - Envoy::Api::External::registerApi(std::string(envoy_event_tracker_api_name), &event_tracker_); + Api::External::registerApi(std::string(ENVOY_EVENT_TRACKER_API_NAME), &event_tracker_); // Envoy Mobile always requires dfp_mixed_scheme for the TLS and cleartext DFP clusters. // While dfp_mixed_scheme defaults to true, some environments force it to false (e.g. within // Google), so we force it back to true in Envoy Mobile. @@ -37,8 +32,8 @@ InternalEngine::InternalEngine(std::unique_ptr callbacks, InternalEngine::InternalEngine(std::unique_ptr callbacks, std::unique_ptr logger, - envoy_event_tracker event_tracker) - : InternalEngine(std::move(callbacks), std::move(logger), event_tracker, + std::unique_ptr event_tracker) + : InternalEngine(std::move(callbacks), std::move(logger), std::move(event_tracker), Thread::PosixThreadFactory::create()) {} envoy_status_t InternalEngine::run(const std::string& config, const std::string& log_level) { @@ -68,18 +63,18 @@ envoy_status_t InternalEngine::main(std::shared_ptr opti { Thread::LockGuard lock(mutex_); TRY_NEEDS_AUDIT { - if (event_tracker_.track != nullptr) { + if (event_tracker_ != nullptr) { assert_handler_registration_ = Assert::addDebugAssertionFailureRecordAction([this](const char* location) { - const auto event = Bridge::Utility::makeEnvoyMap( - {{"name", "assertion"}, {"location", std::string(location)}}); - event_tracker_.track(event, event_tracker_.context); + absl::flat_hash_map event{ + {{"name", "assertion"}, {"location", std::string(location)}}}; + event_tracker_->on_track(event); }); bug_handler_registration_ = Assert::addEnvoyBugFailureRecordAction([this](const char* location) { - const auto event = Bridge::Utility::makeEnvoyMap( - {{"name", "bug"}, {"location", std::string(location)}}); - event_tracker_.track(event, event_tracker_.context); + absl::flat_hash_map event{ + {{"name", "bug"}, {"location", std::string(location)}}}; + event_tracker_->on_track(event); }); } @@ -149,6 +144,9 @@ envoy_status_t InternalEngine::main(std::shared_ptr opti bug_handler_registration_.reset(nullptr); assert_handler_registration_.reset(nullptr); + if (event_tracker_ != nullptr) { + event_tracker_->on_exit(); + } callbacks_->on_exit(); return run_success ? ENVOY_SUCCESS : ENVOY_FAILURE; diff --git a/mobile/library/common/internal_engine.h b/mobile/library/common/internal_engine.h index 67d5fa687772..06f84e797db9 100644 --- a/mobile/library/common/internal_engine.h +++ b/mobile/library/common/internal_engine.h @@ -26,7 +26,7 @@ class InternalEngine : public Logger::Loggable { * @param event_tracker, the event tracker to use for the emission of events. */ InternalEngine(std::unique_ptr callbacks, std::unique_ptr logger, - envoy_event_tracker event_tracker); + std::unique_ptr event_tracker); /** * InternalEngine destructor. @@ -123,7 +123,8 @@ class InternalEngine : public Logger::Loggable { GTEST_FRIEND_CLASS(InternalEngineTest, ThreadCreationFailed); InternalEngine(std::unique_ptr callbacks, std::unique_ptr logger, - envoy_event_tracker event_tracker, Thread::PosixThreadFactoryPtr thread_factory); + std::unique_ptr event_tracker, + Thread::PosixThreadFactoryPtr thread_factory); envoy_status_t main(std::shared_ptr options); static void logInterfaces(absl::string_view event, @@ -135,7 +136,7 @@ class InternalEngine : public Logger::Loggable { Stats::StatNameSetPtr stat_name_set_; std::unique_ptr callbacks_; std::unique_ptr logger_; - envoy_event_tracker event_tracker_; + std::unique_ptr event_tracker_; Assert::ActionRegistrationPtr assert_handler_registration_; Assert::ActionRegistrationPtr bug_handler_registration_; Thread::MutexBasicLockable mutex_; diff --git a/mobile/library/common/logger/logger_delegate.cc b/mobile/library/common/logger/logger_delegate.cc index 00c59b502af9..7803aa6989c0 100644 --- a/mobile/library/common/logger/logger_delegate.cc +++ b/mobile/library/common/logger/logger_delegate.cc @@ -9,14 +9,14 @@ namespace Logger { void EventTrackingDelegate::logWithStableName(absl::string_view stable_name, absl::string_view, absl::string_view, absl::string_view msg) { - if (event_tracker_.track == nullptr) { + if (event_tracker_ == nullptr || (*event_tracker_) == nullptr) { return; } - event_tracker_.track(Bridge::Utility::makeEnvoyMap({{"name", "event_log"}, - {"log_name", std::string(stable_name)}, - {"message", std::string(msg)}}), - event_tracker_.context); + (*event_tracker_) + ->on_track({{"name", "event_log"}, + {"log_name", std::string(stable_name)}, + {"message", std::string(msg)}}); } LambdaDelegate::LambdaDelegate(std::unique_ptr logger, diff --git a/mobile/library/common/logger/logger_delegate.h b/mobile/library/common/logger/logger_delegate.h index 7a4a7e2ec431..aba4ea2fee14 100644 --- a/mobile/library/common/logger/logger_delegate.h +++ b/mobile/library/common/logger/logger_delegate.h @@ -15,14 +15,14 @@ namespace Logger { class EventTrackingDelegate : public SinkDelegate { public: explicit EventTrackingDelegate(DelegatingLogSinkSharedPtr log_sink) - : SinkDelegate(log_sink), event_tracker_(*static_cast( - Api::External::retrieveApi(envoy_event_tracker_api_name))) {} + : SinkDelegate(log_sink), event_tracker_(static_cast*>( + Api::External::retrieveApi(ENVOY_EVENT_TRACKER_API_NAME))) {} void logWithStableName(absl::string_view stable_name, absl::string_view level, absl::string_view component, absl::string_view msg) override; private: - envoy_event_tracker& event_tracker_; + std::unique_ptr* event_tracker_; }; using EventTrackingDelegatePtr = std::unique_ptr; diff --git a/mobile/library/common/types/c_types.cc b/mobile/library/common/types/c_types.cc index 350881d28a63..4948909b42d9 100644 --- a/mobile/library/common/types/c_types.cc +++ b/mobile/library/common/types/c_types.cc @@ -72,5 +72,3 @@ const envoy_data envoy_nodata = {0, NULL, envoy_noop_release, NULL}; const envoy_headers envoy_noheaders = {0, NULL}; const envoy_stats_tags envoy_stats_notags = {0, NULL}; - -const char* envoy_event_tracker_api_name = "event_tracker_api"; diff --git a/mobile/library/common/types/c_types.h b/mobile/library/common/types/c_types.h index 4071c9cd0c49..7ace9d1c26bd 100644 --- a/mobile/library/common/types/c_types.h +++ b/mobile/library/common/types/c_types.h @@ -73,9 +73,6 @@ typedef enum { ENVOY_NET_WWAN = 2, } envoy_network_t; -// The name used to registered event tracker api. -extern const char* envoy_event_tracker_api_name; - #ifdef __cplusplus extern "C" { // release function #endif @@ -411,15 +408,6 @@ typedef void (*envoy_on_cancel_f)(envoy_stream_intel stream_intel, */ typedef void (*envoy_on_send_window_available_f)(envoy_stream_intel stream_intel, void* context); -/** - * Called when envoy's event tracker tracks an event. - * - * @param event, the dictionary with attributes that describe the event. - * @param context, contains the necessary state to carry out platform-specific dispatch and - * execution. - */ -typedef void (*envoy_event_tracker_track_f)(envoy_map event, const void* context); - #ifdef __cplusplus } // function pointers #endif @@ -440,15 +428,6 @@ typedef struct { void* context; } envoy_http_callbacks; -/** - * Interface for event tracking. - */ -typedef struct { - envoy_event_tracker_track_f track; - // Context passed through to callbacks to provide dispatch and execution state. - const void* context; -} envoy_event_tracker; - /** * The list of certificate verification results returned from Java side to the * C++ side. diff --git a/mobile/library/jni/jni_impl.cc b/mobile/library/jni/jni_impl.cc index 12f4a2279313..4319d33fb28a 100644 --- a/mobile/library/jni/jni_impl.cc +++ b/mobile/library/jni/jni_impl.cc @@ -13,7 +13,6 @@ #include "library/common/types/managed_envoy_headers.h" #include "library/jni/android_network_utility.h" #include "library/jni/import/jni_import.h" -#include "library/jni/jni_support.h" #include "library/jni/jni_utility.h" #include "library/jni/types/exception.h" #include "library/jni/types/java_virtual_machine.h" @@ -31,27 +30,6 @@ JNIEXPORT jint JNICALL JNI_OnLoad(JavaVM* vm, void* /*reserved*/) { return Envoy::JNI::JavaVirtualMachine::getJNIVersion(); } -// JniLibrary - -static void jvm_on_track(envoy_map events, const void* context) { - if (context == nullptr) { - return; - } - - Envoy::JNI::JniHelper jni_helper(Envoy::JNI::getEnv()); - Envoy::JNI::LocalRefUniquePtr events_hashmap = - Envoy::JNI::envoyMapToJavaMap(jni_helper, events); - - jobject j_context = static_cast(const_cast(context)); - Envoy::JNI::LocalRefUniquePtr jcls_EnvoyEventTracker = - jni_helper.getObjectClass(j_context); - jmethodID jmid_onTrack = - jni_helper.getMethodId(jcls_EnvoyEventTracker.get(), "track", "(Ljava/util/Map;)V"); - jni_helper.callVoidMethod(j_context, jmid_onTrack, events_hashmap.get()); - - release_envoy_map(events); -} - extern "C" JNIEXPORT void JNICALL Java_io_envoyproxy_envoymobile_engine_JniLibrary_setLogLevel(JNIEnv* /*env*/, jclass, jint level) { Envoy::Logger::Context::changeAllLogLevels(static_cast(level)); @@ -59,61 +37,84 @@ Java_io_envoyproxy_envoymobile_engine_JniLibrary_setLogLevel(JNIEnv* /*env*/, jc extern "C" JNIEXPORT jlong JNICALL Java_io_envoyproxy_envoymobile_engine_JniLibrary_initEngine( JNIEnv* env, jclass, jobject on_start_context, jobject envoy_logger_context, - jobject j_event_tracker) { - - jobject retained_on_start_context = - env->NewGlobalRef(on_start_context); // Required to keep context in memory + jobject event_tracker_context) { + //================================================================================================ + // EngineCallbacks + //================================================================================================ std::unique_ptr callbacks = std::make_unique(); - callbacks->on_engine_running = [retained_on_start_context] { - Envoy::JNI::JniHelper jni_helper(Envoy::JNI::getEnv()); - Envoy::JNI::LocalRefUniquePtr jcls_JvmonEngineRunningContext = - jni_helper.getObjectClass(retained_on_start_context); - jmethodID jmid_onEngineRunning = jni_helper.getMethodId( - jcls_JvmonEngineRunningContext.get(), "invokeOnEngineRunning", "()Ljava/lang/Object;"); - Envoy::JNI::LocalRefUniquePtr unused = - jni_helper.callObjectMethod(retained_on_start_context, jmid_onEngineRunning); - - // TODO(goaway): This isn't re-used by other engine callbacks, so it's safe to delete here. - // This will need to be updated for https://github.com/envoyproxy/envoy-mobile/issues/332 - jni_helper.getEnv()->DeleteGlobalRef(retained_on_start_context); - }; - callbacks->on_exit = [] { - // Note that this is not dispatched because the thread that - // needs to be detached is the engine thread. - // This function is called from the context of the engine's - // thread due to it being posted to the engine's event dispatcher. - Envoy::JNI::JavaVirtualMachine::detachCurrentThread(); - }; - - const jobject retained_logger_context = env->NewGlobalRef(envoy_logger_context); + if (on_start_context != nullptr) { + jobject retained_on_start_context = + env->NewGlobalRef(on_start_context); // Required to keep context in memory + callbacks->on_engine_running = [retained_on_start_context] { + Envoy::JNI::JniHelper jni_helper(Envoy::JNI::getEnv()); + Envoy::JNI::LocalRefUniquePtr java_on_engine_running_class = + jni_helper.getObjectClass(retained_on_start_context); + jmethodID java_on_engine_running_method_id = jni_helper.getMethodId( + java_on_engine_running_class.get(), "invokeOnEngineRunning", "()Ljava/lang/Object;"); + Envoy::JNI::LocalRefUniquePtr unused = + jni_helper.callObjectMethod(retained_on_start_context, java_on_engine_running_method_id); + + // TODO(goaway): This isn't re-used by other engine callbacks, so it's safe to delete here. + // This will need to be updated for https://github.com/envoyproxy/envoy-mobile/issues/332 + jni_helper.getEnv()->DeleteGlobalRef(retained_on_start_context); + }; + callbacks->on_exit = [] { + // Note that this is not dispatched because the thread that + // needs to be detached is the engine thread. + // This function is called from the context of the engine's + // thread due to it being posted to the engine's event dispatcher. + Envoy::JNI::JavaVirtualMachine::detachCurrentThread(); + }; + } + //================================================================================================ + // EnvoyLogger + //================================================================================================ std::unique_ptr logger = std::make_unique(); - logger->on_log = [retained_logger_context](Envoy::Logger::Logger::Levels level, - const std::string& message) { - Envoy::JNI::JniHelper jni_helper(Envoy::JNI::getEnv()); - Envoy::JNI::LocalRefUniquePtr java_message = jni_helper.newStringUtf(message.c_str()); - jint java_level = static_cast(level); - Envoy::JNI::LocalRefUniquePtr java_envoy_logger_class = - jni_helper.getObjectClass(retained_logger_context); - jmethodID java_log_method_id = - jni_helper.getMethodId(java_envoy_logger_class.get(), "log", "(ILjava/lang/String;)V"); - jni_helper.callVoidMethod(retained_logger_context, java_log_method_id, java_level, - java_message.get()); - }; - logger->on_exit = [retained_logger_context] { - Envoy::JNI::getEnv()->DeleteGlobalRef(retained_logger_context); - }; - - envoy_event_tracker event_tracker = {nullptr, nullptr}; - if (j_event_tracker != nullptr) { - // TODO(goaway): The retained_context leaks, but it's tied to the life of the engine. - // This will need to be updated for https://github.com/envoyproxy/envoy-mobile/issues/332. - jobject retained_context = env->NewGlobalRef(j_event_tracker); - event_tracker.track = jvm_on_track; - event_tracker.context = retained_context; + if (envoy_logger_context != nullptr) { + const jobject retained_logger_context = env->NewGlobalRef(envoy_logger_context); + logger->on_log = [retained_logger_context](Envoy::Logger::Logger::Levels level, + const std::string& message) { + Envoy::JNI::JniHelper jni_helper(Envoy::JNI::getEnv()); + Envoy::JNI::LocalRefUniquePtr java_message = + jni_helper.newStringUtf(message.c_str()); + jint java_level = static_cast(level); + Envoy::JNI::LocalRefUniquePtr java_envoy_logger_class = + jni_helper.getObjectClass(retained_logger_context); + jmethodID java_log_method_id = + jni_helper.getMethodId(java_envoy_logger_class.get(), "log", "(ILjava/lang/String;)V"); + jni_helper.callVoidMethod(retained_logger_context, java_log_method_id, java_level, + java_message.get()); + }; + logger->on_exit = [retained_logger_context] { + Envoy::JNI::getEnv()->DeleteGlobalRef(retained_logger_context); + }; + } + //================================================================================================ + // EnvoyEventTracker + //================================================================================================ + std::unique_ptr event_tracker = + std::make_unique(); + if (event_tracker_context != nullptr) { + const jobject retained_event_tracker_context = env->NewGlobalRef(event_tracker_context); + event_tracker->on_track = [retained_event_tracker_context]( + const absl::flat_hash_map& events) { + Envoy::JNI::JniHelper jni_helper(Envoy::JNI::getEnv()); + Envoy::JNI::LocalRefUniquePtr java_events = + Envoy::JNI::cppMapToJavaMap(jni_helper, events); + Envoy::JNI::LocalRefUniquePtr java_envoy_event_tracker_class = + jni_helper.getObjectClass(retained_event_tracker_context); + jmethodID java_track_method_id = jni_helper.getMethodId(java_envoy_event_tracker_class.get(), + "track", "(Ljava/util/Map;)V"); + jni_helper.callVoidMethod(retained_event_tracker_context, java_track_method_id, + java_events.get()); + }; + event_tracker->on_exit = [retained_event_tracker_context] { + Envoy::JNI::getEnv()->DeleteGlobalRef(retained_event_tracker_context); + }; } return reinterpret_cast( - new Envoy::InternalEngine(std::move(callbacks), std::move(logger), event_tracker)); + new Envoy::InternalEngine(std::move(callbacks), std::move(logger), std::move(event_tracker))); } extern "C" JNIEXPORT jint JNICALL Java_io_envoyproxy_envoymobile_engine_JniLibrary_runEngine( diff --git a/mobile/library/jni/jni_utility.h b/mobile/library/jni/jni_utility.h index 01ba556fff91..2a78a734d4ec 100644 --- a/mobile/library/jni/jni_utility.h +++ b/mobile/library/jni/jni_utility.h @@ -134,5 +134,23 @@ LocalRefUniquePtr protoToJavaByteArray(JniHelper& jni_helper, /** Converts from Java `String` to C++ string. */ std::string javaStringToString(JniHelper& jni_helper, jstring java_string); +/** Converts from C++'s map-type to Java's HashMap. */ +template +LocalRefUniquePtr cppMapToJavaMap(JniHelper& jni_helper, const MapType& cpp_map) { + auto java_map_class = jni_helper.findClass("java/util/HashMap"); + auto java_map_init_method_id = jni_helper.getMethodId(java_map_class.get(), "", "(I)V"); + auto java_map_put_method_id = jni_helper.getMethodId( + java_map_class.get(), "put", "(Ljava/lang/Object;Ljava/lang/Object;)Ljava/lang/Object;"); + auto java_map_object = + jni_helper.newObject(java_map_class.get(), java_map_init_method_id, cpp_map.size()); + for (const auto& [key, value] : cpp_map) { + auto java_key = jni_helper.newStringUtf(key.c_str()); + auto java_value = jni_helper.newStringUtf(value.c_str()); + auto ignored = jni_helper.callObjectMethod(java_map_object.get(), java_map_put_method_id, + java_key.get(), java_value.get()); + } + return java_map_object; +} + } // namespace JNI } // namespace Envoy diff --git a/mobile/library/objective-c/EnvoyEngineImpl.mm b/mobile/library/objective-c/EnvoyEngineImpl.mm index 38de2de5c34a..b4384346cbf6 100644 --- a/mobile/library/objective-c/EnvoyEngineImpl.mm +++ b/mobile/library/objective-c/EnvoyEngineImpl.mm @@ -359,15 +359,6 @@ static envoy_data ios_get_string(const void *context) { return toManagedNativeString(accessor.getEnvoyString()); } -static void ios_track_event(envoy_map map, const void *context) { - // This code block runs inside the Envoy event loop. Therefore, an explicit autoreleasepool block - // is necessary to act as a breaker for any Objective-C allocation that happens. - @autoreleasepool { - EnvoyEventTracker *eventTracker = (__bridge EnvoyEventTracker *)context; - eventTracker.track(to_ios_map(map)); - } -} - @implementation EnvoyEngineImpl { envoy_engine_t _engineHandle; Envoy::InternalEngine *_engine; @@ -414,18 +405,24 @@ - (instancetype)initWithRunningCallback:(nullable void (^)())onEngineRunning }; } - // TODO(Augustyniak): Everything here leaks, but it's all tied to the life of the engine. - // This will need to be updated for https://github.com/envoyproxy/envoy-mobile/issues/332. - envoy_event_tracker native_event_tracker = {NULL, NULL}; + std::unique_ptr native_event_tracker = + std::make_unique(); if (eventTracker) { - EnvoyEventTracker *objcEventTracker = - [[EnvoyEventTracker alloc] initWithEventTrackingClosure:eventTracker]; - native_event_tracker.track = ios_track_event; - native_event_tracker.context = CFBridgingRetain(objcEventTracker); + native_event_tracker->on_track = + [eventTracker = + std::move(eventTracker)](const absl::flat_hash_map &events) { + NSMutableDictionary *newMap = [NSMutableDictionary new]; + for (const auto &[cppKey, cppValue] : events) { + NSString *key = @(cppKey.c_str()); + NSString *value = @(cppValue.c_str()); + newMap[key] = value; + } + eventTracker(newMap); + }; } _engine = new Envoy::InternalEngine(std::move(native_callbacks), std::move(native_logger), - native_event_tracker); + std::move(native_event_tracker)); _engineHandle = reinterpret_cast(_engine); if (networkMonitoringMode == 1) { diff --git a/mobile/test/cc/BUILD b/mobile/test/cc/BUILD index e1015ed40171..9280895953ea 100644 --- a/mobile/test/cc/BUILD +++ b/mobile/test/cc/BUILD @@ -14,6 +14,7 @@ envoy_cc_test( repository = "@envoy", deps = [ "//library/cc:engine_builder_lib", + "@envoy//source/common/common:assert_lib", "@envoy_build_config//:test_extensions", ], ) diff --git a/mobile/test/cc/engine_test.cc b/mobile/test/cc/engine_test.cc index 2d2df43c04e6..1550edebd77b 100644 --- a/mobile/test/cc/engine_test.cc +++ b/mobile/test/cc/engine_test.cc @@ -1,3 +1,5 @@ +#include "source/common/common/assert.h" + #include "absl/synchronization/notification.h" #include "gtest/gtest.h" #include "library/cc/engine_builder.h" @@ -56,7 +58,7 @@ TEST(EngineTest, SetLogger) { stream_complete.WaitForNotification(); EXPECT_EQ(actual_status_code, 200); - EXPECT_EQ(actual_end_stream, true); + EXPECT_TRUE(actual_end_stream); EXPECT_TRUE(logging_was_called.load()); EXPECT_EQ(engine->terminate(), ENVOY_SUCCESS); } @@ -108,8 +110,44 @@ TEST(EngineTest, SetEngineCallbacks) { stream_complete.WaitForNotification(); EXPECT_EQ(actual_status_code, 200); - EXPECT_EQ(actual_end_stream, true); + EXPECT_TRUE(actual_end_stream); + EXPECT_EQ(engine->terminate(), ENVOY_SUCCESS); +} + +TEST(EngineTest, SetEventTracker) { + absl::Notification engine_running; + auto engine_callbacks = std::make_unique(); + engine_callbacks->on_engine_running = [&] { engine_running.Notify(); }; + + absl::Notification on_track; + auto event_tracker = std::make_unique(); + event_tracker->on_track = [&](const absl::flat_hash_map& events) { + if (events.count("name") && events.at("name") == "assertion") { + EXPECT_EQ(events.at("location"), "foo_location"); + on_track.Notify(); + } + }; + absl::Notification on_track_exit; + event_tracker->on_exit = [&] { on_track_exit.Notify(); }; + + Platform::EngineBuilder engine_builder; + Platform::EngineSharedPtr engine = + engine_builder.setLogLevel(Logger::Logger::debug) + .setEngineCallbacks(std::move(engine_callbacks)) + .setEventTracker(std::move(event_tracker)) + .addNativeFilter( + "test_remote_response", + "{'@type': " + "type.googleapis.com/" + "envoymobile.extensions.filters.http.test_remote_response.TestRemoteResponse}") + .build(); + engine_running.WaitForNotification(); + + Assert::invokeDebugAssertionFailureRecordActionForAssertMacroUseOnly("foo_location"); + + EXPECT_TRUE(on_track.WaitForNotificationWithTimeout(absl::Seconds(3))); EXPECT_EQ(engine->terminate(), ENVOY_SUCCESS); + EXPECT_TRUE(on_track.WaitForNotificationWithTimeout(absl::Seconds(3))); } } // namespace Envoy diff --git a/mobile/test/common/http/filters/test_event_tracker/BUILD b/mobile/test/common/http/filters/test_event_tracker/BUILD index 49472189a20f..ceb1fbd54d8d 100644 --- a/mobile/test/common/http/filters/test_event_tracker/BUILD +++ b/mobile/test/common/http/filters/test_event_tracker/BUILD @@ -24,6 +24,7 @@ envoy_cc_library( repository = "@envoy", deps = [ "filter_cc_proto", + "//library/common:engine_types_lib", "//library/common/api:c_types", "//library/common/api:external_api_lib", "//library/common/bridge:utility_lib", diff --git a/mobile/test/common/http/filters/test_event_tracker/filter.cc b/mobile/test/common/http/filters/test_event_tracker/filter.cc index 5f5d1733c7dc..fcb52c7c417a 100644 --- a/mobile/test/common/http/filters/test_event_tracker/filter.cc +++ b/mobile/test/common/http/filters/test_event_tracker/filter.cc @@ -11,17 +11,15 @@ namespace TestEventTracker { TestEventTrackerFilterConfig::TestEventTrackerFilterConfig( const envoymobile::extensions::filters::http::test_event_tracker::TestEventTracker& proto_config) - : event_tracker_(static_cast( - Api::External::retrieveApi(envoy_event_tracker_api_name))) { - auto attributes = std::vector>(); - for (auto& pair : proto_config.attributes()) { - attributes.push_back({std::string(pair.first), std::string(pair.second)}); + : event_tracker_(static_cast*>( + Api::External::retrieveApi(ENVOY_EVENT_TRACKER_API_NAME))) { + for (auto& [key, value] : proto_config.attributes()) { + attributes_.emplace(std::string(key), std::string(value)); } - attributes_ = attributes; } Http::FilterHeadersStatus TestEventTrackerFilter::decodeHeaders(Http::RequestHeaderMap&, bool) { - config_->track(Bridge::Utility::makeEnvoyMap(config_->attributes())); + config_->track(config_->attributes()); return Http::FilterHeadersStatus::Continue; } diff --git a/mobile/test/common/http/filters/test_event_tracker/filter.h b/mobile/test/common/http/filters/test_event_tracker/filter.h index ffc5e18086d2..814baad494f0 100644 --- a/mobile/test/common/http/filters/test_event_tracker/filter.h +++ b/mobile/test/common/http/filters/test_event_tracker/filter.h @@ -7,6 +7,7 @@ #include "test/common/http/filters/test_event_tracker/filter.pb.h" #include "library/common/api/c_types.h" +#include "library/common/engine_types.h" namespace Envoy { namespace Extensions { @@ -19,29 +20,29 @@ class TestEventTrackerFilterConfig { const envoymobile::extensions::filters::http::test_event_tracker::TestEventTracker& proto_config); - std::vector> attributes() { return attributes_; }; - void track(envoy_map event) { - if (event_tracker_->track != nullptr) { - event_tracker_->track(event, event_tracker_->context); + absl::flat_hash_map attributes() { return attributes_; }; + void track(const absl::flat_hash_map& events) { + if (event_tracker_ != nullptr) { + (*event_tracker_)->on_track(events); } - }; + } private: - std::vector> attributes_; - const envoy_event_tracker* event_tracker_; + absl::flat_hash_map attributes_; + const std::unique_ptr* event_tracker_; }; using TestEventTrackerFilterConfigSharedPtr = std::shared_ptr; // The filter that emits preconfigured events. It's supposed to be used for // testing of the event tracking functionality only. -class TestEventTrackerFilter final : public ::Envoy::Http::PassThroughFilter { +class TestEventTrackerFilter final : public Http::PassThroughFilter { public: TestEventTrackerFilter(TestEventTrackerFilterConfigSharedPtr config) : config_(config) {} // StreamDecoderFilter - ::Envoy::Http::FilterHeadersStatus decodeHeaders(::Envoy::Http::RequestHeaderMap& headers, - bool end_stream) override; + Http::FilterHeadersStatus decodeHeaders(Http::RequestHeaderMap& headers, + bool end_stream) override; private: const TestEventTrackerFilterConfigSharedPtr config_; diff --git a/mobile/test/common/internal_engine_test.cc b/mobile/test/common/internal_engine_test.cc index 7026e401dc76..1c1bc25f3b61 100644 --- a/mobile/test/common/internal_engine_test.cc +++ b/mobile/test/common/internal_engine_test.cc @@ -251,15 +251,13 @@ TEST_F(InternalEngineTest, EventTrackerRegistersDefaultAPI) { EngineTestContext test_context{}; std::unique_ptr engine( - new InternalEngine(createDefaultEngineCallbacks(test_context), {}, {})); + new InternalEngine(createDefaultEngineCallbacks(test_context), {}, nullptr)); engine->run(MINIMAL_TEST_CONFIG, LEVEL_DEBUG); // A default event tracker is registered in external API registry. - const auto registered_event_tracker = - static_cast(Api::External::retrieveApi(envoy_event_tracker_api_name)); - EXPECT_TRUE(registered_event_tracker != nullptr); - EXPECT_TRUE(registered_event_tracker->track == nullptr); - EXPECT_TRUE(registered_event_tracker->context == nullptr); + const auto registered_event_tracker = static_cast*>( + Api::External::retrieveApi(ENVOY_EVENT_TRACKER_API_NAME)); + EXPECT_TRUE(registered_event_tracker != nullptr && *registered_event_tracker == nullptr); ASSERT_TRUE(test_context.on_engine_running.WaitForNotificationWithTimeout(absl::Seconds(3))); // Simulate a failed assertion by invoking a debug assertion failure @@ -275,29 +273,23 @@ TEST_F(InternalEngineTest, EventTrackerRegistersDefaultAPI) { TEST_F(InternalEngineTest, EventTrackerRegistersAPI) { EngineTestContext test_context{}; - envoy_event_tracker event_tracker{[](envoy_map map, const void* context) -> void { - const auto new_map = toMap(map); - if (new_map.count("foo") && new_map.at("foo") == "bar") { - auto* test_context = static_cast( - const_cast(context)); - test_context->on_event.Notify(); - } - } /*track*/, - &test_context /*context*/}; + auto event_tracker = std::make_unique(); + event_tracker->on_track = [&](const absl::flat_hash_map& events) { + if (events.count("foo") && events.at("foo") == "bar") { + test_context.on_event.Notify(); + } + }; std::unique_ptr engine( - new InternalEngine(createDefaultEngineCallbacks(test_context), {}, event_tracker)); + new InternalEngine(createDefaultEngineCallbacks(test_context), {}, std::move(event_tracker))); engine->run(MINIMAL_TEST_CONFIG, LEVEL_DEBUG); ASSERT_TRUE(test_context.on_engine_running.WaitForNotificationWithTimeout(absl::Seconds(3))); - const auto registered_event_tracker = - static_cast(Api::External::retrieveApi(envoy_event_tracker_api_name)); - EXPECT_TRUE(registered_event_tracker != nullptr); - EXPECT_EQ(event_tracker.track, registered_event_tracker->track); - EXPECT_EQ(event_tracker.context, registered_event_tracker->context); + const auto registered_event_tracker = static_cast*>( + Api::External::retrieveApi(ENVOY_EVENT_TRACKER_API_NAME)); + EXPECT_TRUE(registered_event_tracker != nullptr && *registered_event_tracker != nullptr); - event_tracker.track(Bridge::Utility::makeEnvoyMap({{"foo", "bar"}}), - registered_event_tracker->context); + (*registered_event_tracker)->on_track({{"foo", "bar"}}); ASSERT_TRUE(test_context.on_event.WaitForNotificationWithTimeout(absl::Seconds(3))); engine->terminate(); @@ -307,19 +299,16 @@ TEST_F(InternalEngineTest, EventTrackerRegistersAPI) { TEST_F(InternalEngineTest, EventTrackerRegistersAssertionFailureRecordAction) { EngineTestContext test_context{}; - envoy_event_tracker event_tracker{ - [](envoy_map map, const void* context) -> void { - const auto new_map = toMap(map); - if (new_map.count("name") && new_map.at("name") == "assertion") { - EXPECT_EQ(new_map.at("location"), "foo_location"); - auto* test_context = static_cast(const_cast(context)); - test_context->on_event.Notify(); - } - } /*track*/, - &test_context /*context*/}; + auto event_tracker = std::make_unique(); + event_tracker->on_track = [&](const absl::flat_hash_map& events) { + if (events.count("name") && events.at("name") == "assertion") { + EXPECT_EQ(events.at("location"), "foo_location"); + test_context.on_event.Notify(); + } + }; std::unique_ptr engine( - new InternalEngine(createDefaultEngineCallbacks(test_context), {}, event_tracker)); + new InternalEngine(createDefaultEngineCallbacks(test_context), {}, std::move(event_tracker))); engine->run(MINIMAL_TEST_CONFIG, LEVEL_DEBUG); ASSERT_TRUE(test_context.on_engine_running.WaitForNotificationWithTimeout(absl::Seconds(3))); @@ -337,19 +326,16 @@ TEST_F(InternalEngineTest, EventTrackerRegistersAssertionFailureRecordAction) { TEST_F(InternalEngineTest, EventTrackerRegistersEnvoyBugRecordAction) { EngineTestContext test_context{}; - envoy_event_tracker event_tracker{[](envoy_map map, const void* context) -> void { - const auto new_map = toMap(map); - if (new_map.count("name") && new_map.at("name") == "bug") { - EXPECT_EQ(new_map.at("location"), "foo_location"); - auto* test_context = static_cast( - const_cast(context)); - test_context->on_event.Notify(); - } - } /*track*/, - &test_context /*context*/}; + auto event_tracker = std::make_unique(); + event_tracker->on_track = [&](const absl::flat_hash_map& events) { + if (events.count("name") && events.at("name") == "bug") { + EXPECT_EQ(events.at("location"), "foo_location"); + test_context.on_event.Notify(); + } + }; std::unique_ptr engine( - new InternalEngine(createDefaultEngineCallbacks(test_context), {}, event_tracker)); + new InternalEngine(createDefaultEngineCallbacks(test_context), {}, std::move(event_tracker))); engine->run(MINIMAL_TEST_CONFIG, LEVEL_DEBUG); ASSERT_TRUE(test_context.on_engine_running.WaitForNotificationWithTimeout(absl::Seconds(3))); diff --git a/mobile/test/common/logger/logger_delegate_test.cc b/mobile/test/common/logger/logger_delegate_test.cc index 3a3e4ce68241..4a330907ad84 100644 --- a/mobile/test/common/logger/logger_delegate_test.cc +++ b/mobile/test/common/logger/logger_delegate_test.cc @@ -13,14 +13,15 @@ namespace Logger { class LambdaDelegateTest : public testing::Test { public: - static envoy_event_tracker tracker; + static std::unique_ptr event_tracker; static void SetUpTestSuite() { - Api::External::registerApi(std::string(envoy_event_tracker_api_name), &tracker); + Api::External::registerApi(std::string(ENVOY_EVENT_TRACKER_API_NAME), &event_tracker); } }; -envoy_event_tracker LambdaDelegateTest::tracker{}; +std::unique_ptr LambdaDelegateTest::event_tracker = + std::make_unique(); TEST_F(LambdaDelegateTest, LogCb) { std::string expected_msg = "Hello LambdaDelegate"; diff --git a/mobile/test/java/io/envoyproxy/envoymobile/jni/JniUtilityTest.java b/mobile/test/java/io/envoyproxy/envoymobile/jni/JniUtilityTest.java index 963b3b3a7bff..7457c4d50058 100644 --- a/mobile/test/java/io/envoyproxy/envoymobile/jni/JniUtilityTest.java +++ b/mobile/test/java/io/envoyproxy/envoymobile/jni/JniUtilityTest.java @@ -8,6 +8,8 @@ import org.junit.runner.RunWith; import org.robolectric.RobolectricTestRunner; +import java.util.Map; + @RunWith(RobolectricTestRunner.class) public class JniUtilityTest { public JniUtilityTest() { System.loadLibrary("envoy_jni_utility_test"); } @@ -16,6 +18,7 @@ public class JniUtilityTest { // Native methods for testing. //================================================================================ public static native byte[] protoJavaByteArrayConversion(byte[] source); + public static native Map cppMapToJavaMap(); @Test public void testProtoJavaByteArrayConversion() throws Exception { @@ -28,4 +31,10 @@ public void testProtoJavaByteArrayConversion() throws Exception { Struct dest = Struct.parseFrom(protoJavaByteArrayConversion(source.toByteArray())); assertThat(source).isEqualTo(dest); } + + @Test + public void testCppStringMapToJavaStringMap() { + Map map = cppMapToJavaMap(); + assertThat(map).containsExactly("key1", "value1", "key2", "value2", "key3", "value3"); + } } diff --git a/mobile/test/java/org/chromium/net/BidirectionalStreamTest.java b/mobile/test/java/org/chromium/net/BidirectionalStreamTest.java index d6de3af23ca9..4c3ea0b75b10 100644 --- a/mobile/test/java/org/chromium/net/BidirectionalStreamTest.java +++ b/mobile/test/java/org/chromium/net/BidirectionalStreamTest.java @@ -831,6 +831,7 @@ public void testCustomCronetEngineUserAgent() throws Exception { callback.blockForDone(); assertEquals(200, callback.mResponseInfo.getHttpStatusCode()); assertEquals(userAgentValue, callback.mResponseAsString); + engine.shutdown(); } @Test diff --git a/mobile/test/jni/jni_utility_test.cc b/mobile/test/jni/jni_utility_test.cc index 96a26eafd21e..e2a5f8a776a6 100644 --- a/mobile/test/jni/jni_utility_test.cc +++ b/mobile/test/jni/jni_utility_test.cc @@ -1,5 +1,6 @@ #include +#include "absl/container/flat_hash_map.h" #include "library/jni/jni_utility.h" // NOLINT(namespace-envoy) @@ -15,3 +16,14 @@ Java_io_envoyproxy_envoymobile_jni_JniUtilityTest_protoJavaByteArrayConversion(J Envoy::JNI::javaByteArrayToProto(jni_helper, source, &s); return Envoy::JNI::protoToJavaByteArray(jni_helper, s).release(); } + +extern "C" JNIEXPORT jobject JNICALL +Java_io_envoyproxy_envoymobile_jni_JniUtilityTest_cppMapToJavaMap(JNIEnv* env, jclass) { + Envoy::JNI::JniHelper jni_helper(env); + absl::flat_hash_map cpp_map{ + {"key1", "value1"}, + {"key2", "value2"}, + {"key3", "value3"}, + }; + return Envoy::JNI::cppMapToJavaMap(jni_helper, cpp_map).release(); +} diff --git a/source/common/common/logger.cc b/source/common/common/logger.cc index 57e65b316d24..bf2b15dfead4 100644 --- a/source/common/common/logger.cc +++ b/source/common/common/logger.cc @@ -158,6 +158,17 @@ void DelegatingLogSink::setTlsDelegate(SinkDelegate* sink) { *tlsSink() = sink; SinkDelegate* DelegatingLogSink::tlsDelegate() { return *tlsSink(); } +void DelegatingLogSink::logWithStableName(absl::string_view stable_name, absl::string_view level, + absl::string_view component, absl::string_view message) { + auto tls_sink = tlsDelegate(); + if (tls_sink != nullptr) { + tls_sink->logWithStableName(stable_name, level, component, message); + return; + } + absl::ReaderMutexLock sink_lock(&sink_mutex_); + sink_->logWithStableName(stable_name, level, component, message); +} + static std::atomic current_context = nullptr; static_assert(std::atomic::is_always_lock_free); diff --git a/source/common/common/logger.h b/source/common/common/logger.h index 7fb16e1ae1d8..3c17e05da5b9 100644 --- a/source/common/common/logger.h +++ b/source/common/common/logger.h @@ -205,19 +205,8 @@ class DelegatingLogSink : public spdlog::sinks::sink { void setLock(Thread::BasicLockable& lock) { stderr_sink_->setLock(lock); } void clearLock() { stderr_sink_->clearLock(); } - template void logWithStableName(absl::string_view stable_name, absl::string_view level, - absl::string_view component, FmtStr fmt_str, Args... msg) { - auto tls_sink = tlsDelegate(); - if (tls_sink != nullptr) { - tls_sink->logWithStableName(stable_name, level, component, - fmt::format(fmt::runtime(fmt_str), msg...)); - return; - } - absl::ReaderMutexLock sink_lock(&sink_mutex_); - sink_->logWithStableName(stable_name, level, component, - fmt::format(fmt::runtime(fmt_str), msg...)); - } + absl::string_view component, absl::string_view message); // spdlog::sinks::sink void log(const spdlog::details::log_msg& msg) override; void flush() override; @@ -657,7 +646,7 @@ class ExtractedMessage : public spdlog::custom_flag_formatter { ENVOY_LOG_TO_LOGGER(LOGGER, LEVEL, ##__VA_ARGS__); \ if (ENVOY_LOG_COMP_LEVEL(LOGGER, LEVEL)) { \ ::Envoy::Logger::Registry::getSink()->logWithStableName(EVENT_NAME, #LEVEL, (LOGGER).name(), \ - ##__VA_ARGS__); \ + fmt::format(__VA_ARGS__)); \ } \ } while (0) @@ -669,8 +658,9 @@ class ExtractedMessage : public spdlog::custom_flag_formatter { ENVOY_LOG_TO_LOGGER(ENVOY_LOGGER(), LEVEL, "{}" FORMAT, \ ::Envoy::Logger::Utility::serializeLogTags(log_tags), ##__VA_ARGS__); \ ::Envoy::Logger::Registry::getSink()->logWithStableName( \ - EVENT_NAME, #LEVEL, (ENVOY_LOGGER()).name(), "{}" FORMAT, \ - ::Envoy::Logger::Utility::serializeLogTags(log_tags), ##__VA_ARGS__); \ + EVENT_NAME, #LEVEL, (ENVOY_LOGGER()).name(), \ + fmt::format("{}" FORMAT, ::Envoy::Logger::Utility::serializeLogTags(log_tags), \ + ##__VA_ARGS__)); \ } \ } while (0) diff --git a/source/common/tcp_proxy/tcp_proxy.cc b/source/common/tcp_proxy/tcp_proxy.cc index 6f059270cb12..9bc074f393a2 100644 --- a/source/common/tcp_proxy/tcp_proxy.cc +++ b/source/common/tcp_proxy/tcp_proxy.cc @@ -159,7 +159,8 @@ Config::Config(const envoy::extensions::filters::network::tcp_proxy::v3::TcpProx : max_connect_attempts_(PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, max_connect_attempts, 1)), upstream_drain_manager_slot_(context.serverFactoryContext().threadLocal().allocateSlot()), shared_config_(std::make_shared(config, context)), - random_generator_(context.serverFactoryContext().api().randomGenerator()) { + random_generator_(context.serverFactoryContext().api().randomGenerator()), + regex_engine_(context.serverFactoryContext().regexEngine()) { upstream_drain_manager_slot_->set([](Event::Dispatcher&) { ThreadLocal::ThreadLocalObjectSharedPtr drain_manager = std::make_shared(); @@ -563,7 +564,8 @@ bool Filter::maybeTunnel(Upstream::ThreadLocalCluster& cluster) { // TODO(vikaschoudhary16): Initialize route_ once per cluster. upstream_decoder_filter_callbacks_.route_ = std::make_shared( cluster.info()->name(), - *std::unique_ptr{new Router::RetryPolicyImpl()}); + *std::unique_ptr{new Router::RetryPolicyImpl()}, + config_->regexEngine()); } generic_conn_pool_ = factory->createGenericConnPool( cluster, config_->tunnelingConfigHelper(), this, *upstream_callbacks_, diff --git a/source/common/tcp_proxy/tcp_proxy.h b/source/common/tcp_proxy/tcp_proxy.h index 4019ea1211ef..04f5727ed7b9 100644 --- a/source/common/tcp_proxy/tcp_proxy.h +++ b/source/common/tcp_proxy/tcp_proxy.h @@ -315,6 +315,7 @@ class Config { const OnDemandStats& onDemandStats() const { return shared_config_->onDemandConfig()->stats(); } Random::RandomGenerator& randomGenerator() { return random_generator_; } bool flushAccessLogOnConnected() const { return shared_config_->flushAccessLogOnConnected(); } + Regex::Engine& regexEngine() const { return regex_engine_; } private: struct SimpleRouteImpl : public Route { @@ -367,6 +368,7 @@ class Config { std::unique_ptr cluster_metadata_match_criteria_; Random::RandomGenerator& random_generator_; std::unique_ptr hash_policy_; + Regex::Engine& regex_engine_; // Static lifetime object, safe to store as a reference }; using ConfigSharedPtr = std::shared_ptr; diff --git a/source/common/tls/context_config_impl.cc b/source/common/tls/context_config_impl.cc index 77be4ba6b4a4..1f3000a24f78 100644 --- a/source/common/tls/context_config_impl.cc +++ b/source/common/tls/context_config_impl.cc @@ -172,6 +172,7 @@ ContextConfigImpl::ContextConfigImpl( : api_(factory_context.serverFactoryContext().api()), options_(factory_context.serverFactoryContext().options()), singleton_manager_(factory_context.serverFactoryContext().singletonManager()), + lifecycle_notifier_(factory_context.serverFactoryContext().lifecycleNotifier()), alpn_protocols_(RepeatedPtrUtil::join(config.alpn_protocols(), ",")), cipher_suites_(StringUtil::nonEmptyStringOrDefault( RepeatedPtrUtil::join(config.tls_params().cipher_suites(), ":"), default_cipher_suites)), @@ -233,7 +234,7 @@ ContextConfigImpl::ContextConfigImpl( } HandshakerFactoryContextImpl handshaker_factory_context(api_, options_, alpn_protocols_, - singleton_manager_); + singleton_manager_, lifecycle_notifier_); Ssl::HandshakerFactory* handshaker_factory; if (config.has_custom_handshaker()) { // If a custom handshaker is configured, derive the factory from the config. diff --git a/source/common/tls/context_config_impl.h b/source/common/tls/context_config_impl.h index 5c43c6bab47e..b2a84ee4e46b 100644 --- a/source/common/tls/context_config_impl.h +++ b/source/common/tls/context_config_impl.h @@ -77,6 +77,7 @@ class ContextConfigImpl : public virtual Ssl::ContextConfig { Api::Api& api_; const Server::Options& options_; Singleton::Manager& singleton_manager_; + Server::ServerLifecycleNotifier& lifecycle_notifier_; private: static unsigned tlsVersionFromProto( diff --git a/source/common/tls/ssl_handshaker.h b/source/common/tls/ssl_handshaker.h index 3e5c63125b5f..1a61777e3b24 100644 --- a/source/common/tls/ssl_handshaker.h +++ b/source/common/tls/ssl_handshaker.h @@ -116,21 +116,24 @@ class HandshakerFactoryContextImpl : public Ssl::HandshakerFactoryContext { public: HandshakerFactoryContextImpl(Api::Api& api, const Server::Options& options, absl::string_view alpn_protocols, - Singleton::Manager& singleton_manager) + Singleton::Manager& singleton_manager, + Server::ServerLifecycleNotifier& lifecycle_notifier) : api_(api), options_(options), alpn_protocols_(alpn_protocols), - singleton_manager_(singleton_manager) {} + singleton_manager_(singleton_manager), lifecycle_notifier_(lifecycle_notifier) {} // HandshakerFactoryContext Api::Api& api() override { return api_; } const Server::Options& options() const override { return options_; } absl::string_view alpnProtocols() const override { return alpn_protocols_; } Singleton::Manager& singletonManager() override { return singleton_manager_; } + Server::ServerLifecycleNotifier& lifecycleNotifier() override { return lifecycle_notifier_; } private: Api::Api& api_; const Server::Options& options_; const std::string alpn_protocols_; Singleton::Manager& singleton_manager_; + Server::ServerLifecycleNotifier& lifecycle_notifier_; }; class HandshakerFactoryImpl : public Ssl::HandshakerFactory { diff --git a/source/server/server.cc b/source/server/server.cc index 065aebdb6b6a..8e79b675b062 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -1073,9 +1073,15 @@ InstanceBase::registerCallback(Stage stage, StageCallbackWithCompletion callback void InstanceBase::notifyCallbacksForStage(Stage stage, std::function completion_cb) { ASSERT_IS_MAIN_OR_TEST_THREAD(); - const auto it = stage_callbacks_.find(stage); - if (it != stage_callbacks_.end()) { - for (const StageCallback& callback : it->second) { + const auto stage_it = stage_callbacks_.find(stage); + if (stage_it != stage_callbacks_.end()) { + LifecycleNotifierCallbacks& callbacks = stage_it->second; + for (auto callback_it = callbacks.begin(); callback_it != callbacks.end();) { + StageCallback callback = *callback_it; + // Increment the iterator before invoking the callback in case the + // callback deletes the handle which will unregister itself and + // invalidate this iterator if we're still pointing at it. + ++callback_it; callback(); } } diff --git a/test/common/tls/handshaker_factory_test.cc b/test/common/tls/handshaker_factory_test.cc index e161b5eea047..d38f69b47534 100644 --- a/test/common/tls/handshaker_factory_test.cc +++ b/test/common/tls/handshaker_factory_test.cc @@ -193,6 +193,7 @@ TEST_F(HandshakerFactoryTest, HandshakerContextProvidesObjectsFromParentContext) // provided to the parent context. EXPECT_THAT(context.api(), Ref(mock_factory_ctx.api_)); EXPECT_THAT(context.options(), Ref(mock_factory_ctx.options_)); + EXPECT_THAT(context.lifecycleNotifier(), Ref(mock_factory_ctx.lifecycle_notifier_)); })); Extensions::TransportSockets::Tls::ClientSslSocketFactory socket_factory( diff --git a/test/mocks/server/transport_socket_factory_context.cc b/test/mocks/server/transport_socket_factory_context.cc index a9df701ac278..d06c2711f72f 100644 --- a/test/mocks/server/transport_socket_factory_context.cc +++ b/test/mocks/server/transport_socket_factory_context.cc @@ -26,6 +26,7 @@ MockTransportSocketFactoryContext::MockTransportSocketFactoryContext() ON_CALL(server_context_, options()).WillByDefault(ReturnRef(options_)); ON_CALL(server_context_, accessLogManager()).WillByDefault(ReturnRef(access_log_manager_)); ON_CALL(server_context_, singletonManager()).WillByDefault(ReturnRef(singleton_manager_)); + ON_CALL(server_context_, lifecycleNotifier()).WillByDefault(ReturnRef(lifecycle_notifier_)); } MockTransportSocketFactoryContext::~MockTransportSocketFactoryContext() = default; diff --git a/test/mocks/server/transport_socket_factory_context.h b/test/mocks/server/transport_socket_factory_context.h index 035f50e09aa4..828bf47b44d9 100644 --- a/test/mocks/server/transport_socket_factory_context.h +++ b/test/mocks/server/transport_socket_factory_context.h @@ -41,6 +41,7 @@ class MockTransportSocketFactoryContext : public TransportSocketFactoryContext { testing::NiceMock options_; std::unique_ptr secret_manager_; testing::NiceMock access_log_manager_; + testing::NiceMock lifecycle_notifier_; Singleton::ManagerImpl singleton_manager_; }; } // namespace Configuration diff --git a/test/server/server_test.cc b/test/server/server_test.cc index 3c67fcb4bf3d..d16ae34c334b 100644 --- a/test/server/server_test.cc +++ b/test/server/server_test.cc @@ -591,10 +591,12 @@ TEST_P(ServerInstanceImplTest, LifecycleNotifications) { post_init = true; post_init_fired.Notify(); }); - auto handle3 = server_->registerCallback(ServerLifecycleNotifier::Stage::ShutdownExit, [&] { - shutdown = true; - shutdown_begin.Notify(); - }); + ServerLifecycleNotifier::HandlePtr handle3 = + server_->registerCallback(ServerLifecycleNotifier::Stage::ShutdownExit, [&] { + shutdown = true; + shutdown_begin.Notify(); + handle3 = nullptr; + }); auto handle4 = server_->registerCallback(ServerLifecycleNotifier::Stage::ShutdownExit, [&](Event::PostCb completion_cb) { // Block till we're told to complete @@ -612,7 +614,7 @@ TEST_P(ServerInstanceImplTest, LifecycleNotifications) { server_->run(); handle1 = nullptr; handle2 = nullptr; - handle3 = nullptr; + // handle3 is nulled out in the callback itself, to test that works as well handle4 = nullptr; server_ = nullptr; thread_local_ = nullptr;