Conversation
src/intent.rs
Outdated
| } | ||
|
|
||
| pub fn new(env: JNIEnv<'env>, action: impl AsRef<str>) -> Self { | ||
| pub fn new(mut env: JNIEnv<'env>, action: impl AsRef<str>) -> Self { |
There was a problem hiding this comment.
With new changes I don't think we can own the JNIEnv unless we attach the env permanently - defeating the purpose of fn with_current_env(). Need to see if should store either &'env JNIEnv<'env> in the Intent struct, which can be derived from the AttachGuard.
There was a problem hiding this comment.
Skimming these changes, it looks like its gone in the right direction so far with the API no longer expecting to own a JNIEnv, and instead working with &mut JNIEnv. The previous implementation of with_current_env looks like it probably handed out a JNIEnv<'static> which would have made it possible to keep longer than the thread may have been attached (and then been an invalid pointer).
As a heads up here I'm trying to tackle some annoying, long-standing safety issues in jni-rs related to thread attachments and JNIEnv lifetimes, which is leading to some changes related to AttachGuards.
The two most likely options currently are:
-
Thread attachment will become
unsafeand all kinds of attachments will give you anAttachGuardthat needs to be kept on the stack (can't be moved anywhere'static) and the JNIEnv will only be accessible by borrowing from a thread attachment guard. -
Instead of handing out an
AttachGuard(which is a problematic design because the Rust language doesn't let us constrain it to be immovable in the way that's needed) then thread attachments will apply to a given closure instead like:vm.attach_current_thread(|env| { // do stuff that relies on `&mut JNIEnv` })
(kind of like with_current_env in this crate)
In either case though JNIEnv will no longer be a #[transparent] wrapper over the sys pointer, and the public API will never expose a JNIEnv type by-value - it will be conceptually always be borrowing from a thread attachment (or more specifically can be thought of as borrowing from a JNI stack frame for the attached thread)
If you want to follow along with those changes, I have a draft PR here currently: jni-rs/jni-rs#570
Actually it would be great if you might also be able to be another pair of eyes for that work in progress 🤞
2 participants
No description provided.