Jvm cannot be sent between threads. How can I work with invoke_async?

[pickleburger]

Hello,

Based on #72, Jvm cannot be sent between threads. In that case, how can I work with invoke_async? As soon as I call await on the future returned by invoke_async, I get the compilation error complaining that Jvm cannot be sent between threads. How can I work around it? Thanks!

Sample error message:

the trait `Send` is not implemented for `*mut *const JNINativeInterface_`, which is required by `{async block@src/main.rs:53:134: 76:6}: Send`

Sample code snippet

#[tonic::async_trait]
impl MyApi for MyApiService {

    async fn get(&self, request: Request<GetRequest>) -> Result<Response<GetResponse>, Status> {
        let Ok(jvm) = Jvm::attach_thread()
            else { return Err(Status::internal("attach_thread failed")) };
        let Ok(my_object) = jvm.invoke_static("com.example.MyObject", "getInstance", &Vec::<InvocationArg>::new())
            else { return Err(Status::internal("invoke_static failed")) };
        // let args = ...
        let _ = jvm.invoke_async(&my_object, "get", &vec![args]).await;

        // ...
        Ok(Response::new(GetResponse::default()))
    }
}

[astonbitecode]

As long as you do not use jvm later, it should not be a problem.
Remember that after awaiting the execution thread changes and the Jvm should be attached to the new thread.

Unfortunately I cannot test it myself as I am currently on vacations, staying away from the keyboard..

[pickleburger]

It's probably due to tonic forcing the future to be Send by using async_trait, but I'm not 100% sure. Do you think that's a plausible explanation?

Also found some discussion here: hyperium/tonic#844

[astonbitecode]

Ah, I believe I see the problem... The Future returned by invoke_async is not Send indeed... The reason is that the Jvm is used internally after the Future is created in order to check for the possible errors of the invocation.

This is fixable though and will deal with it in a few days.

Thanks for the report, this is a nice finding!

[pickleburger]

Awesome! Thanks for confirming! Looking forward to the fix.

[astonbitecode]

I took the time to test using async with tonic and unfortunately, there are more errors regarding Send for the Future returned by the invoke_async, additionally to the error message you mentioned...

This is fixable though and will deal with it in a few days.

This was an over-optimistic statement because I took as granted that the issue was only because of the internal reuse of the Jvm during the invoke_async execution and after calling await.

However, there are more errors, with the most important being:

error[E0277]: `*mut *const JNINativeInterface_` cannot be shared between threads safely

Even if the rest of the errors are somehow fixed, I guess we cannot do anything about this one... The reason is that by jni specification:

The JNI interface pointer is only valid in the current thread. A native method, therefore, must not pass the interface pointer from one thread to another.

So, I believe by JNI definition, the Future returned by j4rs can never be Send.

[astonbitecode]

Taking a step back though, I cannot fully understand why we get the compilation error, that is, why the block before await must be also Send...

For example, in the code snippet you provided, could the execution be stopped at line:

let Ok(jvm) = Jvm::attach_thread()
            else { return Err(Status::internal("attach_thread failed")) };

and then continue from line:

let Ok(my_object) = jvm.invoke_static("com.example.MyObject", "getInstance", &Vec::<InvocationArg>::new())
            else { return Err(Status::internal("invoke_static failed")) };

in another thread?

I thought this execution is not interrupted... What do I miss?

[pickleburger]

I thought this execution is not interrupted

Yes, you are correct. I don't think the execution is interrupted after either attach_thread or invoke_static. It's interrupted when awaiting the future returned from invoke_async.

I also thought about how to fix it. I have a strong feeling that we need to implement the future manually, instead of relying on async fn to generate it for us. I'll look deeper into it probably in the coming weekend.

[astonbitecode]

It seems that with a bit of "scoping" and "moving" we can have the Future + Send we need.

I implemented a function invoke_into_sendable_async in Jvm that does not take a Jvm input (either as argument or as self reference) and consumes all its arguments.

1. No Jvm input because it seems that Jvm must not be in scope while awaiting the future.
2. Consumes all its arguments because neither &Instance not &[InvocationArg] are Send

According to the above, your example should compile like:

#[tonic::async_trait]
impl MyApi for MyApiService {

    async fn get(&self, request: Request<GetRequest>) -> Result<Response<GetResponse>, Status> {
    	
        let (my_object, args) = {
            let Ok(jvm) = Jvm::attach_thread() else {
                return Err(Status::internal("attach_thread failed"));
            };
            let Ok(my_object) = jvm.invoke_static(
                "com.example.MyObject",
                "getInstance",
                &Vec::<InvocationArg>::new(),
            ) else {
                return Err(Status::internal("invoke_static failed"));
            };
            let args: Vec<InvocationArg> = vec![
                InvocationArg::try_from("a_string").unwrap(),
                InvocationArg::try_from(3).unwrap(),
                // ...
            ];
            (my_object, args)
        };

        let _ = Jvm::invoke_into_sendable_async(my_object, "get".to_string(), args).await;

        // ...
        Ok(Response::new(GetResponse::default()))
    }
}

