Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix a couple of code paths where exceptions weren't checked for #205

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tjb900
Copy link
Contributor

@tjb900 tjb900 commented Jun 11, 2022

(at least before the next JNI call, which is what generates a warning)

This was discovered with -Xcheck:jni, leading to warnings like these

WARNING in native method: JNI call made without checking exceptions when required to from CallStaticObjectMethodA
WARNING in native method: JNI call made without checking exceptions when required to from CallObjectMethodA
WARNING in native method: JNI call made without checking exceptions when required to from CallBooleanMethodA
WARNING in native method: JNI call made without checking exceptions when required to from CallObjectMethodA
WARNING in native method: JNI call made without checking exceptions when required to from CallObjectMethodA
WARNING in native method: JNI call made without checking exceptions when required to from CallStaticObjectMethodA
WARNING in native method: JNI call made without checking exceptions when required to from CallObjectMethodA
WARNING in native method: JNI call made without checking exceptions when required to from CallBooleanMethodA
WARNING in native method: JNI call made without checking exceptions when required to from CallStaticObjectMethodA
WARNING in native method: JNI call made without checking exceptions when required to from CallObjectMethodA
WARNING in native method: JNI call made without checking exceptions when required to from CallBooleanMethodA

I think what is happening in the two ObjectMethod cases is that while the application code is checking the exception state, javabridge is making some followup JNI calls before that can happen, leading to the warnings.

I can gather further evidence for that if you feel it's needed.

I was initially hesitant to put this one up, because javabridge performing (and ignoring) the exception check risks silencing the warnings in the case that they are valid - i.e. an incorrectly written application that does not check the exception itself. However, I think one can argue that since it's impossible to fix these warnings by making the application correct, the warnings become not particularly useful and silencing them is the lesser of two evils.

Certainly open to other suggestions on how to silence these, though.

(at least before the next JNI call)

This was discovered with -Xcheck:jni, leading to warnings like these

```
WARNING in native method: JNI call made without checking exceptions when required to from CallStaticObjectMethodA
WARNING in native method: JNI call made without checking exceptions when required to from CallObjectMethodA
WARNING in native method: JNI call made without checking exceptions when required to from CallBooleanMethodA
WARNING in native method: JNI call made without checking exceptions when required to from CallObjectMethodA
WARNING in native method: JNI call made without checking exceptions when required to from CallObjectMethodA
WARNING in native method: JNI call made without checking exceptions when required to from CallStaticObjectMethodA
WARNING in native method: JNI call made without checking exceptions when required to from CallObjectMethodA
WARNING in native method: JNI call made without checking exceptions when required to from CallBooleanMethodA
WARNING in native method: JNI call made without checking exceptions when required to from CallStaticObjectMethodA
WARNING in native method: JNI call made without checking exceptions when required to from CallObjectMethodA
WARNING in native method: JNI call made without checking exceptions when required to from CallBooleanMethodA
```
@LeeKamentsky
Copy link
Owner

Thanks for looking at this. I think, at the JNI level, I intended to have the caller call JB_Env.exception_occurred to check for an exception if appropriate and then JB_Env.exception_clear before calling the JNI again.

JNI-level code is usually pretty long-winded. The higher level stuff (e.g. javabridge.jutil.make_call) does check for exceptions and mirrors them as Python exceptions.

@tjb900
Copy link
Contributor Author

tjb900 commented Jun 13, 2022

I intended to have the caller call JB_Env.exception_occurred to check for an exception if appropriate and then JB_Env.exception_clear before calling the JNI again.

Oh, just to make sure we are on the same page, I totally agree - and indeed our application definitely does this! The only issue is that for methods that return an object, only in the case of successful non-null return, the application doesn't get a chance to do that before javabridge itself calls back into JNI as part of make_jb_object, which goes back into JNI via NewGlobalRef/DeleteLocalRef.

I'm fairly sure that the warnings I mentioned are generated by NewGlobalRef being called after a Call[Static]ObjectMethod without an intervening exception check - which is part of what this adds.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants