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

[Java API] Validate that there is a check for an exception after all JNI calls that can raise an exception #434

Open
vkuzmin-uber opened this issue Sep 16, 2020 · 0 comments
Labels
bindings/java Issues and PRs related to the Neuropod Java bindings

Comments

@vkuzmin-uber
Copy link
Contributor

I looked at "Best practices for using the Java Native Interface" https://developer.ibm.com/technologies/java/articles/j-jni/ and found reasonable advises:

  1. "For new JNI code validate that there is a check for an exception after all JNI calls that can raise an exception."
    Current Neuropod JNI code doesn't have any "env‑>ExceptionCheck". Except a check, JNI code should clear it after all. Need to use some consistent approach for every JNI call that may throw exception.

Note, I looked at Tensorflow Java/JNI code and found that its JNI exception handling is not consistent, it only checks for env‑>ExceptionCheck and never clears it.

  1. There are JVM flags dedicated for tracing and checking JNI code. Consider what JNI check options available in target JVM (‑Xcheck:jni). Note source/neuropod/bindings/java/java_build_defs.bzl already uses some settings for "bazel errorprone plugin" (for instance
    "-Xep:ExpectedExceptionChecker:ERROR"). Need to clarify if it works and if it can report error/warn on current absence of JNI ExceptionCheck calls.

Note that current absence of ExceptionCheck is not crucial because in most of the cases exception means that JVM is already in a bad state. But we are going to use it in a critical service with a high load, at scale, with many dependencies. It is critical that Neuropod Java API is following modern best practices.

@VivekPanyam VivekPanyam added the bindings/java Issues and PRs related to the Neuropod Java bindings label Sep 17, 2020
VivekPanyam pushed a commit that referenced this issue Dec 22, 2020
…ll (#462)

### Summary:
Most of JNI calls may cause Java Exception that is not thrown automatically in JNI code. We already check results of calls in some cases. This change also adds ExceptionCheck where this is reasonable. If Java exception detected, we print it, clear and throw custom exception. Issue #434 

### Test Plan:
CI
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bindings/java Issues and PRs related to the Neuropod Java bindings
Projects
None yet
Development

No branches or pull requests

2 participants