Optimize casting and fix local class reference leaks; refactor Env API; add Monitor, add docs, add is_same_object, fix ReferenceType generation

[wuwbobo2021]

The same test case provided in #5 and the test below (based on the previous code) are passed on Android 7.0.

for _ in 0..1000 {
    let test_obj = Test::new(env).unwrap();
    let test_obj = test_obj.cast::<Object>().unwrap();
    let test_obj = test_obj.cast::<Test>().unwrap();
    assert!(test_obj.cast::<Throwable>().is_err());
    
    let jarr = ObjectArray::<Test, Throwable>::new(env, 3);
    for i in 0..3 {
        let test_obj = Test::new(env).unwrap();
        jarr.set(i, test_obj.as_ref()).unwrap();
    }
    let jarr = jarr.cast::<ObjectArray<Object, Throwable>>().unwrap();
    let jarr = jarr.cast::<ObjectArray<Test, Throwable>>().unwrap();
    assert!(jarr.cast::<ObjectArray<Throwable, Throwable>>().is_err());
    assert!(jarr.cast::<IntArray>().is_err());
}
log::info!("object cast test finished.");

PS: Should I change jobject arguments in Env methods to &Ref?

[wuwbobo2021]

Sorry, the wrong cast between object arrays with items of different class is not prevented by the current cast checker. I will check the previous implementation based on IsAssignableFrom if it's a wrong behavior.

[wuwbobo2021]

I should have realized the bug earlier: static OnceLock usage in trait impl for generics.

My current workaround is a bit ugly:

// NOTE: This is a performance compromise for returning `&'static JClass`,
// still faster than non-cached `require_class`.
static OBJ_ARR_CLASSES: LazyLock<RwLock<HashMap<String, &'static JClass>>> =
    LazyLock::new(|| RwLock::new(HashMap::new()));

unsafe impl<T: ReferenceType, E: ThrowableType> ReferenceType for ObjectArray<T, E> {
    fn jni_get_class(env: Env) -> &'static JClass {
        Self::static_with_jni_type(|t| {
            let class_map_reader = OBJ_ARR_CLASSES.read().unwrap();
            if let Some(&class) = class_map_reader.get(t) {
                class
            } else {
                drop(class_map_reader);
                let class: &'static JClass = Box::leak(Box::new(unsafe { env.require_class(t) }));
                let _ = OBJ_ARR_CLASSES.write().unwrap().insert(t.to_string(), class);
                class
            }
        })
    }
}

[wuwbobo2021]

I'm almost finished. All clippy warnings are fixed (Added safety descriptions for some functions, and the rest are marked with #[allow(clippy::missing_safety_doc)]). However, I would like to change the ugly JniType::static_with_jni_type to jni_type that returns Cow<'static, str>.

I realized that jni-rs and java-spaghetti aren't really providing the thread safety of using Java objects and arrays. The JNI interface provides MonitorEnter, MonitorExit, GetPrimitiveArrayCritical, and ReleasePrimitiveArrayCritical for this, but they aren't used here.

[wuwbobo2021]

