viirya commented on code in PR #7042:
URL: https://github.com/apache/datafusion-comet/pull/7042#discussion_r4339744186
##########
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:
They were not kept for another caller, so they are removed now: the
`backtrace` parameter of `from_comet_error`, `jvm_exception`,
`generic_exception` and `natural_exception`, the `NativeError::backtrace`
field, `to_stacktrace_string` with its test data (and their RAT exclusions),
and the stale comments, including the one in `native/core/src/lib.rs`. Dropping
the `"backtrace"` key changes the payload document, so
`NATIVE_ERROR_PAYLOAD_VERSION` is now 2. Nothing reads the payload yet. On
ordering with #7098 and #7104: this only drops the `backtrace` parameter and
the `Some(..)` branch of `natural_exception`, so whichever lands second has a
small conflict there.
##########
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:
Right, `Backtrace` resolves lazily in `Display`. The hook now formats the
report into a `String` with `panic_report` and then writes it with one
`eprintln!("{report}")`, so resolution happens before the lock is taken and the
report is still a single write.
##########
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:
Agreed. The formatting moved into `panic_report`, and
`panic_report_has_the_panic_and_its_backtrace` checks the report, with a note
that the hook itself isn't run because `PanicHookInfo` can't be built outside a
panic. I dropped the case that overlapped
`catch_native_reports_errors_and_panics` and kept the resumed and same-message
cases as a guard against attaching a backtrace to the exception again.
--
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]
