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

Lacking basic error handling #43

Open
ronhe opened this issue Feb 5, 2023 · 3 comments
Open

Lacking basic error handling #43

ronhe opened this issue Feb 5, 2023 · 3 comments
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@ronhe
Copy link

ronhe commented Feb 5, 2023

Hi,

I've noticed that there is currently no error handling at all; No JNI'sExceptionCheck and not even nullptr checks. This leads to app crash when something goes wrong.
For example, trying to invoke a method from a Java class which doesn't exist results in SIGABRT.

This doesn't sound right, and also in contradiction to Android's JNI Tips.

I was wondering if this is something that is going to be fixed in the near future, and also if you have any workaround that can make this (very promising) library usable at its current version.

Thanks in advance!

@jwhpryor
Copy link
Collaborator

Hello! Apologies for not responding sooner, I missed that this issue was added.

Indeed there is not error handling, however, it is very much on my list of things to address. It did not occur to me that this would make applications where an exception is thrown technically unviable.

First, as a lame workaround, you can simply call the underlying JNI methods for checking exceptions. java.lang.Throwable looks to be a regular class so you could presumably describe it with JNI Bind if you wanted. That obviously isn't very satisfying, but if you're keen to use JNI Bind, this would hopefully unblock you.

Long term, I hope for a the exception syntax to look something like:

static constexpr ::jni::Class kClass {
  "kClass",
  jni::Method{"Foo", ... }.Throws(kIOException, kEtcException, ... }
}; 

jni::LocalObject<kClass> obj {};
// obj("Foo", ...);  won't compile
jni::Try {
  obj.Foo(...);
}.Catch([](jni::LocalObject<kIOException> e) {...})
.Catch(...);

This is still insufficient for cases like FindClass, and more broadly, if you wanted to check for exceptions every call (where the above syntax is clumsy, but also referring to something slightly different). I didn't do this for the first pass because I didn't understand the performance implications, and taking it away down the road seemed bad.

What I intend to do is to provide a compile time optional pre and post lambda that you can write for all JNI calls. This could be used for things like logging also. I haven't thought deeply enough on how to accomplish this, and I figure it may take a bit of time.

My first priority is resolving existing bugs, but when that's done, I'll look at this next. Resolving this is probably worth gating the 1.0 for. as the syntax will be forever.

Thank you for your interest in JNI Bind!

@jwhpryor jwhpryor self-assigned this Feb 10, 2023
@jwhpryor jwhpryor added this to the Release 1.0 milestone Feb 10, 2023
@ronhe
Copy link
Author

ronhe commented Feb 11, 2023

Thanks for the elaborated reply.

I think that an optional post JNI call lambda could allow resolving the most burning issue IMHO, which is avoiding app crash.
I would use that lambda for calling ExceptionCheck() and throwing a C++ exception in case a Java exception is pending.

The exception syntax in given example looks very neat, but I'm not sure how it would get along with the post JNI call lamda that checks for Java exceptions.

I'll definitely keep an eye on on this library, I'm sure that version 1.0 will be awesome.

@jwhpryor jwhpryor removed the Release label Nov 16, 2023
@jwhpryor
Copy link
Collaborator

I fully intend to complete this, however, I think unfortunately I'm going to deprioritize this from the initial release. There is simply too much to do, and this isn't absolutely critical for the tool to work (given we're already using it in prod).

I absolutely agree you should do this, but unfortunately, I just haven't had time to complete this yet.

@jwhpryor jwhpryor removed this from the Release 1.0 milestone Nov 18, 2023
@jwhpryor jwhpryor added this to the Release 1.2 milestone Mar 18, 2024
@jwhpryor jwhpryor added the enhancement New feature or request label Mar 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants