Xuanwo commented on code in PR #5746:
URL: https://github.com/apache/opendal/pull/5746#discussion_r1989495134
##########
bindings/java/src/executor.rs:
##########
@@ -186,10 +188,11 @@ pub(crate) fn executor_or_default<'a>(
///
/// This function could be only when the lib is loaded.
unsafe fn default_executor<'a>(env: &mut JNIEnv<'a>) -> Result<&'a Executor> {
- RUNTIME.get_or_try_init(|| {
+ Ok(RUNTIME.get_or_init(|| {
make_tokio_executor(
env,
available_parallelism().map(NonZeroUsize::get).unwrap_or(1),
)
- })
+ .expect("Failed to initialize default executor")
Review Comment:
When handling panic in bindings, we should be very careful to avoid
spreading panic across language boundaries. The bindings code is designed this
way intentionally. It is recommended not to change returned errors into
`expect` or `panic`.
##########
bindings/java/src/executor.rs:
##########
@@ -43,7 +43,9 @@ thread_local! {
/// This function could be only called by java vm when unload this lib.
#[no_mangle]
pub unsafe extern "system" fn JNI_OnUnload(_: JavaVM, _: *mut c_void) {
- let _ = RUNTIME.take();
+ // Since OnceLock doesn't have a take() method, we can't remove the runtime
Review Comment:
Hi, I'm not sure about this
https://doc.rust-lang.org/stable/std/sync/struct.OnceLock.html#method.take
##########
bindings/java/src/executor.rs:
##########
@@ -20,6 +20,7 @@ use std::ffi::c_void;
use std::future::Future;
use std::num::NonZeroUsize;
use std::sync::atomic::{AtomicUsize, Ordering};
+use std::sync::OnceLock;
Review Comment:
MSRV is not as important in bindings as it is in rust core since we only
distribute our built artifacts. However, I still suggest starting a discussion
with the Java bindings maintainers to ensure they are all aware of these
upcoming changes.
cc @tisonkun @G-XD
--
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]
