comphead commented on code in PR #7042:
URL: https://github.com/apache/datafusion-comet/pull/7042#discussion_r4332092160
##########
native/jni-bridge/src/errors.rs:
##########
@@ -633,27 +624,22 @@ impl<'a> NativeError<'a> {
}
}
-/// A failed native call caught at the boundary: the error and, for a panic,
its captured
-/// backtrace.
+/// A failed native call caught at the boundary. A panic's backtrace is logged
by the panic hook
+/// rather than carried here; see [`init`].
#[derive(Debug)]
pub struct NativeFailure {
pub error: CometError,
- pub backtrace: Option<String>,
}
impl From<CometError> for NativeFailure {
fn from(error: CometError) -> Self {
- let backtrace = match error {
- CometError::Panic { .. } => PANIC_BACKTRACE.lock().unwrap().take(),
- _ => None,
- };
- NativeFailure { error, backtrace }
+ NativeFailure { error }
}
}
impl NativeFailure {
pub fn native_error(&self) -> NativeError<'_> {
- NativeError::from_comet_error(&self.error, self.backtrace.clone())
+ NativeError::from_comet_error(&self.error, None)
Review Comment:
After this change `NativeFailure::native_error` is the only production
caller of `from_comet_error`, and it always passes `None`. So the `backtrace`
parameter of `from_comet_error`, `jvm_exception`, `generic_exception` and
`natural_exception`, the `NativeError::backtrace` field and the `"backtrace"`
key in `to_payload` (always `null` now), `to_stacktrace_string` with its
testdata, and the `Some(backtrace_string)` branch look reachable only from
tests. The comments at `native/core/src/lib.rs` line 136 ("capture panic
backtraces") and in `errors.rs` are now stale too. Could this PR remove them,
or are they kept on purpose for another caller of the versioned payload? If
they stay, the field doc should say so. If they go, dropping the payload key
probably needs a decision about `NATIVE_ERROR_PAYLOAD_VERSION`. #7098 and #7104
also edit `natural_exception`, so ordering with them matters. I found no other
caller with a grep over `native` and did not run anything.
##########
native/jni-bridge/src/errors.rs:
##########
@@ -184,12 +172,15 @@ pub enum CometError {
pub fn init() {
std::panic::set_hook(Box::new(|panic_info| {
- // Log the panic message and location to stderr so it is visible in CI
logs
- // even if the exception message is lost crossing the FFI boundary
- eprintln!("Comet native panic: {panic_info}");
- // Capture the backtrace for a panic
- *PANIC_BACKTRACE.lock().unwrap() =
- Some(std::backtrace::Backtrace::force_capture().to_string());
+ // Log the panic message, location and backtrace to stderr, on the
thread that panicked,
+ // so they are in the executor log even if the exception message is
lost crossing the
+ // FFI boundary. The backtrace is not attached to the JVM exception,
because a panic can
+ // reach the JNI boundary on another thread: DataFusion resumes the
panics of its spawned
+ // tasks on the consuming thread with `resume_unwind`, which does not
run this hook. The
+ // boundary therefore cannot tell which captured backtrace belongs to
the panic it
+ // reports. One `eprintln!` keeps a panic's lines together when
threads panic at once.
+ let backtrace = std::backtrace::Backtrace::force_capture();
+ eprintln!("Comet native panic: {panic_info}\n{backtrace}");
Review Comment:
`Backtrace::force_capture` only captures raw frames. Symbol resolution is
lazy and happens in `Display`, which here runs inside `eprintln!` while
`Stderr::write_fmt` holds the stderr lock. The previous hook called
`.to_string()` before storing the trace, so resolution happened outside any
lock. Other threads that write to stderr, including the default log4rs console
appender, would now wait behind symbolication, which can be slow on the first
call for a large `libcomet`. Would formatting the report into a `String` first
and then calling `eprintln!("{report}")` keep the single write and the
non-interleaving without holding the lock while resolving? This is reasoned
from the std and log4rs sources and not measured.
##########
native/jni-bridge/src/errors.rs:
##########
@@ -1637,6 +1622,40 @@ mod tests {
assert!(native_error.message().starts_with("boom"));
}
+ /// Resumes on this thread a panic raised on another one, as DataFusion
does with the panics
+ /// of its spawned tasks. `resume_unwind` does not run the panic hook.
+ fn resume_panic_from_another_thread() -> CometResult<()> {
+ let payload = std::thread::spawn(|| panic!("spawned thread"))
+ .join()
+ .expect_err("the spawned thread panics");
+ std::panic::resume_unwind(payload)
+ }
+
+ /// A panic's exception carries its own message and no backtrace,
whichever thread raised it
+ /// and whatever other panic this thread handled before. The hook logs the
backtrace instead.
+ #[test]
+ fn panic_exception_carries_only_its_message() {
Review Comment:
This test would pass with any hook, since `native_error()` always passes
`None` and the message is just the panic message, and the `formatted message 7`
case overlaps `catch_native_reports_errors_and_panics`. The new behavior, that
the hook logs the message, location and backtrace, is not asserted.
`PanicHookInfo` cannot be built by hand, but moving the formatting into a small
function that takes the rendered pieces would let a test check the `Comet
native panic:` prefix. If that is not worth it, a note that the log side is
untested would help.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
