mobile: check for pending exceptions after JNI call by Augustyniak · Pull Request #24361 · envoyproxy/envoy

Augustyniak · 2022-12-05T17:33:14Z

Commit Message: Check for pending exceptions after some of the Java calls from within the JNI layer. At Lyft, we noticed that we have a bunch of crashes related to us not checking for exceptions after on_request_headers or/and on_response_headers methods.
Additional description: I tried to use ENVOY_LOG_EVENT_TO_LOGGER to log when we detect a pending exception but ran into issues with deadlocks. Got rid of the extra logging to unblock my change.
Risk Level: Low, mostly additive change. New code executes for when the library crashes previously.
Testing: Integration test
Docs Changes:
Release Notes:
Platform Specific Features:

repokitteh-read-only · 2022-12-05T17:33:26Z

CC @envoyproxy/mobile-maintainers: FYI only for changes made to (mobile/).
envoyproxy/mobile-maintainers assignee is @alyssawilk

🐱

Caused by: #24361 was opened by Augustyniak.

see: more, trace.

Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>

This reverts commit 479b7f8936a9e50c25be20ed798347042ebcfda6. Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>

Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>

alyssawilk

cool, thanks for tackling this!
/wait

alyssawilk · 2022-12-06T15:56:10Z

mobile/library/common/jni/jni_interface.cc

 // JvmCallbackContext

