jni-rs: 0.21 mutability requirements on `JNIEnv` methods break use of `catch_unwind`

Right, this is an unfortunate side effect of the changes made for 0.21, sorry about that. More than once now I’ve found myself compare this project to a game of whack a mole when it comes to addressing safety concerns.

As you say, you can currently use AssertUnwindSafe

The JNIEnv is really just a thin wrapper over a pointer to a table of function pointers managed by JNI / the JVM implementation.

As far as Rust is concerned this isn’t really stateful (so you shouldn’t be able to observe an inconsistent state for this table due to a panic).

The design allows the JVM to provide a different table of functions for different threads but I’ve never seen anything to suggest that JNI functions would ever try and dynamically fiddle with that table of pointers on the fly - and even if it did it would still have to remain consistent from the pov of Rust code.

The JVM can store private thread-local storage within the JNIEnv but that also has to always remain consistent from the pov of Rust code. It might be mutated within the JNI/JVM implementation but that code won’t panic / unwind.

At the very least the new &mut JNIEnv is as unwind safe as the previous &JNIEnv was.

One thing I’ve wondered about briefly is whether the jni crate can provide some built-in helpers for catching panics and re-throwing as Java exceptions, similar to with_local_frame. Maybe this would be able to improve the ergonomics here.

Although the closure would still need to be passed a &mut JNIEnv argument we could at least deal with wrapping it within AssertUnwindSafe automatically. I’m not quite sure what the ergonomics of that would feel like without trying it out.

I’ve also been thinking about this from the pov of trying to make JNIEnv stateful - whereby it wouldn’t just be a thin wrapper over the JNI C pointer but would instead be our own struct that lets jni-rs cache certain things like method IDs for commonly used types. Right now the utility APIs for things like JList or JMap aren’t ideal at all because they have to repeatedly query the method IDs for those classes since we have nowhere to cache them.

If we were to introduce a StatefulJNIEnv type of some kind then it would be good if we had a built-in API for handling catching panics (+ re-throwing exceptions) that could always be used consistently to map from a raw JNIEnv passed to a native method to a closure that would be passed a StatefulJNIEnv.

Right now there isn’t another issue tracking this, so thanks for bringing this up.

Sorry that it’s not a great answer for now.

I want to handle panics and errors consistently and without boiler plate code; I simply want to throw Java exceptions and print to stdout if that fails. I think this is probably befitting to what most people want to do, so for what it’s worth I put the below traits and implementations together. I toyed with putting this on crates.io or submitting a PR but I didn’t want to start something that might deviate from this project or cause you problems.

Note, it’s not properly tested! Any feedback would be welcome and appreciated. This approach (especially the dummy value trait) is heavily inspired by libsingal.

// See https://github.com/rust-lang/rfcs/issues/1389
pub fn describe_panic(any: &Box<dyn std::any::Any + Send>) -> String {
    if let Some(msg) = any.downcast_ref::<&str>() {
        msg.to_string()
    } else if let Some(msg) = any.downcast_ref::<String>() {
        msg.to_string()
    } else {
        "(break on rust_panic to debug)".to_string()
    }
}

/// Provides a dummy value to return when an exception is thrown.
/// TODO flesh this out with more types.
pub trait JniDummyValue {
    fn dummy_value() -> Self;
}

impl JniDummyValue for ObjectHandle {
    fn dummy_value() -> Self {
        0
    }
}

impl JniDummyValue for jint {
    fn dummy_value() -> Self {
        0
    }
}

impl JniDummyValue for jobject {
    fn dummy_value() -> Self {
        std::ptr::null_mut()
    }
}

impl JniDummyValue for jboolean {
    fn dummy_value() -> Self {
        0
    }
}

impl JniDummyValue for () {
    fn dummy_value() -> Self {}
}

impl JniDummyValue for JString<'_> {
    fn dummy_value() -> Self {
        unsafe { JString::from_raw(jstring::dummy_value()) }
    }
}

pub(crate) trait JNIExt {
    type Error;

    /// Called when an error is returned from the closure executed by `throw_on_failure`.
    fn handle_error(&mut self, error: Self::Error);
    /// Called when an unwinding panic is returned from the closure executed by `throw_on_failure`.
    fn handle_panic(&mut self, panic_err: Box<dyn std::any::Any + Send>);

