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: [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]