Thoughts:

• The Jvm::invoke_into_sendable_async implicitly invokes Jvm::attach_thread twice... Once before the await and one after. This may have performance penalty. If this is not good for you, you may think if it is better to use Jvm via other means, like message-passing/Actors etc
• This whole Send story made me started thinking whether it is correct to mark &Instance as Send. Such thing would make Instance to be Sync. If this happens, we could have the api of Jvm::invoke_into_sendable_async accepting references instead of moving values
• I still have the feeling that the compiler is needlessly strict in our case. This needed scoping tricks may be the proof.

[pickleburger]

Thanks for putting up the quick fix! I was thinking about manually creating the future so that even if we pass Jvm and Instance as references, they are not necessarily captured in the future. But such implementation could be tricky. Your approach also works.

However, I think there is an issue in the implementation. I put up a PR here #104.

Making Instances Send could be useful if we want to reuse the object after awaiting the future

Yeah, if the compiler can deduce that the non-sendable variables are never used across await, it would make our life lot easier. It seems we have to do manual scoping today. The trick is also described in https://rust-lang.github.io/async-book/07_workarounds/03_send_approximation.html. Sad :(

In terms of performance, it might be Ok to attach the thread twice. I'm not sure how much penalty we will hit. I think we need some benchmarking to get some idea. However, I checked the Java part and I'm concerned about the use of Stream APIs. Based on my past experience, they are bad for GC and CPU. Given j4rs is a low-level library, I think it's worthwhile to optimize for the performance at the sacrifice of "stylish code". I can put up some PRs to migrate them to the old-school loops, if you agree with that statement :).

[pickleburger]

Making Instances Send could be useful if we want to reuse the object after awaiting the future

Sorry, I misread your comment. I saw that Instance was already marked as Send. Making &Instance Send would make Instance Sync IIUC, some work may be needed. An ugly workaround would be sending back the invocation args from the future.

Self::do_return(new_jni_env, instance)?.map(|instance| (instance, inv_args))

It seems that currently scoping is the only way to make compiler believe that a non-sendable variable is no longer being used. I tried the following to reuse the Jvm from the current thread, but it didn't work.

    pub async fn invoke_into_sendable_async_consuming_self(
        self,
        instance: Instance,
        method_name: String,
        inv_args: Vec<InvocationArg>,
    ) -> errors::Result<Instance> {
        debug(&format!(
            "Asynchronously invoking (2) method {} of class {} using {} arguments",
            method_name,
            instance.class_name,
            inv_args.len()
        ));
        // Create the channel
        let (sender, rx) = oneshot::channel::<errors::Result<Instance>>();
        unsafe {
            Self::handle_channel_sender(&self, sender, &instance, &method_name, inv_args.as_ref())?;
        }
        drop(self);

        // Create and return the Instance
        let instance = rx.await?;
        let new_jvm = Jvm::attach_thread()?;
        let new_jni_env = new_jvm.jni_env;
        Self::do_return(new_jni_env, instance)?
    }

[astonbitecode]

The Instances can be Sync, as they do not implement internal mutability, but, there cannot be any guarantees on the Java side about thread-safety. It is (should be) understood that JNI does not come with thread-safety by default.

However, even if we have references of Instances be Send, there will be borrowing issues, as the borrowed references to be used for the Future should have 'static lifetime. So, I believe we do not need currently any changes for the guarantees of Instance.

Having said that, I believe that the api of Jvm::invoke_into_sendable_async cannot get better... If someone wants to reuse Instances passed as arguments, they should call Jvm.clone_instance earlier, so that they can "reuse" the Instance.

So, if it works for you too and you can use the invoke_into_sendable_async without issues, we can close this (after, of course, applying the PR - I mistakenly left the code you correct like this because I was playing around with the lifetimes...)

A bit out of context, but I think the clone_instance should be renamed to something better, in order not to give wrong impression or expectations about cloning. What it actually does, is that it creates a new Instance that contains a new Java reference to what the initial Instance contained. This will be dealt in the near future.

[pickleburger]

Sounds good. Thanks for addressing the issue! Feel free to close it.

[pickleburger]

BTW, when do you think you can release this new feature?

[astonbitecode]

There is no specific schedule. I can create a new release today or tomorrow.