    /// Invokes the given closure and throws a Java exception from an `Result::Err` or unwinding
    /// panic if either occurs.
    ///
    /// # Note
    /// The actual calls to `JNIEnv::throw` are made in the `handle_error` and `handle_panic`
    /// implementations. Where an error variant of a Result is returned from the given closure it is
    /// passed to the `handle_error` method, and where an unwinding panic is caught it is passed to
    /// the `handle_panic` method.
    fn throw_on_failure<
        F: FnOnce(&mut Self) -> Result<R, Self::Error> + UnwindSafe,
        R: JniDummyValue,
    >(
        &mut self,
        f: F,
    ) -> R {
        let mut wrapped_env = AssertUnwindSafe(self);

        match catch_unwind({
            let mut other_wrapped_env = AssertUnwindSafe(&mut wrapped_env);

            move || (f)(***other_wrapped_env)
        }) {
            Ok(Ok(r)) => r,
            Ok(Err(ext_error)) => {
                (**wrapped_env).handle_error(ext_error);
                R::dummy_value()
            },
            Err(catch_unwind_err) => {
                (**wrapped_env).handle_panic(catch_unwind_err);
                R::dummy_value()
            },
        }
    }
}

impl JNIExt for JNIEnv<'_> {
    type Error = Foo;

    fn handle_error(&mut self, error: Self::Error) {
        let exc_msg = match error {
            Foo::SomeErr(_) => "java/lang/RuntimeException",
            _ => "java/lang/AssertionError"
        };

        if let Err(err) = self.throw_new(exc_msg, error.to_string()) {
            println!("failed to throw Java exception from error {}: {}", error, err);
        }
    }

    fn handle_panic(&mut self, panic_err: Box<dyn Any + Send>) {
        let panic_msg = describe_panic(&panic_err);

        if let Err(err) = self.throw_new("java/lang/Exception", panic_msg.as_str()) {
            println!("failed to throw Java exception for {}: {}", panic_msg, err);
        }
    }
}

And usage:

#[no_mangle]
pub extern "system" fn Java_HelloWorld_hello<'local>(
    mut env: JNIEnv<'local>,
    _class: JClass<'local>,
    input: JString<'local>,
) -> JString<'local> {
    env.throw_on_failure(|jni_env| {
        let input: String = jni_env
            .get_string(&input)
            .expect("Couldn't get java string!")
            .into();

        let output = jni_env
            .new_string(format!("Hello, {}!", input))
            .expect("Couldn't create java string!");

        Ok(output)
    })
}

No need to apologise! I understand the whack-a-mole analogy completely. The 0.21 update was mostly great to see from my perspective, having more guarantees surrounding lifetimes is a good thing in FFIs.

By the sounds of it, AssertUnwind is safe to use so I’ll roll with that for now.

I’ll update this thread should anything strange come up. I think it’s probably worth popping something in the docs regarding this for those trying to catch panics in this way.

@AlexKorn: Yes, that’s legal.

Perfect, yeah that’s the kind of thing I’d been imagining but hadn’t considered the idea of making it a trait like this. I wasn’t sure off the top of my head whether it was going to be possible to move the env into the catch_unwind closure via AssertUnwindSafe but effectively hide that and unwrap when calling the user’s closure but it looks like that works fine - nice.

It seems like it would be helpful to have something like this just be part of the jni API, out of the box, ideally, since pretty much any code that’s implementing a native Java method needs to do something like this to stop panics crossing the FFI boundary.

I like the idea of making it a trait like this so it’s possible to just override how you map an Error or panic into an exception that’s appropriate for your app / library.

Regarding the vague ideas around having some kind of stateful JNIEnv, that’s not something I’m investigating / prioritizing atm, so I’m not going to worry about how that might potentially interact with this for now, since there’s nothing concrete to consider yet.

It would be great if you’d be able to even create a PR for this considering that it looks like you’ve done most of the work already.

I’d maybe call the “Dummy” values “Default” values from the pov that it’s conceptually like you’re passing back default initialized values. “Default” matches the Java and Rust parlance here.

Actually, we already implement the Default trait for types like JObject, JClass, JString etc so can we just rely on the Default trait instead of needing the dummy types here?