fix: drop useless import and clippy happy for java binding #5746
Conversation
Signed-off-by: yihong0618 <[email protected]>
github-actions
bot
added
the
releases-note/fix
|
In which PR we bump MSRV? I remember @Xuanwo suppose to hold on that the last time. |
yihong0618
changed the title
Signed-off-by: yihong0618 <[email protected]>
bindings/java/src/executor.rs
Outdated
| @@ -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 | |||
There was a problem hiding this comment.
Hi, I'm not sure about this https://doc.rust-lang.org/stable/std/sync/struct.OnceLock.html#method.take
Signed-off-by: yihong0618 <[email protected]>
Signed-off-by: yihong0618 <[email protected]>
Signed-off-by: yihong0618 <[email protected]>
bindings/java/src/executor.rs
Outdated
| @@ -43,7 +43,8 @@ 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(); | |||
| // This is fine as the JVM is shutting down anyway | |||
There was a problem hiding this comment.
Hi, please don't change this unless we have good reasons.
bindings/java/src/executor.rs
Outdated
| // It's okay if another thread initialized it first | ||
| let _ = RUNTIME.set(executor); | ||
|
|
||
| // Now RUNTIME should be initialized | ||
| Ok(RUNTIME.get().ok_or_else(|| { | ||
| opendal::Error::new( | ||
| opendal::ErrorKind::Unexpected, | ||
| "Failed to initialize default executor", | ||
| ) | ||
| }) | ||
| })?) |
There was a problem hiding this comment.
Maybe we can use get_or_init here to save an extra error check:
Ok(RUNTIME.get_or_init(|| executor))Signed-off-by: yihong0618 <[email protected]>
|
bindings/java/src/executor.rs
Outdated
| // It's okay if another thread initialized it first | ||
| let _ = RUNTIME.set(executor); | ||
|
|
||
| // Now RUNTIME should be initialized | ||
| Ok(RUNTIME.get().ok_or_else(|| { | ||
| opendal::Error::new( | ||
| opendal::ErrorKind::Unexpected, | ||
| "Failed to initialize default executor", |
There was a problem hiding this comment.
| // It's okay if another thread initialized it first | |
| let _ = RUNTIME.set(executor); | |
| // Now RUNTIME should be initialized | |
| Ok(RUNTIME.get().ok_or_else(|| { | |
| opendal::Error::new( | |
| opendal::ErrorKind::Unexpected, | |
| "Failed to initialize default executor", | |
| Ok(RUNTIME.get_or_init(|| executor)) |
There was a problem hiding this comment.
Not sure if Rust corelib has a get_or_try_init. Or else we can add one ..
There was a problem hiding this comment.
Signed-off-by: yihong0618 <[email protected]>
|
is this way lose error message? |
Error handling happen at |
learned thanks |
|
Hi, @yihong0618 I just noticed that this PR removed the |
seems we can not use it here will take a look |
Hi, I don't think it's fine. We know it works good before and I don't want to change here without good reasons. |
yes you are right, will dig into it |
Which issue does this PR close?
Closes #5745
This patch do these things for java binding:
Replace once_cell::sync::OnceCell with the standard library's std::sync::OnceLock (available since Rust 1.70)