aws / glide-for-redis

Apache License 2.0
152 stars 29 forks source link

Java: Add Logger #1422

Open jonathanl-bq opened 1 month ago

jonathanl-bq commented 1 month ago

Issue #, if available: N/A

The Logger implementation here is based on the Python client's implementation, with some relatively small Java/JNI specific changes.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

jonathanl-bq commented 3 weeks ago

IMPORTANT

I see that you use panic and unwrap throughout the lib.rs file, which might crash the user's application, which is definitely something that our client shouldn't do. Have you tested how these unwrap/panics points behave? In node and python we use Results to return errors, both on the logger and in the value_from_pointer function. Please check what is the correct way in JNI to handle exceptions, or if it has some JResult object, and test to see how an application would behave when an exception is thrown in the rust code. See jni-rs/jni-rs#485, might be helpful

I haven't witnessed those SIGABRT messages on the panics so far. It just prints the backtrace to stdout. I can certainly add the catch_unwind and throw Java Exceptions though, just to be safe. The example code in the jni-rs repo does the unwraps, which is what I was following, but maybe the example code isn't very well written.

barshaul commented 3 weeks ago

Yes please add test to check that we handle properly panics in rust

jonathanl-bq commented 3 days ago

Panic handling is coming in a separate PR that overhauls how we handle errors in the FFI layer in general.