You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "viirya (via GitHub)" <gi...@apache.org> on 2026/10/15 22:22:33 UTC
[PR] fix: keep each native panic's backtrace with its own thread [datafusion-comet]
viirya opened a new pull request, #7042:
URL: https://github.com/apache/datafusion-comet/pull/7042
## Which issue does this PR close?
Closes #7041.
## Rationale for this change
The panic hook stores the backtrace of every native panic in one process-wide slot, and the JNI boundary takes whatever the slot holds when a panic becomes a `CometNativeException`. When two threads panic at about the same time, one exception can carry the other thread's stack trace and the other none, so the stack trace points at code that did not fail. Panics that Comet catches itself (for example in the variant casts) also write to the slot, and another thread's boundary can take their backtrace.
## What changes are included in this PR?
`PANIC_BACKTRACE` in `native/jni-bridge/src/errors.rs` becomes a thread-local `Cell<Option<String>>` instead of a global `Mutex`. The panic hook writes the current thread's slot and `NativeFailure::from` takes it. Every place that turns a panic into `CometError::Panic` (`try_unwrap_or_throw`, `catch_native`, and the stream drop in `BatchProducer::stop`) does so on the thread that panicked, so the backtrace is always in that thread's slot.
The hook uses `try_with`, because a panic inside a panic hook aborts the process and the thread-local is gone while its thread is being torn down.
## How are these changes tested?
`panic_backtrace_stays_with_its_thread` makes two threads panic inside `catch_native`. Each thread drops a value while it unwinds that waits until both panic hooks have run, so neither takes its backtrace before the other thread's hook has written. The test checks that each failure carries the backtrace of its own panic. It fails on the current code (one thread gets no backtrace or the other thread's) and passes with this change.
The existing JVM-backed tests in `errors.rs`, including the panic-to-exception test, pass unchanged.
This pull request and its description were written by Isaac.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: log native panic backtraces instead of attaching them to the exception [datafusion-comet]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: log native panic backtraces instead of attaching them to the exception [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove merged PR #7042:
URL: https://github.com/apache/datafusion-comet/pull/7042
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: log native panic backtraces instead of attaching them to the exception [datafusion-comet]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #7042:
URL: https://github.com/apache/datafusion-comet/pull/7042#issuecomment-6359513477
@rich7420 @comphead could you take another look when you have a chance? Since your last reviews the PR no longer attaches a backtrace to the exception: the panic hook logs it with the panic message and location on the thread that panicked, so the same-message case has nothing left to attribute. The unused backtrace plumbing is removed and the report is formatted outside the stderr lock (details in the replies above).
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: log native panic backtraces instead of attaching them to the exception [datafusion-comet]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #7042:
URL: https://github.com/apache/datafusion-comet/pull/7042#issuecomment-6284213473
Thanks @sunchao and @rich7420, the same-message case is real, and I don't think the boundary can close it. Once the hook has run, nothing tells us whether that panic was later caught and handled, and a resumed panic skips the hook. Comparing payload addresses doesn't help either: identical string literals are commonly merged, as in the suggested test. So whenever a panic arrives without a hook run of its own, any captured backtrace is a guess.
I changed the approach instead. The panic hook now logs the backtrace to stderr with the panic message and location, on the thread that panicked, and `CometNativeException` carries the panic message only. That removes the shared slot from #7041 and the per-thread capture, so there is nothing left to attribute, and the executor log has the backtrace of every panic, including a resumed one, which the exception never had reliably.
The trade-off is that the stack frames are no longer in the exception message, so they have to be read from the executor log. I noted that in `debugging.md` and updated the description. `panic_exception_carries_only_its_message` covers a panic on the calling thread, a resumed panic, and the same-message case from the suggestion.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: keep each native panic's backtrace with its own thread [datafusion-comet]
Posted by "rich7420 (via GitHub)" <gi...@apache.org>.
rich7420 commented on code in PR #7042:
URL: https://github.com/apache/datafusion-comet/pull/7042#discussion_r4321163879
##########
native/jni-bridge/src/errors.rs:
##########
@@ -1615,6 +1645,127 @@ mod tests {
std::error::Error::source(error).unwrap().to_string()
}
+ /// Waits twice on a barrier when dropped. Dropped while its thread unwinds, it runs after that
+ /// thread's panic hook and before `catch_native` takes the backtrace.
+ struct WaitOnDrop(Arc<std::sync::Barrier>);
+
+ impl Drop for WaitOnDrop {
+ fn drop(&mut self) {
+ // Both panic hooks have run once both threads pass the first wait.
+ self.0.wait();
+ self.0.wait();
+ }
+ }
+
+ #[inline(never)]
+ fn panic_in_first_thread(barrier: Arc<std::sync::Barrier>) -> CometResult<()> {
+ let _wait = WaitOnDrop(barrier);
+ panic!("first thread")
+ }
+
+ #[inline(never)]
+ fn panic_in_second_thread(barrier: Arc<std::sync::Barrier>) -> CometResult<()> {
+ let _wait = WaitOnDrop(barrier);
+ panic!("second thread")
+ }
+
+ /// Two threads panic at once, and each takes its backtrace only after both panic hooks have
+ /// run. Each must still get the backtrace of its own panic.
+ #[test]
+ fn panic_backtrace_stays_with_its_thread() {
+ init();
+ let barrier = Arc::new(std::sync::Barrier::new(2));
+ let first = {
+ let barrier = Arc::clone(&barrier);
+ std::thread::spawn(move || catch_native(move || panic_in_first_thread(barrier)))
+ };
+ let second = {
+ let barrier = Arc::clone(&barrier);
+ std::thread::spawn(move || catch_native(move || panic_in_second_thread(barrier)))
+ };
+
+ for (handle, own, other) in [
+ (first, "panic_in_first_thread", "panic_in_second_thread"),
+ (second, "panic_in_second_thread", "panic_in_first_thread"),
+ ] {
+ let failure = handle.join().unwrap().unwrap_err();
+ let backtrace = failure
+ .backtrace
+ .unwrap_or_else(|| panic!("no backtrace for {own}"));
+ assert!(backtrace.contains(own), "{own} got another backtrace");
+ assert!(!backtrace.contains(other), "{own} got {other}'s backtrace");
+ }
+ }
+
+ #[inline(never)]
+ fn panic_in_spawned_thread() {
+ panic!("spawned thread")
+ }
+
+ /// 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_in_spawned_thread)
+ .join()
+ .expect_err("the spawned thread panics");
+ std::panic::resume_unwind(payload)
+ }
+
+ #[test]
+ fn panic_resumed_from_another_thread_has_no_backtrace() {
+ init();
+ let failure = catch_native(resume_panic_from_another_thread).unwrap_err();
+ assert!(
+ matches!(&failure.error, CometError::Panic { msg } if msg == "spawned thread"),
+ "{:?}",
+ failure.error
+ );
+ assert!(failure.backtrace.is_none(), "{:?}", failure.backtrace);
+ }
+
+ /// A panic caught and handled during the call leaves its backtrace on this thread. A panic
+ /// resumed from another thread later in the same call must not take it.
+ #[test]
+ fn resumed_panic_does_not_take_a_handled_panic_backtrace() {
+ init();
+ let failure = catch_native(|| {
+ let handled = std::panic::catch_unwind(|| panic!("handled during the call"));
Review Comment:
This reproduces stale attribution:
```suggestion
let handled = std::panic::catch_unwind(|| panic!("spawned thread"));
```
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: log native panic backtraces instead of attaching them to the exception [datafusion-comet]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #7042:
URL: https://github.com/apache/datafusion-comet/pull/7042#issuecomment-6367590680
Thanks all for reviewing!
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: keep each native panic's backtrace with its own thread [datafusion-comet]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #7042:
URL: https://github.com/apache/datafusion-comet/pull/7042#issuecomment-6220324915
Thanks @andygrove and @rich7420, you're right that the same-thread assumption doesn't hold. `RecordBatchReceiverStream` and `SpawnedTask::join_unwind` resume a spawned task's panic on the consuming thread with `resume_unwind`, which skips the hook. That covers `CoalescePartitionsExec`, and `SortPreservingMergeExec` and the spill reader through `spawn_buffered`. `RepartitionExec` isn't one of them: its `wait_for_task` joins the input task with `join()` and sends the panic on as a `DataFusionError` ("Join Error"), so it never becomes `CometError::Panic`, before or after this change.
I went with no backtrace rather than a process-wide fallback, since the fallback would bring back #7041. The latest commit:
- clears the captured panic of the current thread at the start of `try_unwrap_or_throw` and `catch_native`, so a backtrace left by a panic handled before the call, on a pooled thread or otherwise, is never used;
- has the hook record the panic message with the backtrace, and only attaches the backtrace when the message matches the `CometError::Panic` being reported. That also covers panics caught during the call by `catch_unwind` outside Comet, where clearing at each catch site isn't possible.
A resumed panic now reaches the JVM with its message and no stack trace. Its location is still in the executor's stderr, where the hook logs every panic on the thread that raised it.
New tests resume a panic from another thread inside `catch_native` with an empty slot, after a panic handled during the call, and after a handled panic with the same message before the call, and check that none of them gets a backtrace. Removing the message check fails the second, and removing the clear fails the third. I also updated the description.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: log native panic backtraces instead of attaching them to the exception [datafusion-comet]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: log native panic backtraces instead of attaching them to the exception [datafusion-comet]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #7042:
URL: https://github.com/apache/datafusion-comet/pull/7042#discussion_r4321798760
##########
native/jni-bridge/src/errors.rs:
##########
@@ -1615,6 +1645,127 @@ mod tests {
std::error::Error::source(error).unwrap().to_string()
}
+ /// Waits twice on a barrier when dropped. Dropped while its thread unwinds, it runs after that
+ /// thread's panic hook and before `catch_native` takes the backtrace.
+ struct WaitOnDrop(Arc<std::sync::Barrier>);
+
+ impl Drop for WaitOnDrop {
+ fn drop(&mut self) {
+ // Both panic hooks have run once both threads pass the first wait.
+ self.0.wait();
+ self.0.wait();
+ }
+ }
+
+ #[inline(never)]
+ fn panic_in_first_thread(barrier: Arc<std::sync::Barrier>) -> CometResult<()> {
+ let _wait = WaitOnDrop(barrier);
+ panic!("first thread")
+ }
+
+ #[inline(never)]
+ fn panic_in_second_thread(barrier: Arc<std::sync::Barrier>) -> CometResult<()> {
+ let _wait = WaitOnDrop(barrier);
+ panic!("second thread")
+ }
+
+ /// Two threads panic at once, and each takes its backtrace only after both panic hooks have
+ /// run. Each must still get the backtrace of its own panic.
+ #[test]
+ fn panic_backtrace_stays_with_its_thread() {
+ init();
+ let barrier = Arc::new(std::sync::Barrier::new(2));
+ let first = {
+ let barrier = Arc::clone(&barrier);
+ std::thread::spawn(move || catch_native(move || panic_in_first_thread(barrier)))
+ };
+ let second = {
+ let barrier = Arc::clone(&barrier);
+ std::thread::spawn(move || catch_native(move || panic_in_second_thread(barrier)))
+ };
+
+ for (handle, own, other) in [
+ (first, "panic_in_first_thread", "panic_in_second_thread"),
+ (second, "panic_in_second_thread", "panic_in_first_thread"),
+ ] {
+ let failure = handle.join().unwrap().unwrap_err();
+ let backtrace = failure
+ .backtrace
+ .unwrap_or_else(|| panic!("no backtrace for {own}"));
+ assert!(backtrace.contains(own), "{own} got another backtrace");
+ assert!(!backtrace.contains(other), "{own} got {other}'s backtrace");
+ }
+ }
+
+ #[inline(never)]
+ fn panic_in_spawned_thread() {
+ panic!("spawned thread")
+ }
+
+ /// 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_in_spawned_thread)
+ .join()
+ .expect_err("the spawned thread panics");
+ std::panic::resume_unwind(payload)
+ }
+
+ #[test]
+ fn panic_resumed_from_another_thread_has_no_backtrace() {
+ init();
+ let failure = catch_native(resume_panic_from_another_thread).unwrap_err();
+ assert!(
+ matches!(&failure.error, CometError::Panic { msg } if msg == "spawned thread"),
+ "{:?}",
+ failure.error
+ );
+ assert!(failure.backtrace.is_none(), "{:?}", failure.backtrace);
+ }
+
+ /// A panic caught and handled during the call leaves its backtrace on this thread. A panic
+ /// resumed from another thread later in the same call must not take it.
+ #[test]
+ fn resumed_panic_does_not_take_a_handled_panic_backtrace() {
+ init();
+ let failure = catch_native(|| {
+ let handled = std::panic::catch_unwind(|| panic!("handled during the call"));
Review Comment:
Thanks, this case is covered now: `panic_exception_carries_only_its_message` handles a `panic!("spawned thread")` during the call before resuming one with the same message. The exception no longer carries a backtrace, see https://github.com/apache/datafusion-comet/pull/7042#issuecomment-6284213473.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org