Java: Handle panics and errors in the Java FFI layer #355
Conversation
jonathanl-bq
requested review from
GumpacG,
aaron-congo,
Yury-Fridlyand,
acarbonetto,
tjzhang-BQ,
cyip10 and
yipin-chen
| let value = unsafe { Box::from_raw(pointer as *mut Value) }; | ||
| redis_value_to_java(env, *value, true) | ||
| } | ||
| let result = value_from_pointer(&mut env, pointer); |
There was a problem hiding this comment.
What is the benefit of defining a funcion and calling it right away?
Why can't we do this?
| let result = value_from_pointer(&mut env, pointer); | |
| let value = unsafe { Box::from_raw(pointer as *mut Value) }; | |
| let result = redis_value_to_java(env, *value, true); |
There was a problem hiding this comment.
In this case, there is no particular benefit. The reason why I define inner functions is because I didn't want to have to handle errors inline every single time I get a Result back (instead of using unwrap). To use ? syntax, you need a function that returns a Result though, so this is sort of a framework to follow. As for why I couldn't just roll this into another closure that is called from the closure passed to catch_unwind, it's because of this issue: jni-rs/jni-rs#432
Basically, the author of jni-rs didn't implement UnwindSafe for &mut JNIEnv, which means in order to use the JNIEnv, we need to own it inside of the closure instead of just borrowing it. If I were to pass the inner function as a closure, it would need to either borrow the JNIEnv (we can't do that), or always explicitly return the JNIEnv along with every payload, which is just unergonomic. As far as I can tell, there isn't a cleaner solution than this as long as that issue is still open. I could move the inner function definitions to the top level instead to avoid rightward drift, but that's just a style change.
There was a problem hiding this comment.
Is it difficutl to
implement UnwindSafe for &mut JNIEnv
?
Maybe we you can contribute to jni-rs and later update our lib.rs once this fix released.
There was a problem hiding this comment.
It's not possible to do this in our code because foreign traits cannot be implemented for foreign types. It would break trait coherence rules. It would need to be fixed upstream in jni-rs. I don't know how difficult it would be to implement this upstream. I'll need to try and see what goes wrong, but that's not a bad idea.
There was a problem hiding this comment.
Yes I considered pushing a fix to jni-rs, but we shoudn't wait for it now.
jonathanl-bq
merged commit 3f04b40
into
java/integ_lotjonat_panics
This change restructures the FFI layer for the Java client to allow panics to be caught. It also allows errors to be caught in a more flexible and ergonomic manner. The API could theoretically be simplified further, but it is currently not feasible due to
catch_unwindneeding to take ownership of theJNIEnvdue to this issue: jni-rs/jni-rs#432