From cbc1bfcf9374788f363b6b179afbe7a909b4de9b Mon Sep 17 00:00:00 2001 From: Rafal Augustyniak Date: Sat, 3 Dec 2022 00:01:55 -0500 Subject: [PATCH 01/20] checking for exceptions Signed-off-by: Rafal Augustyniak --- mobile/library/common/jni/jni_interface.cc | 41 +++++++++++++++++++--- mobile/library/common/jni/jni_utility.cc | 26 ++++++++++++++ mobile/library/common/jni/jni_utility.h | 11 ++++++ 3 files changed, 73 insertions(+), 5 deletions(-) diff --git a/mobile/library/common/jni/jni_interface.cc b/mobile/library/common/jni/jni_interface.cc index cc9942f22459a..144be514b7090 100644 --- a/mobile/library/common/jni/jni_interface.cc +++ b/mobile/library/common/jni/jni_interface.cc @@ -306,7 +306,7 @@ static void pass_headers(const char* method, envoy_headers headers, jobject j_co } env->DeleteLocalRef(jcls_JvmCallbackContext); - release_envoy_headers(headers); + // release_envoy_headers(headers); } // Platform callback implementation @@ -329,11 +329,30 @@ static void* jvm_on_headers(const char* method, envoy_headers headers, bool end_ // TODO: make this cast safer. jobject result = env->CallObjectMethod(j_context, jmid_onHeaders, (jlong)headers.length, end_stream ? JNI_TRUE : JNI_FALSE, j_stream_intel); + if (check_for_exception(env)) { + env->DeleteLocalRef(j_stream_intel); + env->DeleteLocalRef(jcls_JvmCallbackContext); + + jclass jcls_object_array = env->FindClass("java/lang/Object"); + jobjectArray result = env->NewObjectArray(2, jcls_object_array, NULL); + + jclass jcls_int = env->FindClass("java/lang/Integer"); + jmethodID jmid_intInit = env->GetMethodID(jcls_int, "", "(I)V"); + jobject status = env->NewObject(jcls_int, jmid_intInit, 0); + env->SetObjectArrayElement(result, 0, status); + + env->SetObjectArrayElement(result, 1, ToJavaArrayOfJObjects(env, headers)); + + env->DeleteLocalRef(jcls_object_array); + env->DeleteLocalRef(jcls_int); + + return result; + } else { + env->DeleteLocalRef(j_stream_intel); + env->DeleteLocalRef(jcls_JvmCallbackContext); - env->DeleteLocalRef(j_stream_intel); - env->DeleteLocalRef(jcls_JvmCallbackContext); - - return result; + return result; + } } static void* jvm_on_response_headers(envoy_headers headers, bool end_stream, @@ -713,6 +732,10 @@ static void* call_jvm_on_complete(envoy_stream_intel stream_intel, jlongArray j_final_stream_intel = native_final_stream_intel_to_array(env, final_stream_intel); jobject result = env->CallObjectMethod(j_context, jmid_onComplete, j_stream_intel, j_final_stream_intel); + if (env->ExceptionCheck() == JNI_TRUE) { + env->ExceptionDescribe(); + env->ExceptionClear(); + } env->DeleteLocalRef(j_stream_intel); env->DeleteLocalRef(j_final_stream_intel); @@ -736,6 +759,10 @@ static void* call_jvm_on_error(envoy_error error, envoy_stream_intel stream_inte jobject result = env->CallObjectMethod(j_context, jmid_onError, error.error_code, j_error_message, error.attempt_count, j_stream_intel, j_final_stream_intel); + if (env->ExceptionCheck() == JNI_TRUE) { + env->ExceptionDescribe(); + env->ExceptionClear(); + } env->DeleteLocalRef(j_stream_intel); env->DeleteLocalRef(j_final_stream_intel); @@ -768,6 +795,10 @@ static void* call_jvm_on_cancel(envoy_stream_intel stream_intel, jobject result = env->CallObjectMethod(j_context, jmid_onCancel, j_stream_intel, j_final_stream_intel); + if (env->ExceptionCheck() == JNI_TRUE) { + env->ExceptionDescribe(); + env->ExceptionClear(); + } env->DeleteLocalRef(j_stream_intel); env->DeleteLocalRef(j_final_stream_intel); diff --git a/mobile/library/common/jni/jni_utility.cc b/mobile/library/common/jni/jni_utility.cc index 8fbc0da0b8d94..695ae025c97f8 100644 --- a/mobile/library/common/jni/jni_utility.cc +++ b/mobile/library/common/jni/jni_utility.cc @@ -66,6 +66,17 @@ void jni_delete_const_global_ref(const void* context) { jni_delete_global_ref(const_cast(context)); } +bool check_for_exception(JNIEnv *env) { + if (env->ExceptionCheck() == JNI_TRUE) { + env->ExceptionDescribe(); + env->ExceptionClear(); + ENVOY_LOG_EVENT_TO_LOGGER(GET_MISC_LOGGER(), error, "jni_exception", ""); + return true; + } else { + return false; + } +} + int unbox_integer(JNIEnv* env, jobject boxedInteger) { jclass jcls_Integer = env->FindClass("java/lang/Integer"); jmethodID jmid_intValue = env->GetMethodID(jcls_Integer, "intValue", "()I"); @@ -278,6 +289,21 @@ envoy_map to_native_map(JNIEnv* env, jobjectArray entries) { return native_map; } +jobjectArray ToJavaArrayOfJObjects(JNIEnv* env, envoy_map map) { + jclass jcls_byte_array = env->FindClass("java/lang/Object"); + jobjectArray javaArray = env->NewObjectArray(2 * map.length, jcls_byte_array, nullptr); + + for (envoy_map_size_t i = 0; i < map.length; i++) { + jbyteArray key = native_data_to_array(env, map.entries[i].key); + jbyteArray value = native_data_to_array(env, map.entries[i].value); + + env->SetObjectArrayElement(javaArray, 2 * i, key); + env->SetObjectArrayElement(javaArray, 2 * i + 1, value); + } + + return javaArray; +} + jobjectArray ToJavaArrayOfByteArray(JNIEnv* env, const std::vector& v) { jclass jcls_byte_array = env->FindClass("[B"); jobjectArray joa = env->NewObjectArray(v.size(), jcls_byte_array, nullptr); diff --git a/mobile/library/common/jni/jni_utility.h b/mobile/library/common/jni/jni_utility.h index 2c7f2875f5eff..d12a9b7907e8e 100644 --- a/mobile/library/common/jni/jni_utility.h +++ b/mobile/library/common/jni/jni_utility.h @@ -43,6 +43,15 @@ void jni_delete_global_ref(void* context); void jni_delete_const_global_ref(const void* context); +/** + * Checks whether there is a pending JNI exception and clear it if necessary. + * + * @param env, the JNI env pointer. + * + * @return Whether there was a pending JNI exception. + */ +bool check_for_exception(JNIEnv *env); + int unbox_integer(JNIEnv* env, jobject boxedInteger); envoy_data array_to_native_data(JNIEnv* env, jbyteArray j_data); @@ -100,6 +109,8 @@ jbyteArray ToJavaByteArray(JNIEnv* env, const uint8_t* bytes, size_t len); jbyteArray ToJavaByteArray(JNIEnv* env, const std::string& str); +jobjectArray ToJavaArrayOfJObjects(JNIEnv* env, envoy_map map); + void JavaArrayOfByteArrayToStringVector(JNIEnv* env, jobjectArray array, std::vector* out); From d74110c38133a1c3a39315939629db14dee0eec6 Mon Sep 17 00:00:00 2001 From: Rafal Augustyniak Date: Mon, 5 Dec 2022 08:39:58 -0500 Subject: [PATCH 02/20] checking for exceptions Signed-off-by: Rafal Augustyniak --- mobile/library/common/jni/BUILD | 4 +++ mobile/library/common/jni/jni_interface.cc | 25 +++++++++++-------- mobile/library/common/jni/jni_utility.cc | 12 ++++----- mobile/library/common/jni/jni_utility.h | 5 ++-- mobile/library/common/types/BUILD | 12 +++++++++ .../common/types/managed_envoy_headers.h | 14 +++++++++++ 6 files changed, 53 insertions(+), 19 deletions(-) create mode 100644 mobile/library/common/types/managed_envoy_headers.h diff --git a/mobile/library/common/jni/BUILD b/mobile/library/common/jni/BUILD index 13536a8f68d77..608c0a5f273dd 100644 --- a/mobile/library/common/jni/BUILD +++ b/mobile/library/common/jni/BUILD @@ -21,6 +21,7 @@ cc_library( deps = [ "//library/common/jni/import:jni_import_lib", "//library/common/types:c_types_lib", + "//library/common/types:managed_envoy_headers_lib", "@envoy//source/common/common:assert_lib", ], ) @@ -48,6 +49,7 @@ cc_library( ":ndk_jni_support", "//library/common:envoy_main_interface_lib", "//library/common/api:c_types", + "//library/common/types:managed_envoy_headers_lib", ], # We need this to ensure that we link this into the .so even though there are no code references. alwayslink = True, @@ -115,6 +117,7 @@ cc_binary( "//library/common:envoy_main_interface_lib", "//library/common/api:c_types", "//library/common/jni/import:jni_import_lib", + "//library/common/types:managed_envoy_headers_lib", ], ) @@ -169,6 +172,7 @@ cc_library( "//library/common/jni/import:jni_import_lib", "//library/common:envoy_main_interface_lib", "//library/common/types:c_types_lib", + "//library/common/types:managed_envoy_headers_lib", "@envoy//source/common/common:assert_lib", ] + TEST_EXTENSIONS, ) diff --git a/mobile/library/common/jni/jni_interface.cc b/mobile/library/common/jni/jni_interface.cc index 144be514b7090..587430c03994c 100644 --- a/mobile/library/common/jni/jni_interface.cc +++ b/mobile/library/common/jni/jni_interface.cc @@ -8,6 +8,7 @@ #include "library/common/jni/jni_utility.h" #include "library/common/jni/jni_version.h" #include "library/common/main_interface.h" +#include "library/common/types/managed_envoy_headers.h" // NOLINT(namespace-envoy) @@ -278,22 +279,22 @@ Java_io_envoyproxy_envoymobile_engine_JniLibrary_recordHistogramValue(JNIEnv* en // JvmCallbackContext -static void pass_headers(const char* method, envoy_headers headers, jobject j_context) { +static void pass_headers(const char* method, const ManagedEnvoyHeaders& headers, jobject j_context) { JNIEnv* env = get_env(); jclass jcls_JvmCallbackContext = env->GetObjectClass(j_context); jmethodID jmid_passHeader = env->GetMethodID(jcls_JvmCallbackContext, method, "([B[BZ)V"); jboolean start_headers = JNI_TRUE; - for (envoy_map_size_t i = 0; i < headers.length; i++) { + for (envoy_map_size_t i = 0; i < headers.get().length; i++) { // Note: this is just an initial implementation, and we will pass a more optimized structure in // the future. // Note: the JNI function NewStringUTF would appear to be an appealing option here, except it // requires a null-terminated *modified* UTF-8 string. // Create platform byte array for header key - jbyteArray j_key = native_data_to_array(env, headers.entries[i].key); + jbyteArray j_key = native_data_to_array(env, headers.get().entries[i].key); // Create platform byte array for header value - jbyteArray j_value = native_data_to_array(env, headers.entries[i].value); + jbyteArray j_value = native_data_to_array(env, headers.get().entries[i].value); // Pass this header pair to the platform env->CallVoidMethod(j_context, jmid_passHeader, j_key, j_value, start_headers); @@ -306,14 +307,13 @@ static void pass_headers(const char* method, envoy_headers headers, jobject j_co } env->DeleteLocalRef(jcls_JvmCallbackContext); - // release_envoy_headers(headers); } // Platform callback implementation // These methods call jvm methods which means the local references created will not be // released automatically. Manual bookkeeping is required for these methods. -static void* jvm_on_headers(const char* method, envoy_headers headers, bool end_stream, +static void* jvm_on_headers(const char* method, const ManagedEnvoyHeaders& headers, bool end_stream, envoy_stream_intel stream_intel, void* context) { jni_log("[Envoy]", "jvm_on_headers"); JNIEnv* env = get_env(); @@ -327,9 +327,9 @@ static void* jvm_on_headers(const char* method, envoy_headers headers, bool end_ jlongArray j_stream_intel = native_stream_intel_to_array(env, stream_intel); // Note: be careful of JVM types. Before we casted to jlong we were getting integer problems. // TODO: make this cast safer. - jobject result = env->CallObjectMethod(j_context, jmid_onHeaders, (jlong)headers.length, + jobject result = env->CallObjectMethod(j_context, jmid_onHeaders, (jlong)headers.get().length, end_stream ? JNI_TRUE : JNI_FALSE, j_stream_intel); - if (check_for_exception(env)) { + if (check_exception(env)) { env->DeleteLocalRef(j_stream_intel); env->DeleteLocalRef(jcls_JvmCallbackContext); @@ -357,13 +357,15 @@ static void* jvm_on_headers(const char* method, envoy_headers headers, bool end_ static void* jvm_on_response_headers(envoy_headers headers, bool end_stream, envoy_stream_intel stream_intel, void* context) { - return jvm_on_headers("onResponseHeaders", headers, end_stream, stream_intel, context); + const auto managed_headers = ManagedEnvoyHeaders(headers); + return jvm_on_headers("onResponseHeaders", managed_headers, end_stream, stream_intel, context); } static envoy_filter_headers_status -jvm_http_filter_on_request_headers(envoy_headers headers, bool end_stream, +jvm_http_filter_on_request_headers(envoy_headers input_headers, bool end_stream, envoy_stream_intel stream_intel, const void* context) { JNIEnv* env = get_env(); + const auto headers = ManagedEnvoyHeaders(input_headers); jobjectArray result = static_cast(jvm_on_headers( "onRequestHeaders", headers, end_stream, stream_intel, const_cast(context))); @@ -382,9 +384,10 @@ jvm_http_filter_on_request_headers(envoy_headers headers, bool end_stream, } static envoy_filter_headers_status -jvm_http_filter_on_response_headers(envoy_headers headers, bool end_stream, +jvm_http_filter_on_response_headers(envoy_headers input_headers, bool end_stream, envoy_stream_intel stream_intel, const void* context) { JNIEnv* env = get_env(); + const auto headers = ManagedEnvoyHeaders(input_headers); jobjectArray result = static_cast(jvm_on_headers( "onResponseHeaders", headers, end_stream, stream_intel, const_cast(context))); diff --git a/mobile/library/common/jni/jni_utility.cc b/mobile/library/common/jni/jni_utility.cc index 695ae025c97f8..16d19df81ce07 100644 --- a/mobile/library/common/jni/jni_utility.cc +++ b/mobile/library/common/jni/jni_utility.cc @@ -66,7 +66,7 @@ void jni_delete_const_global_ref(const void* context) { jni_delete_global_ref(const_cast(context)); } -bool check_for_exception(JNIEnv *env) { +bool check_exception(JNIEnv *env) { if (env->ExceptionCheck() == JNI_TRUE) { env->ExceptionDescribe(); env->ExceptionClear(); @@ -289,13 +289,13 @@ envoy_map to_native_map(JNIEnv* env, jobjectArray entries) { return native_map; } -jobjectArray ToJavaArrayOfJObjects(JNIEnv* env, envoy_map map) { +jobjectArray ToJavaArrayOfJObjects(JNIEnv* env, const ManagedEnvoyHeaders& map) { jclass jcls_byte_array = env->FindClass("java/lang/Object"); - jobjectArray javaArray = env->NewObjectArray(2 * map.length, jcls_byte_array, nullptr); + jobjectArray javaArray = env->NewObjectArray(2 * map.get().length, jcls_byte_array, nullptr); - for (envoy_map_size_t i = 0; i < map.length; i++) { - jbyteArray key = native_data_to_array(env, map.entries[i].key); - jbyteArray value = native_data_to_array(env, map.entries[i].value); + for (envoy_map_size_t i = 0; i < map.get().length; i++) { + jbyteArray key = native_data_to_array(env, map.get().entries[i].key); + jbyteArray value = native_data_to_array(env, map.get().entries[i].value); env->SetObjectArrayElement(javaArray, 2 * i, key); env->SetObjectArrayElement(javaArray, 2 * i + 1, value); diff --git a/mobile/library/common/jni/jni_utility.h b/mobile/library/common/jni/jni_utility.h index d12a9b7907e8e..73a2e198206d6 100644 --- a/mobile/library/common/jni/jni_utility.h +++ b/mobile/library/common/jni/jni_utility.h @@ -5,6 +5,7 @@ #include "library/common/jni/import/jni_import.h" #include "library/common/types/c_types.h" +#include "library/common/types/managed_envoy_headers.h" // NOLINT(namespace-envoy) @@ -50,7 +51,7 @@ void jni_delete_const_global_ref(const void* context); * * @return Whether there was a pending JNI exception. */ -bool check_for_exception(JNIEnv *env); +bool check_exception(JNIEnv *env); int unbox_integer(JNIEnv* env, jobject boxedInteger); @@ -109,7 +110,7 @@ jbyteArray ToJavaByteArray(JNIEnv* env, const uint8_t* bytes, size_t len); jbyteArray ToJavaByteArray(JNIEnv* env, const std::string& str); -jobjectArray ToJavaArrayOfJObjects(JNIEnv* env, envoy_map map); +jobjectArray ToJavaArrayOfJObjects(JNIEnv* env, const ManagedEnvoyHeaders& map); void JavaArrayOfByteArrayToStringVector(JNIEnv* env, jobjectArray array, std::vector* out); diff --git a/mobile/library/common/types/BUILD b/mobile/library/common/types/BUILD index e4a61c2cdbb7b..651c3ede1631c 100644 --- a/mobile/library/common/types/BUILD +++ b/mobile/library/common/types/BUILD @@ -18,3 +18,15 @@ envoy_cc_library( "@envoy//source/common/common:assert_lib", ], ) + +envoy_cc_library( + name = "managed_envoy_headers_lib", + srcs = [ + "managed_envoy_headers.h", + ], + repository = "@envoy", + visibility = ["//visibility:public"], + deps = [ + "//library/common/types:c_types_lib", + ], +) diff --git a/mobile/library/common/types/managed_envoy_headers.h b/mobile/library/common/types/managed_envoy_headers.h new file mode 100644 index 0000000000000..5ea119b80c605 --- /dev/null +++ b/mobile/library/common/types/managed_envoy_headers.h @@ -0,0 +1,14 @@ +#pragma once + +#include "library/common/types/c_types.h" + +class ManagedEnvoyHeaders { + public: + ManagedEnvoyHeaders(envoy_headers headers): headers_(headers) {}; + ~ManagedEnvoyHeaders() { release_envoy_headers(headers_); } + const envoy_headers get() const { return headers_; } + private: + envoy_headers headers_; + ManagedEnvoyHeaders(const ManagedEnvoyHeaders&); + ManagedEnvoyHeaders& operator=(const ManagedEnvoyHeaders&); +}; \ No newline at end of file From 0ef0bd82a55e964ca1bdce213cc6c9e7831f7b56 Mon Sep 17 00:00:00 2001 From: Rafal Augustyniak Date: Mon, 5 Dec 2022 10:18:07 -0500 Subject: [PATCH 03/20] add test Signed-off-by: Rafal Augustyniak --- mobile/library/common/jni/BUILD | 8 +- mobile/library/common/jni/jni_interface.cc | 24 ++-- mobile/library/common/jni/jni_utility.cc | 2 +- mobile/library/common/jni/jni_utility.h | 2 +- mobile/library/common/types/BUILD | 2 +- .../common/types/managed_envoy_headers.h | 29 ++-- mobile/test/kotlin/integration/BUILD | 23 ++- .../FilterThrowingExceptionTest.kt | 135 ++++++++++++++++++ 8 files changed, 195 insertions(+), 30 deletions(-) create mode 100644 mobile/test/kotlin/integration/FilterThrowingExceptionTest.kt diff --git a/mobile/library/common/jni/BUILD b/mobile/library/common/jni/BUILD index 608c0a5f273dd..caaa9cc616dde 100644 --- a/mobile/library/common/jni/BUILD +++ b/mobile/library/common/jni/BUILD @@ -21,7 +21,7 @@ cc_library( deps = [ "//library/common/jni/import:jni_import_lib", "//library/common/types:c_types_lib", - "//library/common/types:managed_envoy_headers_lib", + "//library/common/types:managed_types_lib", "@envoy//source/common/common:assert_lib", ], ) @@ -49,7 +49,7 @@ cc_library( ":ndk_jni_support", "//library/common:envoy_main_interface_lib", "//library/common/api:c_types", - "//library/common/types:managed_envoy_headers_lib", + "//library/common/types:managed_types_lib", ], # We need this to ensure that we link this into the .so even though there are no code references. alwayslink = True, @@ -117,7 +117,7 @@ cc_binary( "//library/common:envoy_main_interface_lib", "//library/common/api:c_types", "//library/common/jni/import:jni_import_lib", - "//library/common/types:managed_envoy_headers_lib", + "//library/common/types:managed_types_lib", ], ) @@ -172,7 +172,7 @@ cc_library( "//library/common/jni/import:jni_import_lib", "//library/common:envoy_main_interface_lib", "//library/common/types:c_types_lib", - "//library/common/types:managed_envoy_headers_lib", + "//library/common/types:managed_types_lib", "@envoy//source/common/common:assert_lib", ] + TEST_EXTENSIONS, ) diff --git a/mobile/library/common/jni/jni_interface.cc b/mobile/library/common/jni/jni_interface.cc index 587430c03994c..4b765096ffb56 100644 --- a/mobile/library/common/jni/jni_interface.cc +++ b/mobile/library/common/jni/jni_interface.cc @@ -329,7 +329,9 @@ static void* jvm_on_headers(const char* method, const ManagedEnvoyHeaders& heade // TODO: make this cast safer. jobject result = env->CallObjectMethod(j_context, jmid_onHeaders, (jlong)headers.get().length, end_stream ? JNI_TRUE : JNI_FALSE, j_stream_intel); - if (check_exception(env)) { + // TODO(Augustyniak): Pass the name of the filter in here so that we can instrument the origin of the + // JNI exception better. + if (exception_check(env)) { env->DeleteLocalRef(j_stream_intel); env->DeleteLocalRef(jcls_JvmCallbackContext); @@ -339,8 +341,11 @@ static void* jvm_on_headers(const char* method, const ManagedEnvoyHeaders& heade jclass jcls_int = env->FindClass("java/lang/Integer"); jmethodID jmid_intInit = env->GetMethodID(jcls_int, "", "(I)V"); jobject status = env->NewObject(jcls_int, jmid_intInit, 0); + // Set status to "0" (FilterHeadersStatus::Continue). Signal that the intent + // is to continue the iteration of the filter chain. env->SetObjectArrayElement(result, 0, status); + // Since the "on headers" call threw an exception set input headers as output headers. env->SetObjectArrayElement(result, 1, ToJavaArrayOfJObjects(env, headers)); env->DeleteLocalRef(jcls_object_array); @@ -515,6 +520,7 @@ static void* jvm_on_trailers(const char* method, envoy_headers trailers, jlongArray j_stream_intel = native_stream_intel_to_array(env, stream_intel); // Note: be careful of JVM types. Before we casted to jlong we were getting integer problems. // TODO: make this cast safer. + // TODO(Augustyniak): check for pending exceptions after returning from JNI call. jobject result = env->CallObjectMethod(j_context, jmid_onTrailers, (jlong)trailers.length, j_stream_intel); @@ -735,10 +741,8 @@ static void* call_jvm_on_complete(envoy_stream_intel stream_intel, jlongArray j_final_stream_intel = native_final_stream_intel_to_array(env, final_stream_intel); jobject result = env->CallObjectMethod(j_context, jmid_onComplete, j_stream_intel, j_final_stream_intel); - if (env->ExceptionCheck() == JNI_TRUE) { - env->ExceptionDescribe(); - env->ExceptionClear(); - } + + exception_check(env); env->DeleteLocalRef(j_stream_intel); env->DeleteLocalRef(j_final_stream_intel); @@ -762,10 +766,7 @@ static void* call_jvm_on_error(envoy_error error, envoy_stream_intel stream_inte jobject result = env->CallObjectMethod(j_context, jmid_onError, error.error_code, j_error_message, error.attempt_count, j_stream_intel, j_final_stream_intel); - if (env->ExceptionCheck() == JNI_TRUE) { - env->ExceptionDescribe(); - env->ExceptionClear(); - } + exception_check(env); env->DeleteLocalRef(j_stream_intel); env->DeleteLocalRef(j_final_stream_intel); @@ -798,10 +799,7 @@ static void* call_jvm_on_cancel(envoy_stream_intel stream_intel, jobject result = env->CallObjectMethod(j_context, jmid_onCancel, j_stream_intel, j_final_stream_intel); - if (env->ExceptionCheck() == JNI_TRUE) { - env->ExceptionDescribe(); - env->ExceptionClear(); - } + exception_check(env); env->DeleteLocalRef(j_stream_intel); env->DeleteLocalRef(j_final_stream_intel); diff --git a/mobile/library/common/jni/jni_utility.cc b/mobile/library/common/jni/jni_utility.cc index 16d19df81ce07..96f7931a2aa15 100644 --- a/mobile/library/common/jni/jni_utility.cc +++ b/mobile/library/common/jni/jni_utility.cc @@ -66,7 +66,7 @@ void jni_delete_const_global_ref(const void* context) { jni_delete_global_ref(const_cast(context)); } -bool check_exception(JNIEnv *env) { +bool exception_check(JNIEnv *env) { if (env->ExceptionCheck() == JNI_TRUE) { env->ExceptionDescribe(); env->ExceptionClear(); diff --git a/mobile/library/common/jni/jni_utility.h b/mobile/library/common/jni/jni_utility.h index 73a2e198206d6..69478eaa99708 100644 --- a/mobile/library/common/jni/jni_utility.h +++ b/mobile/library/common/jni/jni_utility.h @@ -51,7 +51,7 @@ void jni_delete_const_global_ref(const void* context); * * @return Whether there was a pending JNI exception. */ -bool check_exception(JNIEnv *env); +bool exception_check(JNIEnv *env); int unbox_integer(JNIEnv* env, jobject boxedInteger); diff --git a/mobile/library/common/types/BUILD b/mobile/library/common/types/BUILD index 651c3ede1631c..3d1c6b8fbf5f6 100644 --- a/mobile/library/common/types/BUILD +++ b/mobile/library/common/types/BUILD @@ -20,7 +20,7 @@ envoy_cc_library( ) envoy_cc_library( - name = "managed_envoy_headers_lib", + name = "managed_types_lib", srcs = [ "managed_envoy_headers.h", ], diff --git a/mobile/library/common/types/managed_envoy_headers.h b/mobile/library/common/types/managed_envoy_headers.h index 5ea119b80c605..279385ca26765 100644 --- a/mobile/library/common/types/managed_envoy_headers.h +++ b/mobile/library/common/types/managed_envoy_headers.h @@ -2,13 +2,24 @@ #include "library/common/types/c_types.h" +/** + * A wrapper around envoy_headers that's responsible for freeing + * the underlying headers when they are not needed anymore. + */ class ManagedEnvoyHeaders { - public: - ManagedEnvoyHeaders(envoy_headers headers): headers_(headers) {}; - ~ManagedEnvoyHeaders() { release_envoy_headers(headers_); } - const envoy_headers get() const { return headers_; } - private: - envoy_headers headers_; - ManagedEnvoyHeaders(const ManagedEnvoyHeaders&); - ManagedEnvoyHeaders& operator=(const ManagedEnvoyHeaders&); -}; \ No newline at end of file + public: + /** + * Initialize a new instance of the receiver using a given instance of envoy headers. + * + * @param headers, that should be wrapped by the receiver. The wrapper will hold onto + * the passed headers and free them once the receiver is not used anymore. + */ + ManagedEnvoyHeaders(envoy_headers headers): headers_(headers) {}; + ~ManagedEnvoyHeaders() { release_envoy_headers(headers_); } + const envoy_headers& get() const { return headers_; } + private: + envoy_headers headers_; + // Make copy and assignment operators private to prevent copying of the receiver. + ManagedEnvoyHeaders(const ManagedEnvoyHeaders&); + ManagedEnvoyHeaders& operator=(const ManagedEnvoyHeaders&); +}; diff --git a/mobile/test/kotlin/integration/BUILD b/mobile/test/kotlin/integration/BUILD index a6ce4f629f006..3325a82f6c97c 100644 --- a/mobile/test/kotlin/integration/BUILD +++ b/mobile/test/kotlin/integration/BUILD @@ -1,4 +1,5 @@ -load("@envoy_mobile//bazel:kotlin_test.bzl", "envoy_mobile_jni_kt_test") +load("@envoy_mobile//bazel:kotlin_test.bzl", "envoy_mobile_android_test", "envoy_mobile_jni_kt_test") +load("@envoy_mobile//bazel:kotlin_lib.bzl", "envoy_mobile_kt_library") envoy_mobile_jni_kt_test( name = "engine_start_test", @@ -200,3 +201,23 @@ envoy_mobile_jni_kt_test( "//library/kotlin/io/envoyproxy/envoymobile:envoy_interfaces_lib", ], ) + +envoy_mobile_android_test( + name = "filter_throwing_exception_test", + srcs = [ + "FilterThrowingExceptionTest.kt", + ], + exec_properties = { + # TODO(lfpino): Remove this once the sandboxNetwork=off works for ipv4 localhost addresses. + "sandboxNetwork": "standard", + "dockerNetwork": "standard", + }, + native_deps = [ + "//library/common/jni:libndk_envoy_jni.so", + "//library/common/jni:libndk_envoy_jni.jnilib", + ], + deps = [ + "//library/kotlin/io/envoyproxy/envoymobile:envoy_interfaces_lib", + "//library/kotlin/io/envoyproxy/envoymobile:envoy_lib", + ], +) diff --git a/mobile/test/kotlin/integration/FilterThrowingExceptionTest.kt b/mobile/test/kotlin/integration/FilterThrowingExceptionTest.kt new file mode 100644 index 0000000000000..63e5f7f5a64fb --- /dev/null +++ b/mobile/test/kotlin/integration/FilterThrowingExceptionTest.kt @@ -0,0 +1,135 @@ +package test.kotlin.integration + +import android.content.Context +import androidx.test.core.app.ApplicationProvider + +import io.envoyproxy.envoymobile.AndroidEngineBuilder +import io.envoyproxy.envoymobile.EnvoyError +import io.envoyproxy.envoymobile.Engine +import io.envoyproxy.envoymobile.engine.JniLibrary +import io.envoyproxy.envoymobile.FilterDataStatus +import io.envoyproxy.envoymobile.FilterHeadersStatus +import io.envoyproxy.envoymobile.FilterTrailersStatus +import io.envoyproxy.envoymobile.FinalStreamIntel +import io.envoyproxy.envoymobile.LogLevel +import io.envoyproxy.envoymobile.RequestFilter +import io.envoyproxy.envoymobile.RequestHeaders +import io.envoyproxy.envoymobile.RequestHeadersBuilder +import io.envoyproxy.envoymobile.RequestMethod +import io.envoyproxy.envoymobile.RequestTrailers +import io.envoyproxy.envoymobile.ResponseFilter +import io.envoyproxy.envoymobile.ResponseHeaders +import io.envoyproxy.envoymobile.ResponseTrailers +import io.envoyproxy.envoymobile.StreamIntel + +import java.nio.ByteBuffer +import java.util.concurrent.CountDownLatch +import java.util.concurrent.Executors +import java.util.concurrent.TimeUnit + +import org.assertj.core.api.Assertions.assertThat +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner + +class ThrowingFilter: RequestFilter, ResponseFilter { + override fun onRequestHeaders( + headers: RequestHeaders, + endStream: Boolean, + streamIntel: StreamIntel + ): FilterHeadersStatus { + throw Exception("Demo Filter simulated onRequestHeaders exception") + } + + override fun onRequestData(body: ByteBuffer, endStream: Boolean, streamIntel: StreamIntel): + FilterDataStatus { + return FilterDataStatus.Continue(body) + } + + override fun onRequestTrailers(trailers: RequestTrailers, streamIntel: StreamIntel): + FilterTrailersStatus { + return FilterTrailersStatus.Continue(trailers) + } + + override fun onResponseHeaders( + headers: ResponseHeaders, + endStream: Boolean, + streamIntel: StreamIntel + ): FilterHeadersStatus { + throw Exception("Demo Filter simulated onResponseHeaders exception") + } + + override fun onResponseData( + body: ByteBuffer, + endStream: Boolean, + streamIntel: StreamIntel + ): FilterDataStatus { + return FilterDataStatus.Continue(body) + } + + override fun onResponseTrailers( + trailers: ResponseTrailers, + streamIntel: StreamIntel + ): FilterTrailersStatus { + return FilterTrailersStatus.Continue(trailers) + } + + override fun onError( + error: EnvoyError, + finalStreamIntel: FinalStreamIntel + ) {} + + override fun onCancel(finalStreamIntel: FinalStreamIntel) {} + + override fun onComplete(finalStreamIntel: FinalStreamIntel) { + throw Exception("Demo Filter simulated onComplete exception") + } +} + +@RunWith(RobolectricTestRunner::class) +class PerformHTTPRequestUsingProxy { + init { + JniLibrary.loadTestLibrary() + } + + @Test + fun `registers a filter that throws an exception and performs an HTTP request`() { + val onEngineRunningLatch = CountDownLatch(1) + val onRespondeHeadersLatch = CountDownLatch(1) + + val context = ApplicationProvider.getApplicationContext() + val builder = AndroidEngineBuilder(context) + val engine = builder + .addLogLevel(LogLevel.DEBUG) + .addPlatformFilter(::ThrowingFilter) + .setOnEngineRunning { onEngineRunningLatch.countDown() } + .build() + + onEngineRunningLatch.await(10, TimeUnit.SECONDS) + assertThat(onEngineRunningLatch.count).isEqualTo(0) + + val requestHeaders = RequestHeadersBuilder( + method = RequestMethod.GET, + scheme = "https", + authority = "api.lyft.com", + path = "/ping" + ) + .build() + + engine + .streamClient() + .newStreamPrototype() + .setOnResponseHeaders { responseHeaders, _, _ -> + val status = responseHeaders.httpStatus ?: 0L + assertThat(status).isEqualTo(200) + onRespondeHeadersLatch.countDown() + } + .start(Executors.newSingleThreadExecutor()) + .sendHeaders(requestHeaders, true) + + onRespondeHeadersLatch.await(15, TimeUnit.SECONDS) + assertThat(onRespondeHeadersLatch.count).isEqualTo(0) + + engine.terminate() + } +} From 7f4f3022d4a63513bed7a94c37044c35b8618ee4 Mon Sep 17 00:00:00 2001 From: Rafal Augustyniak Date: Mon, 5 Dec 2022 12:31:24 -0500 Subject: [PATCH 04/20] improve tests Signed-off-by: Rafal Augustyniak --- mobile/library/common/jni/jni_interface.cc | 3 ++- mobile/library/common/jni/jni_utility.cc | 15 +++++++++-- mobile/library/common/jni/jni_utility.h | 2 +- .../FilterThrowingExceptionTest.kt | 26 ++++++++++++++----- 4 files changed, 36 insertions(+), 10 deletions(-) diff --git a/mobile/library/common/jni/jni_interface.cc b/mobile/library/common/jni/jni_interface.cc index 4b765096ffb56..2403289f2add4 100644 --- a/mobile/library/common/jni/jni_interface.cc +++ b/mobile/library/common/jni/jni_interface.cc @@ -346,7 +346,7 @@ static void* jvm_on_headers(const char* method, const ManagedEnvoyHeaders& heade env->SetObjectArrayElement(result, 0, status); // Since the "on headers" call threw an exception set input headers as output headers. - env->SetObjectArrayElement(result, 1, ToJavaArrayOfJObjects(env, headers)); + env->SetObjectArrayElement(result, 1, ToJavaArrayOfObjectArray(env, headers)); env->DeleteLocalRef(jcls_object_array); env->DeleteLocalRef(jcls_int); @@ -766,6 +766,7 @@ static void* call_jvm_on_error(envoy_error error, envoy_stream_intel stream_inte jobject result = env->CallObjectMethod(j_context, jmid_onError, error.error_code, j_error_message, error.attempt_count, j_stream_intel, j_final_stream_intel); + exception_check(env); env->DeleteLocalRef(j_stream_intel); diff --git a/mobile/library/common/jni/jni_utility.cc b/mobile/library/common/jni/jni_utility.cc index 96f7931a2aa15..c62b08a2463db 100644 --- a/mobile/library/common/jni/jni_utility.cc +++ b/mobile/library/common/jni/jni_utility.cc @@ -68,9 +68,20 @@ void jni_delete_const_global_ref(const void* context) { bool exception_check(JNIEnv *env) { if (env->ExceptionCheck() == JNI_TRUE) { + jthrowable exception = env->ExceptionOccurred(); env->ExceptionDescribe(); env->ExceptionClear(); - ENVOY_LOG_EVENT_TO_LOGGER(GET_MISC_LOGGER(), error, "jni_exception", ""); + + jclass jcls_throwable = env->FindClass("java/lang/Throwable"); + jmethodID jmid_getMessage = env->GetMethodID(jcls_throwable, "getMessage", "()Ljava/lang/String;"); + jstring jobj_exception_message = (jstring)env->CallObjectMethod(exception, jmid_getMessage); + const char* exception_message = env->GetStringUTFChars(jobj_exception_message, nullptr); + + ENVOY_LOG_EVENT_TO_LOGGER(GET_MISC_LOGGER(), error, "jni_exception", std::string(exception_message)); + + env->ReleaseStringUTFChars(jobj_exception_message, exception_message); + env->DeleteLocalRef(jcls_throwable); + return true; } else { return false; @@ -289,7 +300,7 @@ envoy_map to_native_map(JNIEnv* env, jobjectArray entries) { return native_map; } -jobjectArray ToJavaArrayOfJObjects(JNIEnv* env, const ManagedEnvoyHeaders& map) { +jobjectArray ToJavaArrayOfObjectArray(JNIEnv* env, const ManagedEnvoyHeaders& map) { jclass jcls_byte_array = env->FindClass("java/lang/Object"); jobjectArray javaArray = env->NewObjectArray(2 * map.get().length, jcls_byte_array, nullptr); diff --git a/mobile/library/common/jni/jni_utility.h b/mobile/library/common/jni/jni_utility.h index 69478eaa99708..58546df0587fb 100644 --- a/mobile/library/common/jni/jni_utility.h +++ b/mobile/library/common/jni/jni_utility.h @@ -110,7 +110,7 @@ jbyteArray ToJavaByteArray(JNIEnv* env, const uint8_t* bytes, size_t len); jbyteArray ToJavaByteArray(JNIEnv* env, const std::string& str); -jobjectArray ToJavaArrayOfJObjects(JNIEnv* env, const ManagedEnvoyHeaders& map); +jobjectArray ToJavaArrayOfObjectArray(JNIEnv* env, const ManagedEnvoyHeaders& map); void JavaArrayOfByteArrayToStringVector(JNIEnv* env, jobjectArray array, std::vector* out); diff --git a/mobile/test/kotlin/integration/FilterThrowingExceptionTest.kt b/mobile/test/kotlin/integration/FilterThrowingExceptionTest.kt index 63e5f7f5a64fb..7d0fb3e08e83e 100644 --- a/mobile/test/kotlin/integration/FilterThrowingExceptionTest.kt +++ b/mobile/test/kotlin/integration/FilterThrowingExceptionTest.kt @@ -28,6 +28,7 @@ import java.util.concurrent.Executors import java.util.concurrent.TimeUnit import org.assertj.core.api.Assertions.assertThat +import org.junit.Assert.assertNotNull import org.junit.Test import org.junit.runner.RunWith import org.robolectric.RobolectricTestRunner @@ -38,7 +39,7 @@ class ThrowingFilter: RequestFilter, ResponseFilter { endStream: Boolean, streamIntel: StreamIntel ): FilterHeadersStatus { - throw Exception("Demo Filter simulated onRequestHeaders exception") + throw Exception("Simulated onRequestHeaders exception") } override fun onRequestData(body: ByteBuffer, endStream: Boolean, streamIntel: StreamIntel): @@ -56,7 +57,7 @@ class ThrowingFilter: RequestFilter, ResponseFilter { endStream: Boolean, streamIntel: StreamIntel ): FilterHeadersStatus { - throw Exception("Demo Filter simulated onResponseHeaders exception") + throw Exception("Simulated onResponseHeaders exception") } override fun onResponseData( @@ -81,9 +82,7 @@ class ThrowingFilter: RequestFilter, ResponseFilter { override fun onCancel(finalStreamIntel: FinalStreamIntel) {} - override fun onComplete(finalStreamIntel: FinalStreamIntel) { - throw Exception("Demo Filter simulated onComplete exception") - } + override fun onComplete(finalStreamIntel: FinalStreamIntel) {} } @RunWith(RobolectricTestRunner::class) @@ -96,12 +95,25 @@ class PerformHTTPRequestUsingProxy { fun `registers a filter that throws an exception and performs an HTTP request`() { val onEngineRunningLatch = CountDownLatch(1) val onRespondeHeadersLatch = CountDownLatch(1) + val onExceptionEventLatch = CountDownLatch(2) + + val expectedMessages = mutableListOf( + "Simulated onRequestHeaders exception", + "Simulated onResponseHeaders exception", + ) val context = ApplicationProvider.getApplicationContext() val builder = AndroidEngineBuilder(context) val engine = builder .addLogLevel(LogLevel.DEBUG) .addPlatformFilter(::ThrowingFilter) + .setEventTracker { event -> + if (event["name"] == "event_log" && event["log_name"] == "jni_exception") { + assertThat(event["message"]).isEqualTo(expectedMessages.first()) + expectedMessages.removeAt(0) + onExceptionEventLatch.countDown() + } + } .setOnEngineRunning { onEngineRunningLatch.countDown() } .build() @@ -127,7 +139,9 @@ class PerformHTTPRequestUsingProxy { .start(Executors.newSingleThreadExecutor()) .sendHeaders(requestHeaders, true) - onRespondeHeadersLatch.await(15, TimeUnit.SECONDS) + onExceptionEventLatch.await(15, TimeUnit.SECONDS) + assertThat(onExceptionEventLatch.count).isEqualTo(0) + onRespondeHeadersLatch.await(5, TimeUnit.SECONDS) assertThat(onRespondeHeadersLatch.count).isEqualTo(0) engine.terminate() From 7e45442aaeab984ee0bc77cb42ac83fa6cca89b5 Mon Sep 17 00:00:00 2001 From: Rafal Augustyniak Date: Mon, 5 Dec 2022 12:34:06 -0500 Subject: [PATCH 05/20] rename Signed-off-by: Rafal Augustyniak --- mobile/test/kotlin/integration/FilterThrowingExceptionTest.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mobile/test/kotlin/integration/FilterThrowingExceptionTest.kt b/mobile/test/kotlin/integration/FilterThrowingExceptionTest.kt index 7d0fb3e08e83e..3d90bb2db0dda 100644 --- a/mobile/test/kotlin/integration/FilterThrowingExceptionTest.kt +++ b/mobile/test/kotlin/integration/FilterThrowingExceptionTest.kt @@ -86,7 +86,7 @@ class ThrowingFilter: RequestFilter, ResponseFilter { } @RunWith(RobolectricTestRunner::class) -class PerformHTTPRequestUsingProxy { +class FilterThrowingExceptionTest { init { JniLibrary.loadTestLibrary() } From def2550822e1872d554d2acb9ae5fbc785caecf0 Mon Sep 17 00:00:00 2001 From: Rafal Augustyniak Date: Mon, 5 Dec 2022 12:51:17 -0500 Subject: [PATCH 06/20] Lint fixes Signed-off-by: Rafal Augustyniak --- mobile/library/common/jni/jni_interface.cc | 23 ++++++------ mobile/library/common/jni/jni_utility.cc | 10 +++-- mobile/library/common/jni/jni_utility.h | 12 +++--- .../common/types/managed_envoy_headers.h | 37 +++++++++++-------- 4 files changed, 46 insertions(+), 36 deletions(-) diff --git a/mobile/library/common/jni/jni_interface.cc b/mobile/library/common/jni/jni_interface.cc index 2403289f2add4..9580a6bb098fa 100644 --- a/mobile/library/common/jni/jni_interface.cc +++ b/mobile/library/common/jni/jni_interface.cc @@ -279,7 +279,8 @@ Java_io_envoyproxy_envoymobile_engine_JniLibrary_recordHistogramValue(JNIEnv* en // JvmCallbackContext -static void pass_headers(const char* method, const ManagedEnvoyHeaders& headers, jobject j_context) { +static void pass_headers(const char* method, const Envoy::Types::ManagedEnvoyHeaders& headers, + jobject j_context) { JNIEnv* env = get_env(); jclass jcls_JvmCallbackContext = env->GetObjectClass(j_context); jmethodID jmid_passHeader = env->GetMethodID(jcls_JvmCallbackContext, method, "([B[BZ)V"); @@ -313,8 +314,8 @@ static void pass_headers(const char* method, const ManagedEnvoyHeaders& headers, // These methods call jvm methods which means the local references created will not be // released automatically. Manual bookkeeping is required for these methods. -static void* jvm_on_headers(const char* method, const ManagedEnvoyHeaders& headers, bool end_stream, - envoy_stream_intel stream_intel, void* context) { +static void* jvm_on_headers(const char* method, const Envoy::Types::ManagedEnvoyHeaders& headers, + bool end_stream, envoy_stream_intel stream_intel, void* context) { jni_log("[Envoy]", "jvm_on_headers"); JNIEnv* env = get_env(); jobject j_context = static_cast(context); @@ -329,19 +330,19 @@ static void* jvm_on_headers(const char* method, const ManagedEnvoyHeaders& heade // TODO: make this cast safer. jobject result = env->CallObjectMethod(j_context, jmid_onHeaders, (jlong)headers.get().length, end_stream ? JNI_TRUE : JNI_FALSE, j_stream_intel); - // TODO(Augustyniak): Pass the name of the filter in here so that we can instrument the origin of the - // JNI exception better. + // TODO(Augustyniak): Pass the name of the filter in here so that we can instrument the origin of + // the JNI exception better. if (exception_check(env)) { env->DeleteLocalRef(j_stream_intel); env->DeleteLocalRef(jcls_JvmCallbackContext); jclass jcls_object_array = env->FindClass("java/lang/Object"); jobjectArray result = env->NewObjectArray(2, jcls_object_array, NULL); - + jclass jcls_int = env->FindClass("java/lang/Integer"); jmethodID jmid_intInit = env->GetMethodID(jcls_int, "", "(I)V"); jobject status = env->NewObject(jcls_int, jmid_intInit, 0); - // Set status to "0" (FilterHeadersStatus::Continue). Signal that the intent + // Set status to "0" (FilterHeadersStatus::Continue). Signal that the intent // is to continue the iteration of the filter chain. env->SetObjectArrayElement(result, 0, status); @@ -350,7 +351,7 @@ static void* jvm_on_headers(const char* method, const ManagedEnvoyHeaders& heade env->DeleteLocalRef(jcls_object_array); env->DeleteLocalRef(jcls_int); - + return result; } else { env->DeleteLocalRef(j_stream_intel); @@ -362,7 +363,7 @@ static void* jvm_on_headers(const char* method, const ManagedEnvoyHeaders& heade static void* jvm_on_response_headers(envoy_headers headers, bool end_stream, envoy_stream_intel stream_intel, void* context) { - const auto managed_headers = ManagedEnvoyHeaders(headers); + const auto managed_headers = Envoy::Types::ManagedEnvoyHeaders(headers); return jvm_on_headers("onResponseHeaders", managed_headers, end_stream, stream_intel, context); } @@ -370,7 +371,7 @@ static envoy_filter_headers_status jvm_http_filter_on_request_headers(envoy_headers input_headers, bool end_stream, envoy_stream_intel stream_intel, const void* context) { JNIEnv* env = get_env(); - const auto headers = ManagedEnvoyHeaders(input_headers); + const auto headers = Envoy::Types::ManagedEnvoyHeaders(input_headers); jobjectArray result = static_cast(jvm_on_headers( "onRequestHeaders", headers, end_stream, stream_intel, const_cast(context))); @@ -392,7 +393,7 @@ static envoy_filter_headers_status jvm_http_filter_on_response_headers(envoy_headers input_headers, bool end_stream, envoy_stream_intel stream_intel, const void* context) { JNIEnv* env = get_env(); - const auto headers = ManagedEnvoyHeaders(input_headers); + const auto headers = Envoy::Types::ManagedEnvoyHeaders(input_headers); jobjectArray result = static_cast(jvm_on_headers( "onResponseHeaders", headers, end_stream, stream_intel, const_cast(context))); diff --git a/mobile/library/common/jni/jni_utility.cc b/mobile/library/common/jni/jni_utility.cc index c62b08a2463db..81b552ab28a3c 100644 --- a/mobile/library/common/jni/jni_utility.cc +++ b/mobile/library/common/jni/jni_utility.cc @@ -66,18 +66,20 @@ void jni_delete_const_global_ref(const void* context) { jni_delete_global_ref(const_cast(context)); } -bool exception_check(JNIEnv *env) { +bool exception_check(JNIEnv* env) { if (env->ExceptionCheck() == JNI_TRUE) { jthrowable exception = env->ExceptionOccurred(); env->ExceptionDescribe(); env->ExceptionClear(); jclass jcls_throwable = env->FindClass("java/lang/Throwable"); - jmethodID jmid_getMessage = env->GetMethodID(jcls_throwable, "getMessage", "()Ljava/lang/String;"); + jmethodID jmid_getMessage = + env->GetMethodID(jcls_throwable, "getMessage", "()Ljava/lang/String;"); jstring jobj_exception_message = (jstring)env->CallObjectMethod(exception, jmid_getMessage); const char* exception_message = env->GetStringUTFChars(jobj_exception_message, nullptr); - ENVOY_LOG_EVENT_TO_LOGGER(GET_MISC_LOGGER(), error, "jni_exception", std::string(exception_message)); + ENVOY_LOG_EVENT_TO_LOGGER(GET_MISC_LOGGER(), error, "jni_exception", + std::string(exception_message)); env->ReleaseStringUTFChars(jobj_exception_message, exception_message); env->DeleteLocalRef(jcls_throwable); @@ -300,7 +302,7 @@ envoy_map to_native_map(JNIEnv* env, jobjectArray entries) { return native_map; } -jobjectArray ToJavaArrayOfObjectArray(JNIEnv* env, const ManagedEnvoyHeaders& map) { +jobjectArray ToJavaArrayOfObjectArray(JNIEnv* env, const Envoy::Types::ManagedEnvoyHeaders& map) { jclass jcls_byte_array = env->FindClass("java/lang/Object"); jobjectArray javaArray = env->NewObjectArray(2 * map.get().length, jcls_byte_array, nullptr); diff --git a/mobile/library/common/jni/jni_utility.h b/mobile/library/common/jni/jni_utility.h index 58546df0587fb..d13bcfdce934c 100644 --- a/mobile/library/common/jni/jni_utility.h +++ b/mobile/library/common/jni/jni_utility.h @@ -45,13 +45,13 @@ void jni_delete_global_ref(void* context); void jni_delete_const_global_ref(const void* context); /** - * Checks whether there is a pending JNI exception and clear it if necessary. - * + * Checks whether there is a pending JNI exception and clear it if necessary. + * * @param env, the JNI env pointer. - * - * @return Whether there was a pending JNI exception. + * + * @return Whether there was a pending JNI exception. */ -bool exception_check(JNIEnv *env); +bool exception_check(JNIEnv* env); int unbox_integer(JNIEnv* env, jobject boxedInteger); @@ -110,7 +110,7 @@ jbyteArray ToJavaByteArray(JNIEnv* env, const uint8_t* bytes, size_t len); jbyteArray ToJavaByteArray(JNIEnv* env, const std::string& str); -jobjectArray ToJavaArrayOfObjectArray(JNIEnv* env, const ManagedEnvoyHeaders& map); +jobjectArray ToJavaArrayOfObjectArray(JNIEnv* env, const Envoy::Types::ManagedEnvoyHeaders& map); void JavaArrayOfByteArrayToStringVector(JNIEnv* env, jobjectArray array, std::vector* out); diff --git a/mobile/library/common/types/managed_envoy_headers.h b/mobile/library/common/types/managed_envoy_headers.h index 279385ca26765..6cc8e780a42e3 100644 --- a/mobile/library/common/types/managed_envoy_headers.h +++ b/mobile/library/common/types/managed_envoy_headers.h @@ -2,24 +2,31 @@ #include "library/common/types/c_types.h" +namespace Envoy { +namespace Types { + /** * A wrapper around envoy_headers that's responsible for freeing * the underlying headers when they are not needed anymore. */ class ManagedEnvoyHeaders { - public: - /** - * Initialize a new instance of the receiver using a given instance of envoy headers. - * - * @param headers, that should be wrapped by the receiver. The wrapper will hold onto - * the passed headers and free them once the receiver is not used anymore. - */ - ManagedEnvoyHeaders(envoy_headers headers): headers_(headers) {}; - ~ManagedEnvoyHeaders() { release_envoy_headers(headers_); } - const envoy_headers& get() const { return headers_; } - private: - envoy_headers headers_; - // Make copy and assignment operators private to prevent copying of the receiver. - ManagedEnvoyHeaders(const ManagedEnvoyHeaders&); - ManagedEnvoyHeaders& operator=(const ManagedEnvoyHeaders&); +public: + /** + * Initialize a new instance of the receiver using a given instance of envoy headers. + * + * @param headers, that should be wrapped by the receiver. The wrapper will hold onto + * the passed headers and free them once the receiver is not used anymore. + */ + ManagedEnvoyHeaders(envoy_headers headers) : headers_(headers){}; + ~ManagedEnvoyHeaders() { release_envoy_headers(headers_); } + const envoy_headers& get() const { return headers_; } + +private: + envoy_headers headers_; + // Make copy and assignment operators private to prevent copying of the receiver. + ManagedEnvoyHeaders(const ManagedEnvoyHeaders&); + ManagedEnvoyHeaders& operator=(const ManagedEnvoyHeaders&); }; + +} // namespace Types +} // namespace Envoy \ No newline at end of file From b0730d04ef8d097a8a333de25703d4ae22572720 Mon Sep 17 00:00:00 2001 From: Rafal Augustyniak Date: Mon, 5 Dec 2022 13:04:47 -0500 Subject: [PATCH 07/20] potential fix Signed-off-by: Rafal Augustyniak --- mobile/library/common/jni/jni_utility.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/mobile/library/common/jni/jni_utility.cc b/mobile/library/common/jni/jni_utility.cc index 81b552ab28a3c..dc2c0a7644d24 100644 --- a/mobile/library/common/jni/jni_utility.cc +++ b/mobile/library/common/jni/jni_utility.cc @@ -69,7 +69,6 @@ void jni_delete_const_global_ref(const void* context) { bool exception_check(JNIEnv* env) { if (env->ExceptionCheck() == JNI_TRUE) { jthrowable exception = env->ExceptionOccurred(); - env->ExceptionDescribe(); env->ExceptionClear(); jclass jcls_throwable = env->FindClass("java/lang/Throwable"); From 7670d9d1d038595e9457b76f429f25d45a72f6e0 Mon Sep 17 00:00:00 2001 From: Rafal Augustyniak Date: Mon, 5 Dec 2022 14:29:41 -0500 Subject: [PATCH 08/20] change level Signed-off-by: Rafal Augustyniak --- mobile/library/common/jni/jni_utility.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mobile/library/common/jni/jni_utility.cc b/mobile/library/common/jni/jni_utility.cc index dc2c0a7644d24..797093888c66b 100644 --- a/mobile/library/common/jni/jni_utility.cc +++ b/mobile/library/common/jni/jni_utility.cc @@ -77,7 +77,7 @@ bool exception_check(JNIEnv* env) { jstring jobj_exception_message = (jstring)env->CallObjectMethod(exception, jmid_getMessage); const char* exception_message = env->GetStringUTFChars(jobj_exception_message, nullptr); - ENVOY_LOG_EVENT_TO_LOGGER(GET_MISC_LOGGER(), error, "jni_exception", + ENVOY_LOG_EVENT_TO_LOGGER(GET_MISC_LOGGER(), info, "jni_exception", std::string(exception_message)); env->ReleaseStringUTFChars(jobj_exception_message, exception_message); From e1965126b020d448e6414d71523e1704dadc8b2e Mon Sep 17 00:00:00 2001 From: Rafal Augustyniak Date: Mon, 5 Dec 2022 14:56:37 -0500 Subject: [PATCH 09/20] =?UTF-8?q?=E2=80=9C:hammer:=E2=80=9D?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rafal Augustyniak From 4f5137c936a5cd301dc8eee528d1fbaf460ac716 Mon Sep 17 00:00:00 2001 From: Rafal Augustyniak Date: Mon, 5 Dec 2022 15:20:51 -0500 Subject: [PATCH 10/20] attempt Signed-off-by: Rafal Augustyniak --- mobile/library/common/jni/jni_utility.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/mobile/library/common/jni/jni_utility.cc b/mobile/library/common/jni/jni_utility.cc index 797093888c66b..16b48d8938b6b 100644 --- a/mobile/library/common/jni/jni_utility.cc +++ b/mobile/library/common/jni/jni_utility.cc @@ -77,8 +77,8 @@ bool exception_check(JNIEnv* env) { jstring jobj_exception_message = (jstring)env->CallObjectMethod(exception, jmid_getMessage); const char* exception_message = env->GetStringUTFChars(jobj_exception_message, nullptr); - ENVOY_LOG_EVENT_TO_LOGGER(GET_MISC_LOGGER(), info, "jni_exception", - std::string(exception_message)); + ENVOY_LOG_EVENT_TO_LOGGER(Envoy::Logger::Registry::getLog(Envoy::Logger::Id::main), info, + "jni_exception", std::string(exception_message)); env->ReleaseStringUTFChars(jobj_exception_message, exception_message); env->DeleteLocalRef(jcls_throwable); From 00ba3641e7e95010d4d5c845a81a6224bae72a32 Mon Sep 17 00:00:00 2001 From: Rafal Augustyniak Date: Mon, 5 Dec 2022 16:15:37 -0500 Subject: [PATCH 11/20] disable Signed-off-by: Rafal Augustyniak --- mobile/library/common/jni/jni_utility.cc | 4 ++-- .../integration/FilterThrowingExceptionTest.kt | 14 +++++++------- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/mobile/library/common/jni/jni_utility.cc b/mobile/library/common/jni/jni_utility.cc index 16b48d8938b6b..789eb7ebc9952 100644 --- a/mobile/library/common/jni/jni_utility.cc +++ b/mobile/library/common/jni/jni_utility.cc @@ -77,8 +77,8 @@ bool exception_check(JNIEnv* env) { jstring jobj_exception_message = (jstring)env->CallObjectMethod(exception, jmid_getMessage); const char* exception_message = env->GetStringUTFChars(jobj_exception_message, nullptr); - ENVOY_LOG_EVENT_TO_LOGGER(Envoy::Logger::Registry::getLog(Envoy::Logger::Id::main), info, - "jni_exception", std::string(exception_message)); + // ENVOY_LOG_EVENT_TO_LOGGER(Envoy::Logger::Registry::getLog(Envoy::Logger::Id::main), info, + // "jni_exception", std::string(exception_message)); env->ReleaseStringUTFChars(jobj_exception_message, exception_message); env->DeleteLocalRef(jcls_throwable); diff --git a/mobile/test/kotlin/integration/FilterThrowingExceptionTest.kt b/mobile/test/kotlin/integration/FilterThrowingExceptionTest.kt index 3d90bb2db0dda..cab34580d5405 100644 --- a/mobile/test/kotlin/integration/FilterThrowingExceptionTest.kt +++ b/mobile/test/kotlin/integration/FilterThrowingExceptionTest.kt @@ -107,13 +107,13 @@ class FilterThrowingExceptionTest { val engine = builder .addLogLevel(LogLevel.DEBUG) .addPlatformFilter(::ThrowingFilter) - .setEventTracker { event -> - if (event["name"] == "event_log" && event["log_name"] == "jni_exception") { - assertThat(event["message"]).isEqualTo(expectedMessages.first()) - expectedMessages.removeAt(0) - onExceptionEventLatch.countDown() - } - } + // .setEventTracker { event -> + // if (event["name"] == "event_log" && event["log_name"] == "jni_exception") { + // assertThat(event["message"]).isEqualTo(expectedMessages.first()) + // expectedMessages.removeAt(0) + // onExceptionEventLatch.countDown() + // } + // } .setOnEngineRunning { onEngineRunningLatch.countDown() } .build() From 125460c322b6a21e03f2c1ebac2562632ae3f396 Mon Sep 17 00:00:00 2001 From: Rafal Augustyniak Date: Mon, 5 Dec 2022 16:29:15 -0500 Subject: [PATCH 12/20] disable Signed-off-by: Rafal Augustyniak --- .../test/kotlin/integration/FilterThrowingExceptionTest.kt | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/mobile/test/kotlin/integration/FilterThrowingExceptionTest.kt b/mobile/test/kotlin/integration/FilterThrowingExceptionTest.kt index cab34580d5405..435bd7ab49a9d 100644 --- a/mobile/test/kotlin/integration/FilterThrowingExceptionTest.kt +++ b/mobile/test/kotlin/integration/FilterThrowingExceptionTest.kt @@ -139,9 +139,9 @@ class FilterThrowingExceptionTest { .start(Executors.newSingleThreadExecutor()) .sendHeaders(requestHeaders, true) - onExceptionEventLatch.await(15, TimeUnit.SECONDS) - assertThat(onExceptionEventLatch.count).isEqualTo(0) - onRespondeHeadersLatch.await(5, TimeUnit.SECONDS) + // onExceptionEventLatch.await(15, TimeUnit.SECONDS) + // assertThat(onExceptionEventLatch.count).isEqualTo(0) + onRespondeHeadersLatch.await(15, TimeUnit.SECONDS) assertThat(onRespondeHeadersLatch.count).isEqualTo(0) engine.terminate() From 3ad5ae02fd701517064511d0a4f82108668b5f5f Mon Sep 17 00:00:00 2001 From: Rafal Augustyniak Date: Mon, 5 Dec 2022 17:01:07 -0500 Subject: [PATCH 13/20] remove logging as it was locking Signed-off-by: Rafal Augustyniak --- mobile/library/common/jni/jni_utility.cc | 15 +-------------- .../integration/FilterThrowingExceptionTest.kt | 10 ---------- 2 files changed, 1 insertion(+), 24 deletions(-) diff --git a/mobile/library/common/jni/jni_utility.cc b/mobile/library/common/jni/jni_utility.cc index 789eb7ebc9952..cd23dc5f63c38 100644 --- a/mobile/library/common/jni/jni_utility.cc +++ b/mobile/library/common/jni/jni_utility.cc @@ -68,21 +68,8 @@ void jni_delete_const_global_ref(const void* context) { bool exception_check(JNIEnv* env) { if (env->ExceptionCheck() == JNI_TRUE) { - jthrowable exception = env->ExceptionOccurred(); env->ExceptionClear(); - - jclass jcls_throwable = env->FindClass("java/lang/Throwable"); - jmethodID jmid_getMessage = - env->GetMethodID(jcls_throwable, "getMessage", "()Ljava/lang/String;"); - jstring jobj_exception_message = (jstring)env->CallObjectMethod(exception, jmid_getMessage); - const char* exception_message = env->GetStringUTFChars(jobj_exception_message, nullptr); - - // ENVOY_LOG_EVENT_TO_LOGGER(Envoy::Logger::Registry::getLog(Envoy::Logger::Id::main), info, - // "jni_exception", std::string(exception_message)); - - env->ReleaseStringUTFChars(jobj_exception_message, exception_message); - env->DeleteLocalRef(jcls_throwable); - + // TODO(Augustyniak): Log exception details. return true; } else { return false; diff --git a/mobile/test/kotlin/integration/FilterThrowingExceptionTest.kt b/mobile/test/kotlin/integration/FilterThrowingExceptionTest.kt index 435bd7ab49a9d..52655c6a702af 100644 --- a/mobile/test/kotlin/integration/FilterThrowingExceptionTest.kt +++ b/mobile/test/kotlin/integration/FilterThrowingExceptionTest.kt @@ -95,7 +95,6 @@ class FilterThrowingExceptionTest { fun `registers a filter that throws an exception and performs an HTTP request`() { val onEngineRunningLatch = CountDownLatch(1) val onRespondeHeadersLatch = CountDownLatch(1) - val onExceptionEventLatch = CountDownLatch(2) val expectedMessages = mutableListOf( "Simulated onRequestHeaders exception", @@ -107,13 +106,6 @@ class FilterThrowingExceptionTest { val engine = builder .addLogLevel(LogLevel.DEBUG) .addPlatformFilter(::ThrowingFilter) - // .setEventTracker { event -> - // if (event["name"] == "event_log" && event["log_name"] == "jni_exception") { - // assertThat(event["message"]).isEqualTo(expectedMessages.first()) - // expectedMessages.removeAt(0) - // onExceptionEventLatch.countDown() - // } - // } .setOnEngineRunning { onEngineRunningLatch.countDown() } .build() @@ -139,8 +131,6 @@ class FilterThrowingExceptionTest { .start(Executors.newSingleThreadExecutor()) .sendHeaders(requestHeaders, true) - // onExceptionEventLatch.await(15, TimeUnit.SECONDS) - // assertThat(onExceptionEventLatch.count).isEqualTo(0) onRespondeHeadersLatch.await(15, TimeUnit.SECONDS) assertThat(onRespondeHeadersLatch.count).isEqualTo(0) From ff3266d728d2cdc739c9c964aee04e01a3731984 Mon Sep 17 00:00:00 2001 From: Rafal Augustyniak Date: Mon, 5 Dec 2022 22:04:12 -0500 Subject: [PATCH 14/20] remove Signed-off-by: Rafal Augustyniak --- .../kotlin/hello_world/MainActivity.kt | 38 ++++++++++--------- 1 file changed, 20 insertions(+), 18 deletions(-) diff --git a/mobile/examples/kotlin/hello_world/MainActivity.kt b/mobile/examples/kotlin/hello_world/MainActivity.kt index 98234fc1d7a52..b6f8a02482903 100644 --- a/mobile/examples/kotlin/hello_world/MainActivity.kt +++ b/mobile/examples/kotlin/hello_world/MainActivity.kt @@ -52,8 +52,8 @@ class MainActivity : Activity() { .addLogLevel(LogLevel.DEBUG) .enableProxying(true) .addPlatformFilter(::DemoFilter) - .addPlatformFilter(::BufferDemoFilter) - .addPlatformFilter(::AsyncDemoFilter) +// .addPlatformFilter(::BufferDemoFilter) +// .addPlatformFilter(::AsyncDemoFilter) .addNativeFilter("envoy.filters.http.buffer", "{\"@type\":\"type.googleapis.com/envoy.extensions.filters.http.buffer.v3.Buffer\",\"max_request_bytes\":5242880}") .addStringAccessor("demo-accessor", { "PlatformString" }) .setOnEngineRunning { Log.d("MainActivity", "Envoy async internal setup completed") } @@ -79,23 +79,25 @@ class MainActivity : Activity() { thread.start() val handler = Handler(thread.looper) - // Run a request loop and record stats until the application exits. - handler.postDelayed( - object : Runnable { - override fun run() { - try { - makeRequest() - recordStats() - } catch (e: IOException) { - Log.d("MainActivity", "exception making request or recording stats", e) - } + makeRequest() - // Make a call and report stats again - handler.postDelayed(this, TimeUnit.SECONDS.toMillis(1)) - } - }, - TimeUnit.SECONDS.toMillis(1) - ) + // Run a request loop and record stats until the application exits. +// handler.postDelayed( +// object : Runnable { +// override fun run() { +// try { +// makeRequest() +// recordStats() +// } catch (e: IOException) { +// Log.d("MainActivity", "exception making request or recording stats", e) +// } +// +// // Make a call and report stats again +// handler.postDelayed(this, TimeUnit.SECONDS.toMillis(1)) +// } +// }, +// TimeUnit.SECONDS.toMillis(1) +// ) } override fun onDestroy() { From 57727d333285a9b8aafdb77b330526ae73f90e74 Mon Sep 17 00:00:00 2001 From: Rafal Augustyniak Date: Tue, 6 Dec 2022 08:13:08 -0500 Subject: [PATCH 15/20] fix pre-format Signed-off-by: Rafal Augustyniak --- mobile/library/common/types/managed_envoy_headers.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mobile/library/common/types/managed_envoy_headers.h b/mobile/library/common/types/managed_envoy_headers.h index 6cc8e780a42e3..2e2072eb7245e 100644 --- a/mobile/library/common/types/managed_envoy_headers.h +++ b/mobile/library/common/types/managed_envoy_headers.h @@ -29,4 +29,4 @@ class ManagedEnvoyHeaders { }; } // namespace Types -} // namespace Envoy \ No newline at end of file +} // namespace Envoy From 7ee1e64a89a8079fc41ef9b07f7f062373368247 Mon Sep 17 00:00:00 2001 From: Rafal Augustyniak Date: Tue, 6 Dec 2022 08:16:40 -0500 Subject: [PATCH 16/20] Revert "remove" This reverts commit 479b7f8936a9e50c25be20ed798347042ebcfda6. Signed-off-by: Rafal Augustyniak --- .../kotlin/hello_world/MainActivity.kt | 38 +++++++++---------- 1 file changed, 18 insertions(+), 20 deletions(-) diff --git a/mobile/examples/kotlin/hello_world/MainActivity.kt b/mobile/examples/kotlin/hello_world/MainActivity.kt index b6f8a02482903..98234fc1d7a52 100644 --- a/mobile/examples/kotlin/hello_world/MainActivity.kt +++ b/mobile/examples/kotlin/hello_world/MainActivity.kt @@ -52,8 +52,8 @@ class MainActivity : Activity() { .addLogLevel(LogLevel.DEBUG) .enableProxying(true) .addPlatformFilter(::DemoFilter) -// .addPlatformFilter(::BufferDemoFilter) -// .addPlatformFilter(::AsyncDemoFilter) + .addPlatformFilter(::BufferDemoFilter) + .addPlatformFilter(::AsyncDemoFilter) .addNativeFilter("envoy.filters.http.buffer", "{\"@type\":\"type.googleapis.com/envoy.extensions.filters.http.buffer.v3.Buffer\",\"max_request_bytes\":5242880}") .addStringAccessor("demo-accessor", { "PlatformString" }) .setOnEngineRunning { Log.d("MainActivity", "Envoy async internal setup completed") } @@ -79,25 +79,23 @@ class MainActivity : Activity() { thread.start() val handler = Handler(thread.looper) - makeRequest() - // Run a request loop and record stats until the application exits. -// handler.postDelayed( -// object : Runnable { -// override fun run() { -// try { -// makeRequest() -// recordStats() -// } catch (e: IOException) { -// Log.d("MainActivity", "exception making request or recording stats", e) -// } -// -// // Make a call and report stats again -// handler.postDelayed(this, TimeUnit.SECONDS.toMillis(1)) -// } -// }, -// TimeUnit.SECONDS.toMillis(1) -// ) + handler.postDelayed( + object : Runnable { + override fun run() { + try { + makeRequest() + recordStats() + } catch (e: IOException) { + Log.d("MainActivity", "exception making request or recording stats", e) + } + + // Make a call and report stats again + handler.postDelayed(this, TimeUnit.SECONDS.toMillis(1)) + } + }, + TimeUnit.SECONDS.toMillis(1) + ) } override fun onDestroy() { From f56661085ade02800fbad603c78ef3f20fe73418 Mon Sep 17 00:00:00 2001 From: Rafal Augustyniak Date: Tue, 6 Dec 2022 08:17:26 -0500 Subject: [PATCH 17/20] remove expectedMessages Signed-off-by: Rafal Augustyniak --- .../test/kotlin/integration/FilterThrowingExceptionTest.kt | 5 ----- 1 file changed, 5 deletions(-) diff --git a/mobile/test/kotlin/integration/FilterThrowingExceptionTest.kt b/mobile/test/kotlin/integration/FilterThrowingExceptionTest.kt index 52655c6a702af..aa6f993dadabf 100644 --- a/mobile/test/kotlin/integration/FilterThrowingExceptionTest.kt +++ b/mobile/test/kotlin/integration/FilterThrowingExceptionTest.kt @@ -96,11 +96,6 @@ class FilterThrowingExceptionTest { val onEngineRunningLatch = CountDownLatch(1) val onRespondeHeadersLatch = CountDownLatch(1) - val expectedMessages = mutableListOf( - "Simulated onRequestHeaders exception", - "Simulated onResponseHeaders exception", - ) - val context = ApplicationProvider.getApplicationContext() val builder = AndroidEngineBuilder(context) val engine = builder From 95217413e88377902fd9ca21abef0ee765da2cf5 Mon Sep 17 00:00:00 2001 From: Rafal Augustyniak Date: Tue, 6 Dec 2022 16:22:38 -0500 Subject: [PATCH 18/20] cr Signed-off-by: Rafal Augustyniak --- mobile/library/common/jni/jni_interface.cc | 53 ++++++++++++---------- mobile/library/common/jni/jni_utility.cc | 2 +- mobile/library/common/jni/jni_utility.h | 6 +-- 3 files changed, 32 insertions(+), 29 deletions(-) diff --git a/mobile/library/common/jni/jni_interface.cc b/mobile/library/common/jni/jni_interface.cc index 9580a6bb098fa..8ae80caa6aec9 100644 --- a/mobile/library/common/jni/jni_interface.cc +++ b/mobile/library/common/jni/jni_interface.cc @@ -329,36 +329,38 @@ static void* jvm_on_headers(const char* method, const Envoy::Types::ManagedEnvoy // Note: be careful of JVM types. Before we casted to jlong we were getting integer problems. // TODO: make this cast safer. jobject result = env->CallObjectMethod(j_context, jmid_onHeaders, (jlong)headers.get().length, - end_stream ? JNI_TRUE : JNI_FALSE, j_stream_intel); + end_stream ? JNI_TRUE : JNI_FALSE, j_stream_intel); // TODO(Augustyniak): Pass the name of the filter in here so that we can instrument the origin of // the JNI exception better. - if (exception_check(env)) { - env->DeleteLocalRef(j_stream_intel); - env->DeleteLocalRef(jcls_JvmCallbackContext); + bool exception_cleared = clear_pending_exceptions(env); - jclass jcls_object_array = env->FindClass("java/lang/Object"); - jobjectArray result = env->NewObjectArray(2, jcls_object_array, NULL); + env->DeleteLocalRef(j_stream_intel); + env->DeleteLocalRef(jcls_JvmCallbackContext); + + if (!exception_cleared) { + return result; + } - jclass jcls_int = env->FindClass("java/lang/Integer"); - jmethodID jmid_intInit = env->GetMethodID(jcls_int, "", "(I)V"); - jobject status = env->NewObject(jcls_int, jmid_intInit, 0); - // Set status to "0" (FilterHeadersStatus::Continue). Signal that the intent - // is to continue the iteration of the filter chain. - env->SetObjectArrayElement(result, 0, status); + // Create a "no operation" result: + // 1. Tell the filter chain to continue the iteration. + // 2. Return headers received on as method's input as part of the method's output. + jclass jcls_object_array = env->FindClass("java/lang/Object"); + jobjectArray noopResult = env->NewObjectArray(2, jcls_object_array, NULL); - // Since the "on headers" call threw an exception set input headers as output headers. - env->SetObjectArrayElement(result, 1, ToJavaArrayOfObjectArray(env, headers)); + jclass jcls_int = env->FindClass("java/lang/Integer"); + jmethodID jmid_intInit = env->GetMethodID(jcls_int, "", "(I)V"); + jobject j_status = env->NewObject(jcls_int, jmid_intInit, 0); + // Set status to "0" (FilterHeadersStatus::Continue). Signal that the intent + // is to continue the iteration of the filter chain. + env->SetObjectArrayElement(noopResult, 0, j_status); - env->DeleteLocalRef(jcls_object_array); - env->DeleteLocalRef(jcls_int); + // Since the "on headers" call threw an exception set input headers as output headers. + env->SetObjectArrayElement(noopResult, 1, ToJavaArrayOfObjectArray(env, headers)); - return result; - } else { - env->DeleteLocalRef(j_stream_intel); - env->DeleteLocalRef(jcls_JvmCallbackContext); + env->DeleteLocalRef(jcls_object_array); + env->DeleteLocalRef(jcls_int); - return result; - } + return noopResult; } static void* jvm_on_response_headers(envoy_headers headers, bool end_stream, @@ -743,7 +745,7 @@ static void* call_jvm_on_complete(envoy_stream_intel stream_intel, jobject result = env->CallObjectMethod(j_context, jmid_onComplete, j_stream_intel, j_final_stream_intel); - exception_check(env); + clear_pending_exceptions(env); env->DeleteLocalRef(j_stream_intel); env->DeleteLocalRef(j_final_stream_intel); @@ -768,7 +770,7 @@ static void* call_jvm_on_error(envoy_error error, envoy_stream_intel stream_inte jobject result = env->CallObjectMethod(j_context, jmid_onError, error.error_code, j_error_message, error.attempt_count, j_stream_intel, j_final_stream_intel); - exception_check(env); + clear_pending_exceptions(env); env->DeleteLocalRef(j_stream_intel); env->DeleteLocalRef(j_final_stream_intel); @@ -801,7 +803,8 @@ static void* call_jvm_on_cancel(envoy_stream_intel stream_intel, jobject result = env->CallObjectMethod(j_context, jmid_onCancel, j_stream_intel, j_final_stream_intel); - exception_check(env); + + clear_pending_exceptions(env); env->DeleteLocalRef(j_stream_intel); env->DeleteLocalRef(j_final_stream_intel); diff --git a/mobile/library/common/jni/jni_utility.cc b/mobile/library/common/jni/jni_utility.cc index cd23dc5f63c38..3a4debc698e61 100644 --- a/mobile/library/common/jni/jni_utility.cc +++ b/mobile/library/common/jni/jni_utility.cc @@ -66,7 +66,7 @@ void jni_delete_const_global_ref(const void* context) { jni_delete_global_ref(const_cast(context)); } -bool exception_check(JNIEnv* env) { +bool clear_pending_exceptions(JNIEnv* env) { if (env->ExceptionCheck() == JNI_TRUE) { env->ExceptionClear(); // TODO(Augustyniak): Log exception details. diff --git a/mobile/library/common/jni/jni_utility.h b/mobile/library/common/jni/jni_utility.h index d13bcfdce934c..572fee8a0dc32 100644 --- a/mobile/library/common/jni/jni_utility.h +++ b/mobile/library/common/jni/jni_utility.h @@ -45,13 +45,13 @@ void jni_delete_global_ref(void* context); void jni_delete_const_global_ref(const void* context); /** - * Checks whether there is a pending JNI exception and clear it if necessary. + * Clears any pending exceptions that may have been rides in result to a call into Java code. * * @param env, the JNI env pointer. * - * @return Whether there was a pending JNI exception. + * @return Whether any pending JNI exception was cleared. */ -bool exception_check(JNIEnv* env); +bool clear_pending_exceptions(JNIEnv* env); int unbox_integer(JNIEnv* env, jobject boxedInteger); From bda8425818086374f8208478a3b06ac4498b1bbb Mon Sep 17 00:00:00 2001 From: Rafal Augustyniak Date: Tue, 6 Dec 2022 16:37:35 -0500 Subject: [PATCH 19/20] lint fix Signed-off-by: Rafal Augustyniak --- mobile/library/common/jni/jni_interface.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mobile/library/common/jni/jni_interface.cc b/mobile/library/common/jni/jni_interface.cc index 8ae80caa6aec9..bc7cbca4b8a0f 100644 --- a/mobile/library/common/jni/jni_interface.cc +++ b/mobile/library/common/jni/jni_interface.cc @@ -329,7 +329,7 @@ static void* jvm_on_headers(const char* method, const Envoy::Types::ManagedEnvoy // Note: be careful of JVM types. Before we casted to jlong we were getting integer problems. // TODO: make this cast safer. jobject result = env->CallObjectMethod(j_context, jmid_onHeaders, (jlong)headers.get().length, - end_stream ? JNI_TRUE : JNI_FALSE, j_stream_intel); + end_stream ? JNI_TRUE : JNI_FALSE, j_stream_intel); // TODO(Augustyniak): Pass the name of the filter in here so that we can instrument the origin of // the JNI exception better. bool exception_cleared = clear_pending_exceptions(env); From b99b523e8bccdbf180581642e0390d7f79aa1ae1 Mon Sep 17 00:00:00 2001 From: Rafal Augustyniak Date: Tue, 6 Dec 2022 16:45:04 -0500 Subject: [PATCH 20/20] rename pass_headers Signed-off-by: Rafal Augustyniak --- mobile/library/common/jni/jni_interface.cc | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/mobile/library/common/jni/jni_interface.cc b/mobile/library/common/jni/jni_interface.cc index bc7cbca4b8a0f..8dded5a7a4f11 100644 --- a/mobile/library/common/jni/jni_interface.cc +++ b/mobile/library/common/jni/jni_interface.cc @@ -279,8 +279,8 @@ Java_io_envoyproxy_envoymobile_engine_JniLibrary_recordHistogramValue(JNIEnv* en // JvmCallbackContext -static void pass_headers(const char* method, const Envoy::Types::ManagedEnvoyHeaders& headers, - jobject j_context) { +static void passHeaders(const char* method, const Envoy::Types::ManagedEnvoyHeaders& headers, + jobject j_context) { JNIEnv* env = get_env(); jclass jcls_JvmCallbackContext = env->GetObjectClass(j_context); jmethodID jmid_passHeader = env->GetMethodID(jcls_JvmCallbackContext, method, "([B[BZ)V"); @@ -319,7 +319,7 @@ static void* jvm_on_headers(const char* method, const Envoy::Types::ManagedEnvoy jni_log("[Envoy]", "jvm_on_headers"); JNIEnv* env = get_env(); jobject j_context = static_cast(context); - pass_headers("passHeader", headers, j_context); + passHeaders("passHeader", headers, j_context); jclass jcls_JvmCallbackContext = env->GetObjectClass(j_context); jmethodID jmid_onHeaders = @@ -514,7 +514,7 @@ static void* jvm_on_trailers(const char* method, envoy_headers trailers, JNIEnv* env = get_env(); jobject j_context = static_cast(context); - pass_headers("passHeader", trailers, j_context); + passHeaders("passHeader", trailers, j_context); jclass jcls_JvmCallbackContext = env->GetObjectClass(j_context); jmethodID jmid_onTrailers = @@ -663,7 +663,7 @@ jvm_http_filter_on_resume(const char* method, envoy_headers* headers, envoy_data jlong headers_length = -1; if (headers) { headers_length = (jlong)headers->length; - pass_headers("passHeader", *headers, j_context); + passHeaders("passHeader", *headers, j_context); } jbyteArray j_in_data = nullptr; if (data) { @@ -672,7 +672,7 @@ jvm_http_filter_on_resume(const char* method, envoy_headers* headers, envoy_data jlong trailers_length = -1; if (trailers) { trailers_length = (jlong)trailers->length; - pass_headers("passTrailer", *trailers, j_context); + passHeaders("passTrailer", *trailers, j_context); } jlongArray j_stream_intel = native_stream_intel_to_array(env, stream_intel);