From 5bfccd16ccb03a280db75750c88da567798fde0e Mon Sep 17 00:00:00 2001 From: Jonathan Louie Date: Sat, 8 Jun 2024 01:49:11 -0700 Subject: [PATCH 01/12] Restructure Java FFI layer to handle errors properly --- java/src/lib.rs | 298 +++++++++++++++++++++++++++++++----------------- 1 file changed, 195 insertions(+), 103 deletions(-) diff --git a/java/src/lib.rs b/java/src/lib.rs index 9d42b8e298..96c8ea86e1 100644 --- a/java/src/lib.rs +++ b/java/src/lib.rs @@ -3,11 +3,13 @@ */ use glide_core::start_socket_listener; -use jni::objects::{JClass, JObject, JObjectArray, JString, JThrowable}; +use jni::errors::Error as JNIError; +use jni::objects::{JClass, JObject, JObjectArray, JString}; use jni::sys::jlong; use jni::JNIEnv; use log::error; use redis::Value; +use std::string::FromUtf8Error; use std::sync::mpsc; #[cfg(ffi_test)] @@ -15,98 +17,102 @@ mod ffi_test; #[cfg(ffi_test)] pub use ffi_test::*; +enum FFIError { + JniError(JNIError), + UDSError(String), + Utf8Error(FromUtf8Error), +} + +impl From for FFIError { + fn from(value: jni::errors::Error) -> Self { + FFIError::JniError(value) + } +} + +impl From for FFIError { + fn from(value: FromUtf8Error) -> Self { + FFIError::Utf8Error(value) + } +} + +impl std::fmt::Display for FFIError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + FFIError::JniError(err) => write!(f, "{}", err.to_string()), + FFIError::UDSError(err) => write!(f, "{}", err), + FFIError::Utf8Error(err) => write!(f, "{}", err.to_string()), + } + } +} + // TODO: Consider caching method IDs here in a static variable (might need RwLock to mutate) fn redis_value_to_java<'local>( env: &mut JNIEnv<'local>, val: Value, encoding_utf8: bool, -) -> JObject<'local> { +) -> Result, FFIError> { match val { - Value::Nil => JObject::null(), - Value::SimpleString(str) => JObject::from(env.new_string(str).unwrap()), - Value::Okay => JObject::from(env.new_string("OK").unwrap()), - Value::Int(num) => env - .new_object("java/lang/Long", "(J)V", &[num.into()]) - .unwrap(), + Value::Nil => Ok(JObject::null()), + Value::SimpleString(str) => Ok(JObject::from(env.new_string(str)?)), + Value::Okay => Ok(JObject::from(env.new_string("OK")?)), + Value::Int(num) => Ok(JObject::from(env.new_object( + "java/lang/Long", + "(J)V", + &[num.into()], + )?)), Value::BulkString(data) => { if encoding_utf8 { - let Ok(utf8_str) = String::from_utf8(data) else { - let _ = env.throw("Failed to construct UTF-8 string"); - return JObject::null(); - }; - match env.new_string(utf8_str) { - Ok(string) => JObject::from(string), - Err(e) => { - let _ = env.throw(format!( - "Failed to construct Java UTF-8 string from Rust UTF-8 string. {:?}", - e - )); - JObject::null() - } - } + let utf8_str = String::from_utf8(data)?; + Ok(JObject::from(env.new_string(utf8_str)?)) } else { - let Ok(bytearr) = env.byte_array_from_slice(data.as_ref()) else { - let _ = env.throw("Failed to allocate byte array"); - return JObject::null(); - }; - bytearr.into() + Ok(JObject::from(env.byte_array_from_slice(data.as_ref())?)) } } Value::Array(array) => { - let items: JObjectArray = env - .new_object_array(array.len() as i32, "java/lang/Object", JObject::null()) - .unwrap(); + let items: JObjectArray = + env.new_object_array(array.len() as i32, "java/lang/Object", JObject::null())?; for (i, item) in array.into_iter().enumerate() { - let java_value = redis_value_to_java(env, item, encoding_utf8); - env.set_object_array_element(&items, i as i32, java_value) - .unwrap(); + let java_value = redis_value_to_java(env, item, encoding_utf8)?; + env.set_object_array_element(&items, i as i32, java_value)?; } - items.into() + Ok(items.into()) } Value::Map(map) => { - let linked_hash_map = env - .new_object("java/util/LinkedHashMap", "()V", &[]) - .unwrap(); + let linked_hash_map = env.new_object("java/util/LinkedHashMap", "()V", &[])?; for (key, value) in map { - let java_key = redis_value_to_java(env, key, encoding_utf8); - let java_value = redis_value_to_java(env, value, encoding_utf8); + let java_key = redis_value_to_java(env, key, encoding_utf8)?; + let java_value = redis_value_to_java(env, value, encoding_utf8)?; env.call_method( &linked_hash_map, "put", "(Ljava/lang/Object;Ljava/lang/Object;)Ljava/lang/Object;", &[(&java_key).into(), (&java_value).into()], - ) - .unwrap(); + )?; } - linked_hash_map + Ok(linked_hash_map) } - Value::Double(float) => env - .new_object("java/lang/Double", "(D)V", &[float.into()]) - .unwrap(), - Value::Boolean(bool) => env - .new_object("java/lang/Boolean", "(Z)V", &[bool.into()]) - .unwrap(), - Value::VerbatimString { format: _, text } => JObject::from(env.new_string(text).unwrap()), + Value::Double(float) => Ok(env.new_object("java/lang/Double", "(D)V", &[float.into()])?), + Value::Boolean(bool) => Ok(env.new_object("java/lang/Boolean", "(Z)V", &[bool.into()])?), + Value::VerbatimString { format: _, text } => Ok(JObject::from(env.new_string(text)?)), Value::BigNumber(_num) => todo!(), Value::Set(array) => { - let set = env.new_object("java/util/HashSet", "()V", &[]).unwrap(); + let set = env.new_object("java/util/HashSet", "()V", &[])?; for elem in array { - let java_value = redis_value_to_java(env, elem, encoding_utf8); + let java_value = redis_value_to_java(env, elem, encoding_utf8)?; env.call_method( &set, "add", "(Ljava/lang/Object;)Z", &[(&java_value).into()], - ) - .unwrap(); + )?; } - set + Ok(set) } Value::Attribute { data: _, @@ -122,8 +128,18 @@ pub extern "system" fn Java_glide_ffi_resolvers_RedisValueResolver_valueFromPoin _class: JClass<'local>, pointer: jlong, ) -> JObject<'local> { - let value = unsafe { Box::from_raw(pointer as *mut Value) }; - redis_value_to_java(&mut env, *value, true) + handle_panics( + move || { + fn f<'a>(env: &mut JNIEnv<'a>, pointer: jlong) -> Result, FFIError> { + let value = unsafe { Box::from_raw(pointer as *mut Value) }; + redis_value_to_java(env, *value, true) + } + let result = f(&mut env, pointer); + handle_errors(&mut env, result) + }, + "valueFromPointer", + JObject::null(), + ) } #[no_mangle] @@ -134,59 +150,55 @@ pub extern "system" fn Java_glide_ffi_resolvers_RedisValueResolver_valueFromPoin _class: JClass<'local>, pointer: jlong, ) -> JObject<'local> { - let value = unsafe { Box::from_raw(pointer as *mut Value) }; - redis_value_to_java(&mut env, *value, false) + handle_panics( + move || { + fn f<'a>(env: &mut JNIEnv<'a>, pointer: jlong) -> Result, FFIError> { + let value = unsafe { Box::from_raw(pointer as *mut Value) }; + redis_value_to_java(env, *value, false) + } + let result = f(&mut env, pointer); + handle_errors(&mut env, result) + }, + "valueFromPointerBinary", + JObject::null(), + ) } #[no_mangle] pub extern "system" fn Java_glide_ffi_resolvers_SocketListenerResolver_startSocketListener< 'local, >( - env: JNIEnv<'local>, + mut env: JNIEnv<'local>, _class: JClass<'local>, ) -> JObject<'local> { - let (tx, rx) = mpsc::channel::>(); + handle_panics( + move || { + fn f<'a>(env: &mut JNIEnv<'a>) -> Result, FFIError> { + let (tx, rx) = mpsc::channel::>(); - start_socket_listener(move |socket_path: Result| { - // Signals that thread has started - let _ = tx.send(socket_path); - }); + start_socket_listener(move |socket_path: Result| { + // Signals that thread has started + let _ = tx.send(socket_path); + }); - // Wait until the thread has started - let socket_path = rx.recv(); + // Wait until the thread has started + let socket_path = rx.recv(); - match socket_path { - Ok(Ok(path)) => env.new_string(path).unwrap().into(), - Ok(Err(error_message)) => { - throw_java_exception(env, error_message); - JObject::null() - } - Err(error) => { - throw_java_exception(env, error.to_string()); - JObject::null() - } - } -} - -fn throw_java_exception(mut env: JNIEnv, message: String) { - let res = env.new_object( - "java/lang/Exception", - "(Ljava/lang/String;)V", - &[(&env.new_string(message.clone()).unwrap()).into()], - ); - - match res { - Ok(res) => { - let _ = env.throw(JThrowable::from(res)); - } - Err(err) => { - error!( - "Failed to create exception with string {}: {}", - message, - err.to_string() - ); - } - }; + match socket_path { + Ok(Ok(path)) => env + .new_string(path) + .map(|p| p.into()) + .map_err(|err| FFIError::UDSError(err.to_string())), + Ok(Err(error_message)) => Err(FFIError::UDSError(error_message)), + Err(error) => Err(FFIError::UDSError(error.to_string())), + } + } + let result = f(&mut env); + handle_errors(&mut env, result) + }, + "startSocketListener", + JObject::null(), + ) } #[no_mangle] @@ -195,9 +207,19 @@ pub extern "system" fn Java_glide_ffi_resolvers_ScriptResolver_storeScript<'loca _class: JClass<'local>, code: JString, ) -> JObject<'local> { - let code_str: String = env.get_string(&code).unwrap().into(); - let hash = glide_core::scripts_container::add_script(&code_str); - JObject::from(env.new_string(hash).unwrap()) + handle_panics( + move || { + fn f<'a>(env: &mut JNIEnv<'a>, code: JString) -> Result, FFIError> { + let code_str: String = env.get_string(&code)?.into(); + let hash = glide_core::scripts_container::add_script(&code_str); + Ok(JObject::from(env.new_string(hash)?)) + } + let result = f(&mut env, code); + handle_errors(&mut env, result) + }, + "storeScript", + JObject::null(), + ) } #[no_mangle] @@ -206,6 +228,76 @@ pub extern "system" fn Java_glide_ffi_resolvers_ScriptResolver_dropScript<'local _class: JClass<'local>, hash: JString, ) { - let hash_str: String = env.get_string(&hash).unwrap().into(); - glide_core::scripts_container::remove_script(&hash_str); + handle_panics( + move || { + fn f<'a>(env: &mut JNIEnv<'a>, hash: JString) -> Result<(), FFIError> { + let hash_str: String = env.get_string(&hash)?.into(); + glide_core::scripts_container::remove_script(&hash_str); + Ok(()) + } + let result = f(&mut env, hash); + handle_errors(&mut env, result) + }, + "dropScript", + (), + ) +} + +fn handle_errors(env: &mut JNIEnv, result: Result) -> Option { + match result { + Ok(value) => Some(value), + Err(err) => { + throw_java_exception(env, err.to_string()); + None + } + } +} + +fn handle_panics Option>( + func: F, + ffi_func_name: &str, + default_value: T, +) -> T { + match std::panic::catch_unwind(func) { + Ok(Some(value)) => value, + Ok(None) => default_value, + Err(_err) => { + /* + env.throw_new( + "java/lang/RuntimeException", + format!("Native function {} panicked", ffi_func_name), + ) + .unwrap_or_else(|err| { + error!( + "Native function {} panicked. Failed to create runtime exception: {}", + ffi_func_name, + err.to_string() + ); + }); + */ + default_value + } + } +} + +fn throw_java_exception(env: &mut JNIEnv, message: String) { + match env.exception_check() { + Ok(true) => (), + Ok(false) => { + env.throw_new("java/lang/Exception", &message) + .unwrap_or_else(|err| { + error!( + "Failed to create exception with string {}: {}", + message, + err.to_string() + ); + }); + } + Err(err) => { + error!( + "Failed to check if an exception is currently being thrown: {}", + err.to_string() + ); + } + } } From af41556ca04020c106ba6f4a56ac91039d4ab076 Mon Sep 17 00:00:00 2001 From: Jonathan Louie Date: Sat, 8 Jun 2024 02:55:04 -0700 Subject: [PATCH 02/12] Fix failing tests --- java/src/lib.rs | 43 +++++++++++++++++++++++++++---------------- 1 file changed, 27 insertions(+), 16 deletions(-) diff --git a/java/src/lib.rs b/java/src/lib.rs index 96c8ea86e1..37f511e7cc 100644 --- a/java/src/lib.rs +++ b/java/src/lib.rs @@ -243,11 +243,32 @@ pub extern "system" fn Java_glide_ffi_resolvers_ScriptResolver_dropScript<'local ) } +enum ExceptionType { + Exception, + RuntimeException, +} + +impl std::fmt::Display for ExceptionType { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + ExceptionType::Exception => write!(f, "{}", "java/lang/Exception"), + ExceptionType::RuntimeException => write!(f, "{}", "java/lang/RuntimeException"), + } + } +} + fn handle_errors(env: &mut JNIEnv, result: Result) -> Option { match result { Ok(value) => Some(value), Err(err) => { - throw_java_exception(env, err.to_string()); + match err { + FFIError::Utf8Error(utf8_error) => throw_java_exception( + env, + ExceptionType::RuntimeException, + utf8_error.to_string(), + ), + error => throw_java_exception(env, ExceptionType::Exception, error.to_string()), + }; None } } @@ -262,29 +283,19 @@ fn handle_panics Option>( Ok(Some(value)) => value, Ok(None) => default_value, Err(_err) => { - /* - env.throw_new( - "java/lang/RuntimeException", - format!("Native function {} panicked", ffi_func_name), - ) - .unwrap_or_else(|err| { - error!( - "Native function {} panicked. Failed to create runtime exception: {}", - ffi_func_name, - err.to_string() - ); - }); - */ + // Following https://github.com/jni-rs/jni-rs/issues/76#issuecomment-363523906 + // and throwing a runtime exception is not feasible here because of https://github.com/jni-rs/jni-rs/issues/432 + error!("Native function {} panicked.", ffi_func_name); default_value } } } -fn throw_java_exception(env: &mut JNIEnv, message: String) { +fn throw_java_exception(env: &mut JNIEnv, exception_type: ExceptionType, message: String) { match env.exception_check() { Ok(true) => (), Ok(false) => { - env.throw_new("java/lang/Exception", &message) + env.throw_new(exception_type.to_string(), &message) .unwrap_or_else(|err| { error!( "Failed to create exception with string {}: {}", From f6b5b17e5deb4466f065dbd5f43b43b36e09837f Mon Sep 17 00:00:00 2001 From: Jonathan Louie Date: Sat, 8 Jun 2024 03:01:02 -0700 Subject: [PATCH 03/12] Address clippy lints --- java/src/lib.rs | 36 ++++++++++++++++-------------------- 1 file changed, 16 insertions(+), 20 deletions(-) diff --git a/java/src/lib.rs b/java/src/lib.rs index 37f511e7cc..ce5d7965ae 100644 --- a/java/src/lib.rs +++ b/java/src/lib.rs @@ -18,29 +18,29 @@ mod ffi_test; pub use ffi_test::*; enum FFIError { - JniError(JNIError), - UDSError(String), - Utf8Error(FromUtf8Error), + Jni(JNIError), + Uds(String), + Utf8(FromUtf8Error), } impl From for FFIError { fn from(value: jni::errors::Error) -> Self { - FFIError::JniError(value) + FFIError::Jni(value) } } impl From for FFIError { fn from(value: FromUtf8Error) -> Self { - FFIError::Utf8Error(value) + FFIError::Utf8(value) } } impl std::fmt::Display for FFIError { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { - FFIError::JniError(err) => write!(f, "{}", err.to_string()), - FFIError::UDSError(err) => write!(f, "{}", err), - FFIError::Utf8Error(err) => write!(f, "{}", err.to_string()), + FFIError::Jni(err) => write!(f, "{}", err), + FFIError::Uds(err) => write!(f, "{}", err), + FFIError::Utf8(err) => write!(f, "{}", err), } } } @@ -55,11 +55,7 @@ fn redis_value_to_java<'local>( Value::Nil => Ok(JObject::null()), Value::SimpleString(str) => Ok(JObject::from(env.new_string(str)?)), Value::Okay => Ok(JObject::from(env.new_string("OK")?)), - Value::Int(num) => Ok(JObject::from(env.new_object( - "java/lang/Long", - "(J)V", - &[num.into()], - )?)), + Value::Int(num) => Ok(env.new_object("java/lang/Long", "(J)V", &[num.into()])?), Value::BulkString(data) => { if encoding_utf8 { let utf8_str = String::from_utf8(data)?; @@ -188,9 +184,9 @@ pub extern "system" fn Java_glide_ffi_resolvers_SocketListenerResolver_startSock Ok(Ok(path)) => env .new_string(path) .map(|p| p.into()) - .map_err(|err| FFIError::UDSError(err.to_string())), - Ok(Err(error_message)) => Err(FFIError::UDSError(error_message)), - Err(error) => Err(FFIError::UDSError(error.to_string())), + .map_err(|err| FFIError::Uds(err.to_string())), + Ok(Err(error_message)) => Err(FFIError::Uds(error_message)), + Err(error) => Err(FFIError::Uds(error.to_string())), } } let result = f(&mut env); @@ -230,7 +226,7 @@ pub extern "system" fn Java_glide_ffi_resolvers_ScriptResolver_dropScript<'local ) { handle_panics( move || { - fn f<'a>(env: &mut JNIEnv<'a>, hash: JString) -> Result<(), FFIError> { + fn f(env: &mut JNIEnv<'_>, hash: JString) -> Result<(), FFIError> { let hash_str: String = env.get_string(&hash)?.into(); glide_core::scripts_container::remove_script(&hash_str); Ok(()) @@ -251,8 +247,8 @@ enum ExceptionType { impl std::fmt::Display for ExceptionType { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { - ExceptionType::Exception => write!(f, "{}", "java/lang/Exception"), - ExceptionType::RuntimeException => write!(f, "{}", "java/lang/RuntimeException"), + ExceptionType::Exception => write!(f, "java/lang/Exception"), + ExceptionType::RuntimeException => write!(f, "java/lang/RuntimeException"), } } } @@ -262,7 +258,7 @@ fn handle_errors(env: &mut JNIEnv, result: Result) -> Option Ok(value) => Some(value), Err(err) => { match err { - FFIError::Utf8Error(utf8_error) => throw_java_exception( + FFIError::Utf8(utf8_error) => throw_java_exception( env, ExceptionType::RuntimeException, utf8_error.to_string(), From de68abd3f56d53aaffc9d24c4faaf74201c8e90f Mon Sep 17 00:00:00 2001 From: Jonathan Louie Date: Sat, 8 Jun 2024 19:46:56 -0700 Subject: [PATCH 04/12] Add tests for error and panic handling --- .../src/test/java/glide/ffi/FfiTest.java | 61 +++++++- java/src/ffi_test.rs | 80 +++++++++- java/src/lib.rs | 138 ++++-------------- 3 files changed, 158 insertions(+), 121 deletions(-) diff --git a/java/client/src/test/java/glide/ffi/FfiTest.java b/java/client/src/test/java/glide/ffi/FfiTest.java index 73c9082c20..0b8a7c833b 100644 --- a/java/client/src/test/java/glide/ffi/FfiTest.java +++ b/java/client/src/test/java/glide/ffi/FfiTest.java @@ -5,11 +5,14 @@ import static org.junit.jupiter.api.Assertions.assertArrayEquals; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; +import glide.ffi.resolvers.NativeUtils; import glide.ffi.resolvers.RedisValueResolver; import java.util.HashMap; import java.util.HashSet; + import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.ValueSource; @@ -17,7 +20,7 @@ public class FfiTest { static { - System.loadLibrary("glide_rs"); + NativeUtils.loadGlideLib(); } public static native long createLeakedNil(); @@ -42,6 +45,12 @@ public class FfiTest { public static native long createLeakedLongSet(long[] value); + public static native long handlePanics(boolean shouldPanic, boolean errorPresent, long value, long defaultValue); + + public static native long handleErrors(boolean isSuccess, long value, long defaultValue); + + public static native void throwException(boolean throwTwice, boolean isRuntimeException, String message); + @Test public void redisValueToJavaValue_Nil() { long ptr = FfiTest.createLeakedNil(); @@ -141,4 +150,54 @@ public void redisValueToJavaValue_Set() { () -> assertTrue(result.contains(2L)), () -> assertEquals(result.size(), 2)); } + + @Test + public void handlePanics_panic() { + long expectedValue = 0L; + long value = FfiTest.handlePanics(true, false, 1L, expectedValue); + assertEquals(expectedValue, value); + } + + @Test + public void handlePanics_returnError() { + long expectedValue = 0L; + long value = FfiTest.handlePanics(false, true, 1L, expectedValue); + assertEquals(expectedValue, value); + } + + @Test + public void handlePanics_returnValue() { + long expectedValue = 2L; + long value = FfiTest.handlePanics(false, false, expectedValue, 0L); + assertEquals(expectedValue, value); + } + + @Test + public void handleErrors_success() { + long expectedValue = 0L; + long value = FfiTest.handleErrors(true, expectedValue, 1L); + assertEquals(expectedValue, value); + } + + @Test + public void handleErrors_error() { + assertThrows(Exception.class, () -> FfiTest.handleErrors(true, 0L, 1L)); + } + + @Test + public void throwException() { + assertThrows(Exception.class, () -> FfiTest.throwException(false, false, "My message")); + } + + @Test + public void throwException_throwTwice() { + assertThrows(Exception.class, () -> FfiTest.throwException(true, false, "My message")); + } + + @Test + public void throwException_throwRuntimeException() { + assertThrows(RuntimeException.class, () -> FfiTest.throwException(false, true, "My message")); + } + + } diff --git a/java/src/ffi_test.rs b/java/src/ffi_test.rs index 199a811392..b297f5ca4a 100644 --- a/java/src/ffi_test.rs +++ b/java/src/ffi_test.rs @@ -1,9 +1,10 @@ +use crate::errors::{handle_errors, handle_panics, throw_java_exception, ExceptionType, FFIError}; /** * Copyright GLIDE-for-Redis Project Contributors - SPDX Identifier: Apache-2.0 */ use jni::{ - objects::{JClass, JLongArray}, - sys::jlong, + objects::{JByteArray, JClass, JLongArray, JString}, + sys::{jboolean, jdouble, jlong}, JNIEnv, }; use redis::Value; @@ -21,7 +22,7 @@ pub extern "system" fn Java_glide_ffi_FfiTest_createLeakedNil<'local>( pub extern "system" fn Java_glide_ffi_FfiTest_createLeakedSimpleString<'local>( mut env: JNIEnv<'local>, _class: JClass<'local>, - value: jni::objects::JString<'local>, + value: JString<'local>, ) -> jlong { let value: String = env.get_string(&value).unwrap().into(); let redis_value = Value::SimpleString(value); @@ -51,7 +52,7 @@ pub extern "system" fn Java_glide_ffi_FfiTest_createLeakedInt<'local>( pub extern "system" fn Java_glide_ffi_FfiTest_createLeakedBulkString<'local>( env: JNIEnv<'local>, _class: JClass<'local>, - value: jni::objects::JByteArray<'local>, + value: JByteArray<'local>, ) -> jlong { let value = env.convert_byte_array(&value).unwrap(); let value = value.into_iter().collect::>(); @@ -88,7 +89,7 @@ pub extern "system" fn Java_glide_ffi_FfiTest_createLeakedMap<'local>( pub extern "system" fn Java_glide_ffi_FfiTest_createLeakedDouble<'local>( _env: JNIEnv<'local>, _class: JClass<'local>, - value: jni::sys::jdouble, + value: jdouble, ) -> jlong { let redis_value = Value::Double(value.into()); Box::leak(Box::new(redis_value)) as *mut Value as jlong @@ -98,7 +99,7 @@ pub extern "system" fn Java_glide_ffi_FfiTest_createLeakedDouble<'local>( pub extern "system" fn Java_glide_ffi_FfiTest_createLeakedBoolean<'local>( _env: JNIEnv<'local>, _class: JClass<'local>, - value: jni::sys::jboolean, + value: jboolean, ) -> jlong { let redis_value = Value::Boolean(value != 0); Box::leak(Box::new(redis_value)) as *mut Value as jlong @@ -108,7 +109,7 @@ pub extern "system" fn Java_glide_ffi_FfiTest_createLeakedBoolean<'local>( pub extern "system" fn Java_glide_ffi_FfiTest_createLeakedVerbatimString<'local>( mut env: JNIEnv<'local>, _class: JClass<'local>, - value: jni::objects::JString<'local>, + value: JString<'local>, ) -> jlong { use redis::VerbatimFormat; let value: String = env.get_string(&value).unwrap().into(); @@ -144,3 +145,68 @@ fn java_long_array_to_value<'local>( .map(|value| Value::Int(*value)) .collect::>() } + +#[no_mangle] +pub extern "system" fn Java_glide_ffi_FfiTest_handlePanics<'local>( + mut env: JNIEnv<'local>, + _class: JClass<'local>, + should_panic: jboolean, + error_present: jboolean, + value: jlong, + default_value: jlong, +) -> jlong { + let should_panic = should_panic != 0; + let error_present = error_present != 0; + handle_panics( + || { + if should_panic { + panic!("Panicking") + } else if error_present { + None + } else { + Some(value) + } + }, + "handlePanics", + default_value, + ) +} + +#[no_mangle] +pub extern "system" fn Java_glide_ffi_FfiTest_handleErrors<'local>( + mut env: JNIEnv<'local>, + _class: JClass<'local>, + is_success: jboolean, + value: jlong, + default_value: jlong, +) -> jlong { + let is_success = is_success != 0; + let error = FFIError::Uds("Error starting socket listener".to_string()); + let result = if is_success { Ok(value) } else { Err(error) }; + handle_errors(&mut env, result).unwrap_or(default_value) +} + +#[no_mangle] +pub extern "system" fn Java_glide_ffi_FfiTest_throwException<'local>( + mut env: JNIEnv<'local>, + _class: JClass<'local>, + throw_twice: jboolean, + is_runtime_exception: jboolean, + message: JString<'local>, +) { + let throw_twice = throw_twice != 0; + let is_runtime_exception = is_runtime_exception != 0; + + let exception_type = if is_runtime_exception { + ExceptionType::RuntimeException + } else { + ExceptionType::Exception + }; + + let message: String = env.get_string(&message).unwrap().into(); + throw_java_exception(&mut env, exception_type, &message); + + if throw_twice { + throw_java_exception(&mut env, exception_type, &message); + } +} diff --git a/java/src/lib.rs b/java/src/lib.rs index ce5d7965ae..fdcff786dd 100644 --- a/java/src/lib.rs +++ b/java/src/lib.rs @@ -1,50 +1,23 @@ /** * Copyright GLIDE-for-Redis Project Contributors - SPDX Identifier: Apache-2.0 */ -use glide_core::start_socket_listener; +use glide_core::start_socket_listener as start_socket_listener_core; -use jni::errors::Error as JNIError; use jni::objects::{JClass, JObject, JObjectArray, JString}; use jni::sys::jlong; use jni::JNIEnv; -use log::error; use redis::Value; -use std::string::FromUtf8Error; use std::sync::mpsc; +mod errors; + +use errors::{handle_errors, handle_panics, FFIError}; + #[cfg(ffi_test)] mod ffi_test; #[cfg(ffi_test)] pub use ffi_test::*; -enum FFIError { - Jni(JNIError), - Uds(String), - Utf8(FromUtf8Error), -} - -impl From for FFIError { - fn from(value: jni::errors::Error) -> Self { - FFIError::Jni(value) - } -} - -impl From for FFIError { - fn from(value: FromUtf8Error) -> Self { - FFIError::Utf8(value) - } -} - -impl std::fmt::Display for FFIError { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - match self { - FFIError::Jni(err) => write!(f, "{}", err), - FFIError::Uds(err) => write!(f, "{}", err), - FFIError::Utf8(err) => write!(f, "{}", err), - } - } -} - // TODO: Consider caching method IDs here in a static variable (might need RwLock to mutate) fn redis_value_to_java<'local>( env: &mut JNIEnv<'local>, @@ -126,11 +99,14 @@ pub extern "system" fn Java_glide_ffi_resolvers_RedisValueResolver_valueFromPoin ) -> JObject<'local> { handle_panics( move || { - fn f<'a>(env: &mut JNIEnv<'a>, pointer: jlong) -> Result, FFIError> { + fn value_from_pointer<'a>( + env: &mut JNIEnv<'a>, + pointer: jlong, + ) -> Result, FFIError> { let value = unsafe { Box::from_raw(pointer as *mut Value) }; redis_value_to_java(env, *value, true) } - let result = f(&mut env, pointer); + let result = value_from_pointer(&mut env, pointer); handle_errors(&mut env, result) }, "valueFromPointer", @@ -148,11 +124,14 @@ pub extern "system" fn Java_glide_ffi_resolvers_RedisValueResolver_valueFromPoin ) -> JObject<'local> { handle_panics( move || { - fn f<'a>(env: &mut JNIEnv<'a>, pointer: jlong) -> Result, FFIError> { + fn value_from_pointer_binary<'a>( + env: &mut JNIEnv<'a>, + pointer: jlong, + ) -> Result, FFIError> { let value = unsafe { Box::from_raw(pointer as *mut Value) }; redis_value_to_java(env, *value, false) } - let result = f(&mut env, pointer); + let result = value_from_pointer_binary(&mut env, pointer); handle_errors(&mut env, result) }, "valueFromPointerBinary", @@ -169,10 +148,10 @@ pub extern "system" fn Java_glide_ffi_resolvers_SocketListenerResolver_startSock ) -> JObject<'local> { handle_panics( move || { - fn f<'a>(env: &mut JNIEnv<'a>) -> Result, FFIError> { + fn start_socket_listener<'a>(env: &mut JNIEnv<'a>) -> Result, FFIError> { let (tx, rx) = mpsc::channel::>(); - start_socket_listener(move |socket_path: Result| { + start_socket_listener_core(move |socket_path: Result| { // Signals that thread has started let _ = tx.send(socket_path); }); @@ -189,7 +168,7 @@ pub extern "system" fn Java_glide_ffi_resolvers_SocketListenerResolver_startSock Err(error) => Err(FFIError::Uds(error.to_string())), } } - let result = f(&mut env); + let result = start_socket_listener(&mut env); handle_errors(&mut env, result) }, "startSocketListener", @@ -205,12 +184,15 @@ pub extern "system" fn Java_glide_ffi_resolvers_ScriptResolver_storeScript<'loca ) -> JObject<'local> { handle_panics( move || { - fn f<'a>(env: &mut JNIEnv<'a>, code: JString) -> Result, FFIError> { + fn store_script<'a>( + env: &mut JNIEnv<'a>, + code: JString, + ) -> Result, FFIError> { let code_str: String = env.get_string(&code)?.into(); let hash = glide_core::scripts_container::add_script(&code_str); Ok(JObject::from(env.new_string(hash)?)) } - let result = f(&mut env, code); + let result = store_script(&mut env, code); handle_errors(&mut env, result) }, "storeScript", @@ -226,85 +208,15 @@ pub extern "system" fn Java_glide_ffi_resolvers_ScriptResolver_dropScript<'local ) { handle_panics( move || { - fn f(env: &mut JNIEnv<'_>, hash: JString) -> Result<(), FFIError> { + fn drop_script(env: &mut JNIEnv<'_>, hash: JString) -> Result<(), FFIError> { let hash_str: String = env.get_string(&hash)?.into(); glide_core::scripts_container::remove_script(&hash_str); Ok(()) } - let result = f(&mut env, hash); + let result = drop_script(&mut env, hash); handle_errors(&mut env, result) }, "dropScript", (), ) } - -enum ExceptionType { - Exception, - RuntimeException, -} - -impl std::fmt::Display for ExceptionType { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - match self { - ExceptionType::Exception => write!(f, "java/lang/Exception"), - ExceptionType::RuntimeException => write!(f, "java/lang/RuntimeException"), - } - } -} - -fn handle_errors(env: &mut JNIEnv, result: Result) -> Option { - match result { - Ok(value) => Some(value), - Err(err) => { - match err { - FFIError::Utf8(utf8_error) => throw_java_exception( - env, - ExceptionType::RuntimeException, - utf8_error.to_string(), - ), - error => throw_java_exception(env, ExceptionType::Exception, error.to_string()), - }; - None - } - } -} - -fn handle_panics Option>( - func: F, - ffi_func_name: &str, - default_value: T, -) -> T { - match std::panic::catch_unwind(func) { - Ok(Some(value)) => value, - Ok(None) => default_value, - Err(_err) => { - // Following https://github.com/jni-rs/jni-rs/issues/76#issuecomment-363523906 - // and throwing a runtime exception is not feasible here because of https://github.com/jni-rs/jni-rs/issues/432 - error!("Native function {} panicked.", ffi_func_name); - default_value - } - } -} - -fn throw_java_exception(env: &mut JNIEnv, exception_type: ExceptionType, message: String) { - match env.exception_check() { - Ok(true) => (), - Ok(false) => { - env.throw_new(exception_type.to_string(), &message) - .unwrap_or_else(|err| { - error!( - "Failed to create exception with string {}: {}", - message, - err.to_string() - ); - }); - } - Err(err) => { - error!( - "Failed to check if an exception is currently being thrown: {}", - err.to_string() - ); - } - } -} From bcdb92a7e65af69c4d2ceda79438bca652e8ce22 Mon Sep 17 00:00:00 2001 From: Jonathan Louie Date: Sat, 8 Jun 2024 19:47:23 -0700 Subject: [PATCH 05/12] Add missing errors module --- java/src/errors.rs | 105 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 105 insertions(+) create mode 100644 java/src/errors.rs diff --git a/java/src/errors.rs b/java/src/errors.rs new file mode 100644 index 0000000000..5c21be93c7 --- /dev/null +++ b/java/src/errors.rs @@ -0,0 +1,105 @@ +use jni::{errors::Error as JNIError, JNIEnv}; +use log::error; +use std::string::FromUtf8Error; + +pub enum FFIError { + Jni(JNIError), + Uds(String), + Utf8(FromUtf8Error), +} + +impl From for FFIError { + fn from(value: jni::errors::Error) -> Self { + FFIError::Jni(value) + } +} + +impl From for FFIError { + fn from(value: FromUtf8Error) -> Self { + FFIError::Utf8(value) + } +} + +impl std::fmt::Display for FFIError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + FFIError::Jni(err) => write!(f, "{}", err), + FFIError::Uds(err) => write!(f, "{}", err), + FFIError::Utf8(err) => write!(f, "{}", err), + } + } +} + +#[derive(Copy, Clone)] +pub enum ExceptionType { + Exception, + RuntimeException, +} + +impl std::fmt::Display for ExceptionType { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + ExceptionType::Exception => write!(f, "java/lang/Exception"), + ExceptionType::RuntimeException => write!(f, "java/lang/RuntimeException"), + } + } +} + +pub fn handle_errors(env: &mut JNIEnv, result: Result) -> Option { + match result { + Ok(value) => Some(value), + Err(err) => { + match err { + FFIError::Utf8(utf8_error) => throw_java_exception( + env, + ExceptionType::RuntimeException, + &utf8_error.to_string(), + ), + error => throw_java_exception(env, ExceptionType::Exception, &error.to_string()), + }; + // Return `None` because we need to still return a value after throwing. + // This signals to `handle_panics` that we need to return the default value. + None + } + } +} + +// `func` returns an `Option` because this is intended to wrap the output of `handle_errors` +pub fn handle_panics Option>( + func: F, + ffi_func_name: &str, + default_value: T, +) -> T { + match std::panic::catch_unwind(func) { + Ok(Some(value)) => value, + Ok(None) => default_value, + Err(_err) => { + // Following https://github.com/jni-rs/jni-rs/issues/76#issuecomment-363523906 + // and throwing a runtime exception is not feasible here because of https://github.com/jni-rs/jni-rs/issues/432 + error!("Native function {} panicked.", ffi_func_name); + default_value + } + } +} + +pub fn throw_java_exception(env: &mut JNIEnv, exception_type: ExceptionType, message: &str) { + match env.exception_check() { + Ok(true) => (), + Ok(false) => { + env.throw_new(exception_type.to_string(), &message) + .unwrap_or_else(|err| { + error!( + "Failed to create exception with string {}: {}", + message, + err.to_string() + ); + }); + } + Err(err) => { + error!( + "Failed to check if an exception is currently being thrown: {}", + err.to_string() + ); + } + } +} From 7c297ad4832418e782d368d2f0ae1a4b2cf1e073 Mon Sep 17 00:00:00 2001 From: Jonathan Louie Date: Tue, 11 Jun 2024 14:08:16 -0700 Subject: [PATCH 06/12] Fix clippy lint --- java/src/errors.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/src/errors.rs b/java/src/errors.rs index 5c21be93c7..69c8bf8e42 100644 --- a/java/src/errors.rs +++ b/java/src/errors.rs @@ -86,7 +86,7 @@ pub fn throw_java_exception(env: &mut JNIEnv, exception_type: ExceptionType, mes match env.exception_check() { Ok(true) => (), Ok(false) => { - env.throw_new(exception_type.to_string(), &message) + env.throw_new(exception_type.to_string(), message) .unwrap_or_else(|err| { error!( "Failed to create exception with string {}: {}", From 87cf9cdeee584d203f7a4f558529dd53d8b8f886 Mon Sep 17 00:00:00 2001 From: Jonathan Louie Date: Tue, 11 Jun 2024 16:45:40 -0700 Subject: [PATCH 07/12] Fix FFI tests --- java/client/build.gradle | 3 ++- java/client/src/test/java/glide/ffi/FfiTest.java | 6 ++---- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/java/client/build.gradle b/java/client/build.gradle index 13fccbfea6..5bcde0c213 100644 --- a/java/client/build.gradle +++ b/java/client/build.gradle @@ -158,7 +158,7 @@ jar.dependsOn('copyNativeLib') copyNativeLib.dependsOn('buildRustRelease') compileTestJava.dependsOn('publishToMavenLocal') test.dependsOn('buildRust') -testFfi.dependsOn('buildRust') +testFfi.dependsOn(['buildRust', 'copyNativeLib']) test { exclude "glide/ffi/FfiTest.class" @@ -219,6 +219,7 @@ tasks.withType(Test) { events "started", "skipped", "passed", "failed" showStandardStreams true } + jvmArgs "-Djava.library.path=${projectDir}/../target/debug" } jar { diff --git a/java/client/src/test/java/glide/ffi/FfiTest.java b/java/client/src/test/java/glide/ffi/FfiTest.java index 0b8a7c833b..33e0ed1643 100644 --- a/java/client/src/test/java/glide/ffi/FfiTest.java +++ b/java/client/src/test/java/glide/ffi/FfiTest.java @@ -20,7 +20,7 @@ public class FfiTest { static { - NativeUtils.loadGlideLib(); + System.loadLibrary("glide_rs"); } public static native long createLeakedNil(); @@ -181,7 +181,7 @@ public void handleErrors_success() { @Test public void handleErrors_error() { - assertThrows(Exception.class, () -> FfiTest.handleErrors(true, 0L, 1L)); + assertThrows(Exception.class, () -> FfiTest.handleErrors(false, 0L, 1L)); } @Test @@ -198,6 +198,4 @@ public void throwException_throwTwice() { public void throwException_throwRuntimeException() { assertThrows(RuntimeException.class, () -> FfiTest.throwException(false, true, "My message")); } - - } From e430a45761769aa6d38324880b3b1b04af3abe86 Mon Sep 17 00:00:00 2001 From: Jonathan Louie Date: Tue, 11 Jun 2024 16:57:57 -0700 Subject: [PATCH 08/12] Apply Spotless --- java/client/src/test/java/glide/ffi/FfiTest.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/java/client/src/test/java/glide/ffi/FfiTest.java b/java/client/src/test/java/glide/ffi/FfiTest.java index 33e0ed1643..7ab1f328f8 100644 --- a/java/client/src/test/java/glide/ffi/FfiTest.java +++ b/java/client/src/test/java/glide/ffi/FfiTest.java @@ -8,11 +8,9 @@ import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; -import glide.ffi.resolvers.NativeUtils; import glide.ffi.resolvers.RedisValueResolver; import java.util.HashMap; import java.util.HashSet; - import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.ValueSource; @@ -45,11 +43,13 @@ public class FfiTest { public static native long createLeakedLongSet(long[] value); - public static native long handlePanics(boolean shouldPanic, boolean errorPresent, long value, long defaultValue); + public static native long handlePanics( + boolean shouldPanic, boolean errorPresent, long value, long defaultValue); public static native long handleErrors(boolean isSuccess, long value, long defaultValue); - public static native void throwException(boolean throwTwice, boolean isRuntimeException, String message); + public static native void throwException( + boolean throwTwice, boolean isRuntimeException, String message); @Test public void redisValueToJavaValue_Nil() { From d890c8477a1eab2ccbb0b5bfeed4a35ae8bc8c2f Mon Sep 17 00:00:00 2001 From: Jonathan Louie Date: Tue, 11 Jun 2024 17:21:40 -0700 Subject: [PATCH 09/12] Fix some minor issue I forgot about --- java/client/build.gradle | 2 +- java/src/errors.rs | 3 +++ java/src/ffi_test.rs | 4 ++-- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/java/client/build.gradle b/java/client/build.gradle index 5bcde0c213..673c596c11 100644 --- a/java/client/build.gradle +++ b/java/client/build.gradle @@ -158,7 +158,7 @@ jar.dependsOn('copyNativeLib') copyNativeLib.dependsOn('buildRustRelease') compileTestJava.dependsOn('publishToMavenLocal') test.dependsOn('buildRust') -testFfi.dependsOn(['buildRust', 'copyNativeLib']) +testFfi.dependsOn('buildRust') test { exclude "glide/ffi/FfiTest.class" diff --git a/java/src/errors.rs b/java/src/errors.rs index 69c8bf8e42..b36d02ff2e 100644 --- a/java/src/errors.rs +++ b/java/src/errors.rs @@ -1,3 +1,6 @@ +/** + * Copyright GLIDE-for-Redis Project Contributors - SPDX Identifier: Apache-2.0 + */ use jni::{errors::Error as JNIError, JNIEnv}; use log::error; use std::string::FromUtf8Error; diff --git a/java/src/ffi_test.rs b/java/src/ffi_test.rs index b297f5ca4a..862cfdf1d3 100644 --- a/java/src/ffi_test.rs +++ b/java/src/ffi_test.rs @@ -1,7 +1,7 @@ -use crate::errors::{handle_errors, handle_panics, throw_java_exception, ExceptionType, FFIError}; /** * Copyright GLIDE-for-Redis Project Contributors - SPDX Identifier: Apache-2.0 */ +use crate::errors::{handle_errors, handle_panics, throw_java_exception, ExceptionType, FFIError}; use jni::{ objects::{JByteArray, JClass, JLongArray, JString}, sys::{jboolean, jdouble, jlong}, @@ -148,7 +148,7 @@ fn java_long_array_to_value<'local>( #[no_mangle] pub extern "system" fn Java_glide_ffi_FfiTest_handlePanics<'local>( - mut env: JNIEnv<'local>, + _env: JNIEnv<'local>, _class: JClass<'local>, should_panic: jboolean, error_present: jboolean, From 7ca187c7c3e51a28dc4e94cddc75f7556982f0f6 Mon Sep 17 00:00:00 2001 From: Jonathan Louie Date: Mon, 17 Jun 2024 08:45:49 -0700 Subject: [PATCH 10/12] Add some comments --- java/client/build.gradle | 1 + java/client/src/test/java/glide/ffi/FfiTest.java | 3 +++ java/src/errors.rs | 5 ++++- 3 files changed, 8 insertions(+), 1 deletion(-) diff --git a/java/client/build.gradle b/java/client/build.gradle index 673c596c11..f8ab99b25d 100644 --- a/java/client/build.gradle +++ b/java/client/build.gradle @@ -219,6 +219,7 @@ tasks.withType(Test) { events "started", "skipped", "passed", "failed" showStandardStreams true } + // This is needed for the FFI tests jvmArgs "-Djava.library.path=${projectDir}/../target/debug" } diff --git a/java/client/src/test/java/glide/ffi/FfiTest.java b/java/client/src/test/java/glide/ffi/FfiTest.java index 7ab1f328f8..ec2bc84ebd 100644 --- a/java/client/src/test/java/glide/ffi/FfiTest.java +++ b/java/client/src/test/java/glide/ffi/FfiTest.java @@ -43,11 +43,14 @@ public class FfiTest { public static native long createLeakedLongSet(long[] value); + // This tests that panics do not cross the FFI boundary and an exception is thrown if a panic is caught public static native long handlePanics( boolean shouldPanic, boolean errorPresent, long value, long defaultValue); + // This tests that Rust errors are properly converted into Java exceptions and thrown public static native long handleErrors(boolean isSuccess, long value, long defaultValue); + // This tests that a Java exception is properly thrown across the FFI boundary public static native void throwException( boolean throwTwice, boolean isRuntimeException, String message); diff --git a/java/src/errors.rs b/java/src/errors.rs index b36d02ff2e..618fc26b5f 100644 --- a/java/src/errors.rs +++ b/java/src/errors.rs @@ -48,6 +48,7 @@ impl std::fmt::Display for ExceptionType { } } +// This handles `FFIError`s by converting them to Java exceptions and throwing them. pub fn handle_errors(env: &mut JNIEnv, result: Result) -> Option { match result { Ok(value) => Some(value), @@ -67,7 +68,9 @@ pub fn handle_errors(env: &mut JNIEnv, result: Result) -> Option } } -// `func` returns an `Option` because this is intended to wrap the output of `handle_errors` +// This function handles Rust panics by converting them into Java exceptions and throwing them. +// `func` returns an `Option` because this is intended to wrap the output of `handle_errors`. +// When an exception is thrown, a value still need to be returned, so a `default_value` is used for this. pub fn handle_panics Option>( func: F, ffi_func_name: &str, From 88f1f466a5877676878d7990bcca8cc999950a80 Mon Sep 17 00:00:00 2001 From: Jonathan Louie Date: Mon, 17 Jun 2024 08:46:53 -0700 Subject: [PATCH 11/12] Apply Spotless --- java/client/src/test/java/glide/ffi/FfiTest.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/java/client/src/test/java/glide/ffi/FfiTest.java b/java/client/src/test/java/glide/ffi/FfiTest.java index ec2bc84ebd..f5d25f0db8 100644 --- a/java/client/src/test/java/glide/ffi/FfiTest.java +++ b/java/client/src/test/java/glide/ffi/FfiTest.java @@ -43,7 +43,8 @@ public class FfiTest { public static native long createLeakedLongSet(long[] value); - // This tests that panics do not cross the FFI boundary and an exception is thrown if a panic is caught + // This tests that panics do not cross the FFI boundary and an exception is thrown if a panic is + // caught public static native long handlePanics( boolean shouldPanic, boolean errorPresent, long value, long defaultValue); From ee09467092ddb51a9fcfab4ee67c19e528fb0d63 Mon Sep 17 00:00:00 2001 From: Jonathan Louie Date: Thu, 27 Jun 2024 15:04:36 -0700 Subject: [PATCH 12/12] Make handle_panics return Option instead --- java/src/errors.rs | 11 ++++------- java/src/ffi_test.rs | 2 +- java/src/lib.rs | 10 +++++----- 3 files changed, 10 insertions(+), 13 deletions(-) diff --git a/java/src/errors.rs b/java/src/errors.rs index 618fc26b5f..031a898551 100644 --- a/java/src/errors.rs +++ b/java/src/errors.rs @@ -62,7 +62,7 @@ pub fn handle_errors(env: &mut JNIEnv, result: Result) -> Option error => throw_java_exception(env, ExceptionType::Exception, &error.to_string()), }; // Return `None` because we need to still return a value after throwing. - // This signals to `handle_panics` that we need to return the default value. + // This signals to the caller that we need to return the default value. None } } @@ -70,20 +70,17 @@ pub fn handle_errors(env: &mut JNIEnv, result: Result) -> Option // This function handles Rust panics by converting them into Java exceptions and throwing them. // `func` returns an `Option` because this is intended to wrap the output of `handle_errors`. -// When an exception is thrown, a value still need to be returned, so a `default_value` is used for this. pub fn handle_panics Option>( func: F, ffi_func_name: &str, - default_value: T, -) -> T { +) -> Option { match std::panic::catch_unwind(func) { - Ok(Some(value)) => value, - Ok(None) => default_value, + Ok(value) => value, Err(_err) => { // Following https://github.com/jni-rs/jni-rs/issues/76#issuecomment-363523906 // and throwing a runtime exception is not feasible here because of https://github.com/jni-rs/jni-rs/issues/432 error!("Native function {} panicked.", ffi_func_name); - default_value + None } } } diff --git a/java/src/ffi_test.rs b/java/src/ffi_test.rs index 862cfdf1d3..c0b71afe06 100644 --- a/java/src/ffi_test.rs +++ b/java/src/ffi_test.rs @@ -168,8 +168,8 @@ pub extern "system" fn Java_glide_ffi_FfiTest_handlePanics<'local>( } }, "handlePanics", - default_value, ) + .unwrap_or(default_value) } #[no_mangle] diff --git a/java/src/lib.rs b/java/src/lib.rs index def7584950..cf5bdee5aa 100644 --- a/java/src/lib.rs +++ b/java/src/lib.rs @@ -110,8 +110,8 @@ pub extern "system" fn Java_glide_ffi_resolvers_RedisValueResolver_valueFromPoin handle_errors(&mut env, result) }, "valueFromPointer", - JObject::null(), ) + .unwrap_or(JObject::null()) } #[no_mangle] @@ -135,8 +135,8 @@ pub extern "system" fn Java_glide_ffi_resolvers_RedisValueResolver_valueFromPoin handle_errors(&mut env, result) }, "valueFromPointerBinary", - JObject::null(), ) + .unwrap_or(JObject::null()) } #[no_mangle] @@ -172,8 +172,8 @@ pub extern "system" fn Java_glide_ffi_resolvers_SocketListenerResolver_startSock handle_errors(&mut env, result) }, "startSocketListener", - JObject::null(), ) + .unwrap_or(JObject::null()) } #[no_mangle] @@ -196,8 +196,8 @@ pub extern "system" fn Java_glide_ffi_resolvers_ScriptResolver_storeScript<'loca handle_errors(&mut env, result) }, "storeScript", - JObject::null(), ) + .unwrap_or(JObject::null()) } #[no_mangle] @@ -217,6 +217,6 @@ pub extern "system" fn Java_glide_ffi_resolvers_ScriptResolver_dropScript<'local handle_errors(&mut env, result) }, "dropScript", - (), ) + .unwrap_or(()) }