Skip to content

Commit

Permalink
Java: Handle panics and errors in the Java FFI layer (valkey-io#1601)
Browse files Browse the repository at this point in the history
* Restructure Java FFI layer to handle errors properly

* Fix failing tests

* Address clippy lints

* Add tests for error and panic handling

* Add missing errors module

* Fix clippy lint

* Fix FFI tests

* Apply Spotless

* Fix some minor issue I forgot about

* Add some comments

* Apply Spotless

* Make handle_panics return Option<T> instead
  • Loading branch information
jonathanl-bq authored and cyip10 committed Jul 16, 2024
1 parent 6163a31 commit 977e680
Show file tree
Hide file tree
Showing 5 changed files with 363 additions and 115 deletions.
2 changes: 2 additions & 0 deletions java/client/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,8 @@ tasks.withType(Test) {
events "started", "skipped", "passed", "failed"
showStandardStreams true
}
// This is needed for the FFI tests
jvmArgs "-Djava.library.path=${projectDir}/../target/debug"
}

jar {
Expand Down
61 changes: 61 additions & 0 deletions java/client/src/test/java/glide/ffi/FfiTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
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.RedisValueResolver;
Expand Down Expand Up @@ -42,6 +43,18 @@ 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);

@Test
public void redisValueToJavaValue_Nil() {
long ptr = FfiTest.createLeakedNil();
Expand Down Expand Up @@ -141,4 +154,52 @@ 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(false, 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"));
}
}
108 changes: 108 additions & 0 deletions java/src/errors.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
/**
* 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;

pub enum FFIError {
Jni(JNIError),
Uds(String),
Utf8(FromUtf8Error),
}

impl From<jni::errors::Error> for FFIError {
fn from(value: jni::errors::Error) -> Self {
FFIError::Jni(value)
}
}

impl From<FromUtf8Error> 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"),
}
}
}

// This handles `FFIError`s by converting them to Java exceptions and throwing them.
pub fn handle_errors<T>(env: &mut JNIEnv, result: Result<T, FFIError>) -> Option<T> {
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 the caller that we need to return the default value.
None
}
}
}

// This function handles Rust panics by converting them into Java exceptions and throwing them.
// `func` returns an `Option<T>` because this is intended to wrap the output of `handle_errors`.
pub fn handle_panics<T, F: std::panic::UnwindSafe + FnOnce() -> Option<T>>(
func: F,
ffi_func_name: &str,
) -> Option<T> {
match std::panic::catch_unwind(func) {
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);
None
}
}
}

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()
);
}
}
}
80 changes: 73 additions & 7 deletions java/src/ffi_test.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
/**
* Copyright Valkey GLIDE Project Contributors - SPDX Identifier: Apache-2.0
*/
use crate::errors::{handle_errors, handle_panics, throw_java_exception, ExceptionType, FFIError};
use jni::{
objects::{JClass, JLongArray},
sys::jlong,
objects::{JByteArray, JClass, JLongArray, JString},
sys::{jboolean, jdouble, jlong},
JNIEnv,
};
use redis::Value;
Expand All @@ -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);
Expand Down Expand Up @@ -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::<Vec<u8>>();
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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();
Expand Down Expand Up @@ -144,3 +145,68 @@ fn java_long_array_to_value<'local>(
.map(|value| Value::Int(*value))
.collect::<Vec<Value>>()
}

#[no_mangle]
pub extern "system" fn Java_glide_ffi_FfiTest_handlePanics<'local>(
_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",
)
.unwrap_or(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);
}
}
Loading

0 comments on commit 977e680

Please sign in to comment.