rib commented on code in PR #7000:
URL: https://github.com/apache/opendal/pull/7000#discussion_r2616495475
##########
bindings/java/src/executor.rs:
##########
@@ -116,24 +116,45 @@ pub unsafe extern "system" fn
Java_org_apache_opendal_AsyncExecutor_disposeInter
}
pub(crate) fn make_tokio_executor(env: &mut JNIEnv, cores: usize) ->
Result<Executor> {
- let vm = env.get_java_vm().expect("JavaVM must be available");
+ let vm = Arc::new(env.get_java_vm().expect("JavaVM must be available"));
let counter = AtomicUsize::new(0);
let executor = tokio::runtime::Builder::new_multi_thread()
.worker_threads(cores)
.thread_name_fn(move || {
let id = counter.fetch_add(1, Ordering::SeqCst);
format!("opendal-tokio-worker-{id}")
})
- .on_thread_start(move || {
- ENV.with(|cell| {
- let mut env = vm
- .attach_current_thread_as_daemon()
- .expect("attach thread must succeed");
+ .on_thread_start({
+ let vm = vm.clone();
+ move || {
+ ENV.with(|cell| {
+ let mut env = vm
+ .attach_current_thread_as_daemon()
Review Comment:
This should almost certainly call
[attach_current_thread_permanently](https://docs.rs/jni/latest/jni/struct.JavaVM.html#method.attach_current_thread_permanently)
instead of `_as_daemon`.
JNI daemon threads have poorly specified semantics (because the behavior of
VMs after they are destroyed is very poorly defined) and the only time their
behaviour will differ from attach_current_thread_permanently is in applications
that call `JavaVM::destroy()`. If anything would call JavaVM::destroy though,
it will become inherently unsafe and unsound for this Tokio thread to still be
running, due to the chance that `vm.detach_current_thread()` will be called
after the VM has been destroyed.
Daemon threads are so unsafe in the context of Rust JNI bindings that `jni`
0.22 plans to remove support for creating them.
Specifically here, it's either unsound or redundant to create a daemon
thread like this because the `on_thread_stop` could result in the thread using
JNI after JavaVM::destroy() has been called and the only way that wouldn't
happen is if the application makes sure to destroy the tokio runtime + thread
pool first (and then there would be no reason to have created daemon thread
attachments).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