Added Monitor, added brief descriptions for methods of reference types, changed JniType usage to ReferenceType. JniType trait is preserved, but it may be removed (it's up to you).

Besides, I made java-spaghetti-gen implement ReferenceType for static class types, because this Java code compiles:

Instantiatable static class

class Test {
    static int staticNum = 0;
    
    public static void main(String[] args) {
        // static class can be used as an non-static class
        StaticClassTest test1 = new StaticClassTest();
        test1.num = 7;
        StaticClassTest test2 = new StaticClassTest();
        test2.num = 9;
        System.out.println("test1.num: " + test1.num);
        
        // non-static field of the static class cannot be accessed non-statically
        // System.out.println("Wrong: " + StaticClassTest.num);
        
        // the static field value of the nested static class can be modified
        System.out.println("StaticClassTest.staticNum: " + StaticClassTest.staticNum);
        StaticClassTest.staticNum = 2;
        System.out.println("StaticClassTest.staticNum (modified): " + StaticClassTest.staticNum);
        
        // the static field value of the nested non-static class can not be modified (must be final)
        System.out.println("ClassTest.staticFinalNum: " + ClassTest.staticFinalNum);
    }
    
    public void modifyStaticNonstatically() { // compiles
        Test.staticNum = 4;
        StaticClassTest.staticNum = 4;
    }
    
    public static class StaticClassTest {
        public int num = 3;
        public static int staticNum = 1;
    }
    
    public class ClassTest {
        public int num = 3;
        public static final int staticFinalNum = 1; // failed to compile without `final`
    }
}

class ThisCompilesToo {
    public static Test.StaticClassTest createStaticClass() {
        return new Test.StaticClassTest();
    }
}

Notes for GetPrimitiveArrayCritical and MonitorEnter

See MaulingMonkey/jni-bindgen#66.

I have tested JNI GetPrimitiveArrayCritical and discovered that it may not guarantee exclusive access to the Java array. I guess it's just pinning the address of the raw storage for the Java primitive array.

I will not add a corresponding wrapper function in PrimitiveArray trait.

The useless `PrimitiveArray::with_critical_access` function, tested and removed

    /// Uses JNI `GetPrimitiveArrayCritical` and `ReleasePrimitiveArrayCritical` to obtain critical access temporarily.
    ///
    /// Do not perform any JNI operation or call any function that may be blocked by another thread. Failing to
    /// honor these restrictions may cause an uncatchable error to be thrown, or lead to intermittent
    /// [deadlock](https://developer.ibm.com/articles/j-jni/#xxx10) within the application or the JVM as a whole.
    fn with_critical_access<R>(self: &Ref<'_, Self>, f: impl FnOnce(&mut [T]) -> R) -> R {
        let len = self.len();
        if len == 0 {
            return f(&mut []);
        }
        let env = self.env().as_raw();
        unsafe {
            let p_arr = ((**env).v1_2.GetPrimitiveArrayCritical)(env, self.as_raw(), null_mut());
            assert!(!p_arr.is_null());
            self.env().exception_check_raw().expect("OOM"); // Only sane exception here is an OOM exception
            let arr_slice = std::slice::from_raw_parts_mut(p_arr as *mut T, len);
            let ret = f(arr_slice);
            ((**env).v1_2.ReleasePrimitiveArrayCritical)(env, self.as_raw(), p_arr, 0);
            ret
        }
    }

JNI MonitorEnter and MonitorExit works as expected: https://docs.oracle.com/en/java/javase/11/docs/specs/jni/functions.html#monitor-operations. Unfortunately, this monitor is different from the lock used by Java synchronized methods, and they cannot block each other.

Test case for JNI monitor

Note: this test case is a bit confusing. Printed array items are the same, so it doesn't tell that Get<Type>ArrayRegion and Set<Type>ArrayRegion are non-atomic by themselves. I should have replaced the tested array variable with a primitive int, but it was introduced for testing PrimitiveArray::with_critical_access.

package com.example.javaspaghetti;

import java.lang.InterruptedException;

public class RaceTest {
    public class TestStore {
        public int[] intArray;

        public TestStore() {
            this.intArray = new int[10];
        }

        public void intArrayIncreaseAll() {
            this.javaBusyDelay();
            for (int i = 0; i < 10; i++) {
                this.intArray[i] += 1;
            }
        }

/*
        Test result is the same as `synchronized void intArrayIncreaseAllSync()` below
        public void intArrayIncreaseAllSync() {
            synchronized (this) {
                this.javaBusyDelay();
                for (int i = 0; i < 10; i++) {
                    this.intArray[i] += 1;
                }
            }
        }
*/
        public synchronized void intArrayIncreaseAllSync() {
            this.javaBusyDelay();
            for (int i = 0; i < 10; i++) {
                this.intArray[i] += 1;
            }
        }

        void javaBusyDelay() {
            try {
                Thread.sleep(1000);
            } catch (InterruptedException e) {
                System.out.println("!!! sleep interrupted");
            }
        }
    }

    public static void main(String[] args) {
        runTest();
    }

    static native void runTest();

    static {
        System.loadLibrary("mylib");
    }
}

[package]
name = "mylib"
version = "0.1.0"
edition = "2024"

[dependencies]
java-spaghetti = { path = "../java-spaghetti" }

[lib]
crate-type = ["cdylib"]
path = "lib.rs"

include = [
    "java/lang/Object",
    "java/lang/Throwable",
    "java/lang/StackTraceElement",
    "java/lang/String",
    "java/lang/Integer",
    "com/example/javaspaghetti/RaceTest",
    "com/example/javaspaghetti/RaceTest$TestStore"
]

[[documentation.pattern]]
class_url_pattern           = "https://developer.android.com/reference/{CLASS}.html"
method_url_pattern          = "https://developer.android.com/reference/{CLASS}.html#{METHOD}({ARGUMENTS})"
constructor_url_pattern     = "https://developer.android.com/reference/{CLASS}.html#{CLASS.INNER}({ARGUMENTS})"
field_url_pattern           = "https://developer.android.com/reference/{CLASS}.html#{FIELD}"
argument_seperator          = ",%20"

[logging]
verbose = true

[input]
files = [
    # To be modified
    "E:\\android\\platforms\\android-30\\android.jar",
    ".\\RaceTest.class",
    ".\\RaceTest$TestStore.class"
]

[output]
path = "bindings.rs"

[codegen]
method_naming_style             = "java"
method_naming_style_collision   = "java_short_signature"
keep_rejected_emits             = false

[codegen.field_naming_style]
const_finals    = true
rustify_names   = false
getter_pattern  = "{NAME}"
setter_pattern  = "set_{NAME}"

Test for JNI monitor:

#![feature(arbitrary_self_types)]

use java_spaghetti::{Arg, Env, PrimitiveArray, VM};

mod bindings;
use bindings::com::example::javaspaghetti::{RaceTest, RaceTest_TestStore};
use bindings::java::lang::Object;

#[unsafe(no_mangle)]
pub extern "system" fn Java_com_example_javaspaghetti_RaceTest_runTest<'env>(env: Env<'env>, _class: Arg<Object>) {
    let test = RaceTest::new(env).unwrap();
    let test_store = RaceTest_TestStore::new(env, test.as_ref()).unwrap().as_global();

    for _ in 0..10 {
        let test_store = test_store.clone();
        std::thread::spawn(move || {
            test_store.vm().with_env(|env| {
                // Uncommenting one of both commented lines eliminates the race condition.
                let store = test_store.as_ref(env);
                //let store = store.monitor();
                let jarr = store.intArray().unwrap();
                //let jarr = jarr.as_ref().monitor();
                let mut vec_read = jarr.as_vec();
                for item in &mut vec_read {
                    *item += 1;
                }
                jarr.set_region(0, &vec_read);
                jarr.get_region(0, &mut vec_read);
                std::thread::sleep(std::time::Duration::from_millis(1000));
                eprintln!(
                    "jarr JNI Monitor test from thread {:?}, current value: {:?}",
                    std::thread::current().id(),
                    vec_read
                );
            })
        });
    }

    eprintln!("runTest() Finished");
}