-static void pass_headers(const char* method, envoy_headers headers, jobject j_context) {
+static void pass_headers(const char* method, const Envoy::Types::ManagedEnvoyHeaders& headers,


I thought these all had to be simple (C) types. can you clue me in on why it's OK use ManagedEnvoyHeaders?

edit: nm I think I see why. if this isn't called from Java layers can we use standard passHeaders naming to make that clear?

I thought these all had to be simple (C) types. can you clue me in on why it's OK use ManagedEnvoyHeaders?

the usage of c types is a self-imposed limitations in many places in our code. Decided to use c++ type in here to make mem management simpler.

edit: nm I think I see why. if this isn't called from Java layers can we use standard passHeaders naming to make that clear?

OK

alyssawilk · 2022-12-06T15:58:38Z

mobile/library/common/jni/jni_interface.cc

+  // 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);


can the DeleteLocalRefs be moved outside of the exception check? alternately if the exception check needs to happen before deletion
bool saw_exception = exception_check(env)
deletelocalRef
deletelocalRef
if (saw_exception) {...

alyssawilk · 2022-12-06T15:59:11Z

mobile/library/common/jni/jni_interface.cc

+    env->DeleteLocalRef(j_stream_intel);
+    env->DeleteLocalRef(jcls_JvmCallbackContext);
+
+    return result;


to proxy Ryan, let's do this first, and do
if (!exception) {
return result
}
// handle exception

thank you @RyanTheOptimist :)

alyssawilk · 2022-12-06T16:03:06Z

mobile/library/common/jni/jni_interface.cc

  jobject result =
      env->CallObjectMethod(j_context, jmid_onComplete, j_stream_intel, j_final_stream_intel);

+  exception_check(env);


comment on why it's Ok to clear without handling?

alyssawilk · 2022-12-06T16:03:37Z

mobile/library/common/jni/jni_utility.cc

  jni_delete_global_ref(const_cast<void*>(context));
 }

+bool exception_check(JNIEnv* env) {


I'd be inclined to ABSL_MUST_USE_RESULT for this. or at least make it clear we're clearing

I do not think that it always have to be handled explicitly. Per ur suggestion, will change method's name to imply that it clears any pending exceptions

Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>

alyssawilk · 2022-12-07T17:07:51Z

mobile/library/common/jni/jni_interface.cc


-static void* jvm_on_headers(const char* method, envoy_headers headers, bool end_stream,
-                            envoy_stream_intel stream_intel, void* context) {
+static void* jvm_on_headers(const char* method, const Envoy::Types::ManagedEnvoyHeaders& headers,


this one isn't called by jvm directly either. optional nit onHeadersFromJvm or something

it's calling a method from the java world tho which I think is why it has a jvm prefix (pass_headers - now renamed to passHeaders- method did not havejvmprefix as it was/is not calling intojvm`).

I do not have a strong opinion in here but will keep the things they are until we figure out new naming patterns for this file - ideally in a separate PR.

…-cpp-to-latest-version * origin/main: (23 commits) Reduce Route memory utilization by avoiding RuntimeData instances when not needed (#24327) build: fix compile error for mac (#24429) postgres: support for upstream SSL (#23990) iOS: split `EnvoyEngine.h` into multiple header files (#24397) mobile: check for pending exceptions after JNI call (#24361) Remove uneccessary `this->` from mobile engine builder (#24389) Add setRequestDecoder to ResponseEncoder interface (#24368) downstream: refactoring code to remove listener hard deps (#24394) lb api: moving load balancing policy specific configuration to extension configuration (#23967) ci: Skip docker/examples verification for docs or mobile only changes (#24417) ci: run mobile GitHub Actions on every PR (#24407) mobile: remove `bump_lyft_support_rotation.sh` script (#24404) Add file size to DirectoryEntry (#24176) bazel: update to 6.0.0rc4 (#24235) bazel: update rules_rust (#24409) Ecds config dump recommit (#24384) bazel: add another config_setting incompatible flag (#24270) listeners: moving listeners to extension directory (#24248) mobile: build Swift with whole module optimization (#24396) ci: update `actions/setup-java` from v1 to v3.8 (#24393) ... Signed-off-by: JP Simard <jp@jpsim.com>

Augustyniak requested review from RyanTheOptimist, abeyad, alyssawilk and jpsim as code owners December 5, 2022 17:33

repokitteh-read-only bot added the mobile label Dec 5, 2022

repokitteh-read-only bot assigned alyssawilk Dec 5, 2022

Augustyniak added 17 commits December 6, 2022 08:19

checking for exceptions

cbc1bfc

Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>

checking for exceptions

d74110c

Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>

add test

0ef0bd8

Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>

improve tests

7f4f302

Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>

rename

7e45442

Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>

Lint fixes

def2550

Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>

potential fix

b0730d0

Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>

change level

7670d9d

Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>

“:hammer:”

e196512

Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>

attempt

4f5137c

Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>

disable

00ba364

Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>

disable

125460c

Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>

remove logging as it was locking

3ad5ae0

Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>

remove

ff3266d

Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>

fix pre-format

57727d3

Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>

Revert "remove"

7ee1e64

This reverts commit 479b7f8936a9e50c25be20ed798347042ebcfda6. Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>

remove expectedMessages

f566610

Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>

Augustyniak force-pushed the checking-for-exceptions-after-headers branch from 8ac1784 to f566610 Compare December 6, 2022 13:19

alyssawilk reviewed Dec 6, 2022

View reviewed changes

repokitteh-read-only bot added waiting and removed waiting labels Dec 6, 2022

Augustyniak added 2 commits December 6, 2022 16:38

cr

9521741

Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>

lint fix

bda8425

Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>

Augustyniak force-pushed the checking-for-exceptions-after-headers branch from 0f8783f to bda8425 Compare December 6, 2022 21:38

rename pass_headers

b99b523

Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>

alyssawilk approved these changes Dec 7, 2022

View reviewed changes

Augustyniak merged commit d3d4a01 into main Dec 7, 2022

Augustyniak deleted the checking-for-exceptions-after-headers branch December 7, 2022 20:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mobile: check for pending exceptions after JNI call#24361

mobile: check for pending exceptions after JNI call#24361
Augustyniak merged 20 commits intomainfrom
checking-for-exceptions-after-headers

Augustyniak commented Dec 5, 2022 •

edited

Loading

Uh oh!

repokitteh-read-only bot commented Dec 5, 2022

Uh oh!

alyssawilk left a comment

Uh oh!

alyssawilk Dec 6, 2022

Uh oh!

Augustyniak Dec 6, 2022 •

edited

Loading

Uh oh!

alyssawilk Dec 6, 2022

Uh oh!

alyssawilk Dec 6, 2022

Uh oh!

Augustyniak Dec 6, 2022

Uh oh!

alyssawilk Dec 6, 2022

Uh oh!

alyssawilk Dec 6, 2022

Uh oh!

Augustyniak Dec 6, 2022

Uh oh!

alyssawilk Dec 7, 2022

Uh oh!

Augustyniak Dec 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

Augustyniak commented Dec 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Dec 5, 2022

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Augustyniak Dec 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Augustyniak commented Dec 5, 2022 •

edited

Loading

Augustyniak Dec 6, 2022 •

edited

Loading