jni-rs icon indicating copy to clipboard operation
jni-rs copied to clipboard

Display Java exception messages in `Error::JavaException` error message

Open argv-minus-one opened this issue 9 months ago • 1 comments

Overview

(Currently a draft PR because this is based on #478, which isn't yet merged.)

This PR fixes #496 by capturing information about Java exceptions in Error::JavaException, and displaying that information as part of the error message.

It accomplishes this by storing a GlobalRef to the exception object, then calling its toString() method (or, if that also throws an exception, getClass().getName()) to produce an exception message.

Other changes

In addition to the above, this PR makes two other changes, in separate commits.

This PR adds the method AttachGuard::was_already_attached, which returns true iff the thread was already attached when JavaVM::attach_current_thread was called. This information is used to decide whether to log a warning about the thread not being attached when the impl Display for JavaException is invoked. The GlobalRef destructor is changed to use this new method as well.

This PR makes JNIEnv::exception_occurred infallibly return Option<JThrowable>. It wasn't made infallible in #478 like the other exception-checking JNIEnv methods, and my exception-capturing code needed it to be infallible.

Breaking changes

Error::JavaException is now a tuple variant instead of a unit variant, which is a breaking change.

Open questions

Should we do this at all? On my machine, this adds an overhead of at least 2µs every time a JNIEnv call (other than throw or throw_new) results in a Java exception being thrown.

Should the exception message be captured eagerly or lazily? This code is currently lazy: it creates a GlobalRef to the exception object, and only calls its toString() if Display::fmt is called. It could be changed to eagerly call toString() when the exception is thrown instead. Here's how they compare:

  • Lazy mode saves 2µs if Display::fmt is called, but costs 2µs if Display::fmt is not called (i.e. the exception is thrown, but the exception message is never displayed, at least not by Rust code).
  • Lazy mode costs an additional 86µs if Display::fmt and Drop::drop are called on a thread that's not currently attached to the JVM. This is the same problem as dropping a GlobalRef on an unattached thread, but doubled: once for Display::fmt, once for Drop::drop.

Since I expect most Java exceptions will never be displayed by Rust code, only passed back to Java, lazy mode seems like the best approach.

Should users be allowed to suppress exception capture? This removes the benefit of seeing exception messages, but reduces overhead to almost zero. If the answer to this question is yes, should it be by a function call, an environment variable, or both?

Am I overthinking this? Wouldn't be the first time I overthought a performance edge case… 😅

Benchmark results

test tests::exception_capture_eager_nostringify_nosuppress    ... bench:       4,097 ns/iter (+/- 60)
test tests::exception_capture_eager_nostringify_suppress      ... bench:          20 ns/iter (+/- 0)
test tests::exception_capture_eager_stringify_nosuppress      ... bench:       4,135 ns/iter (+/- 516)
test tests::exception_capture_eager_stringify_suppress        ... bench:          48 ns/iter (+/- 3)
test tests::exception_capture_lazy_nostringify_nosuppress     ... bench:       2,195 ns/iter (+/- 117)
test tests::exception_capture_lazy_nostringify_suppress       ... bench:          20 ns/iter (+/- 0)
test tests::exception_capture_lazy_stringify_nosuppress       ... bench:       6,410 ns/iter (+/- 116)
test tests::exception_capture_lazy_stringify_suppress         ... bench:          48 ns/iter (+/- 1)
test tests::exception_capture_send_baseline                   ... bench:      29,852 ns/iter (+/- 10,037)
test tests::exception_capture_send_withattach                 ... bench:     116,000 ns/iter (+/- 26,477)

Benchmark function names contain the following keywords:

  • eager vs lazy:
    • eager: Calls the exception's toString() immediately when it is thrown.
    • lazy: Calls the exception's toString() only if Display::fmt is called.
  • stringify vs nostringify:
    • stringify: Display::fmt is called.
    • nostringify: The JavaException is silently discarded without ever calling Display::fmt.
  • suppress vs nosuppress:
    • suppress: toString() is not called and GlobalRef is not created. Display::fmt produces a dummy exception message instead of the real one.
    • nosuppress: toString() and/or GlobalRef is called/created. Display::fmt produces the real exception message.
  • send_baseline and send_withattach:
    • send_baseline: Sends the JavaException to another thread using an MPSC channel, receives it back with another MPSC channel, and then stringifies it as in lazy_stringify_nosuppress. This benchmark is the baseline for send_withattach.
    • send_withattach: Sends the JavaException to another thread using an MPSC channel, which stringifies and drops it, then sends back the resulting string using another MPSC channel. This demonstrates the severe overhead of stringifying and dropping a JavaException on an unattached thread.

Definition of Done

  • [x] There are no TODOs left in the code
  • [x] Change is covered by automated tests
  • [x] The coding guidelines are followed
  • [x] Public API has documentation
  • [x] User-visible changes are mentioned in the Changelog
  • [ ] The continuous integration build passes

argv-minus-one avatar Sep 26 '23 02:09 argv-minus-one