#[unsafe(no_mangle)]
pub extern "system" fn JNI_OnLoad(_: VM, _: *mut ()) -> java_spaghetti::sys::jint {
    java_spaghetti::sys::JNI_VERSION_1_2
}

Test for calling synchronized methods with JNI:

    for _ in 0..10 {
        let test_store = test_store.clone();
        std::thread::spawn(move || {
            test_store.vm().with_env(|env| {
                let store = test_store.as_ref(env);
                store.intArrayIncreaseAllSync().unwrap();
                let vec_read = store.intArray().unwrap().as_vec();
                eprintln!(
                    "intArrayIncreaseAllSync test from thread {:?}, current value: {:?}",
                    std::thread::current().id(),
                    vec_read
                );
            })
        });
    }

Test for the race condition of JNI monitored calls and Java synchronous methods (I get a intArrayIncreaseAllSync prompt and a jarr JNI Monitor test prompt every second):

    for _ in 0..10 {
        let store = test_store.clone();
        std::thread::spawn(move || {
            store.vm().with_env(|env| {
                let store = store.as_ref(env);
                let jarr = store.intArray().unwrap();
                let jarr = jarr.as_ref().monitor();
                let mut vec_read = jarr.as_vec();
                for item in &mut vec_read {
                    *item += 1;
                }
                jarr.set_region(0, &vec_read);
                jarr.get_region(0, &mut vec_read);
                std::thread::sleep(std::time::Duration::from_millis(1000));
                eprintln!(
                    "jarr JNI Monitor test from thread {:?}, current value: {:?}",
                    std::thread::current().id(),
                    vec_read
                );
            })
        });
        let store = test_store.clone();
        std::thread::spawn(move || {
            store.vm().with_env(|env| {
                let store = store.as_ref(env);
                store.intArrayIncreaseAllSync().unwrap();
                let vec_read = store.intArray().unwrap().as_vec();
                eprintln!(
                    "intArrayIncreaseAllSync test from thread {:?}, current value: {:?}",
                    std::thread::current().id(),
                    vec_read
                );
            })
        });
    }

[Dirbaio]

this PR is huge and adds many features that are unrelated and could be merged independently. This makes it quite hard to review. Could you split it in independent PRs? Thanks!

[wuwbobo2021]

Sorry, I have made many commits, some of which has corrected errors introduced by prior commits. I cannot simply split them. Instead, I may review this PR for myself and describe all of these changes (maybe later).

[Dirbaio]

Instead, I may review this PR for myself

no, I need to review the changes.

You can create one branch per feature then use git cherry-pick, git commit --amend, git rebase -i etc to reorganize the commit history.