From 95b467a76f71514e938545b7c793eeb6663fa5ed Mon Sep 17 00:00:00 2001 From: Franz Dietrich Date: Sun, 20 Sep 2026 13:53:15 +0200 Subject: [PATCH] Remove Process-Wide Panic Hook in Headless SVG Export Greptile flagged that [`run_headless_svg_export`](file:///home/dietrich/Projekte/Source/turtlers/turtle-lib/src/export.rs#L80) was setting and taking the global, process-wide panic hook without thread coordination. In concurrent environments (such as multi-threaded test runners or multi-threaded host applications), this caused race conditions that could clobber custom hooks, restore stale hooks, or capture another thread's panic hook. [export.rs](file:///home/dietrich/Projekte/Source/turtlers/turtle-lib/src/export.rs) 1. **Removed Global Panic Hook Manipulation**: - Removed `PanicHookFn` and `PanicHookGuard`. - Eliminated calls to `std::panic::take_hook()` and `std::panic::set_hook()`. - Removed `AtomicBool` and `Arc` overhead. 2. **Inspect Panic Payload Directly in [`run_headless_svg_export`](file:///home/dietrich/Projekte/Source/turtlers/turtle-lib/src/export.rs#L80)**: - Evaluates user command closures inside `std::panic::catch_unwind`. - Downcasts caught panic payload to `&str` or `String` and checks for Macroquad assertion signatures (`THREAD_ID.is_some()` or `macroquad`). - Packages diagnostic guidance directly into `ExportError::Execution(...)` without emitting unsolicited messages to `eprintln!`. 3. **Banner in CLI Entrypoint [`handle_svg_export`](file:///home/dietrich/Projekte/Source/turtlers/turtle-lib/src/export.rs#L137)**: - When executed via `--export-svg`, if `run_headless_svg_export` fails due to Macroquad window/GUI functions, formats and displays the friendly "Headless SVG Export Note" banner to `eprintln!`. 4. **Unit Tests**: - Added `test_headless_svg_export_success`: checks complete headless SVG export output. - Added `test_headless_svg_export_macroquad_panic`: verifies proper detection and error message for simulated Macroquad panics. - Added `test_headless_svg_export_custom_panic`: verifies that non-Macroquad panics retain their original error message. --- - **Default tests**: ```bash cargo test --package turtle-lib ``` Result: 26 unit tests passed, 34 doc-tests passed (0 failures). - **SVG feature tests**: ```bash cargo test --package turtle-lib --features svg ``` Result: 32 unit tests passed, 34 doc-tests passed (0 failures). - **Clippy**: ```bash cargo clippy --package turtle-lib --features svg -- -Wclippy::pedantic \ -Aclippy::cast_precision_loss -Aclippy::cast_sign_loss -Aclippy::cast_possible_truncation ``` Result: 0 warnings, clean. - **SVG export example**: ```bash cargo run --package turtle-lib --example test_svg_export --features svg -- --export-svg /tmp/test_output.svg ``` Result: Successfully generated valid SVG file `/tmp/test_output.svg`. --- turtle-lib/src/export.rs | 136 +++++++++++++++++++++------------------ 1 file changed, 74 insertions(+), 62 deletions(-) diff --git a/turtle-lib/src/export.rs b/turtle-lib/src/export.rs index 68f6e19..decde53 100644 --- a/turtle-lib/src/export.rs +++ b/turtle-lib/src/export.rs @@ -72,23 +72,6 @@ pub fn parse_svg_export_arg() -> Option { parse_svg_export_from_args(&mut args).ok().flatten() } -#[cfg(feature = "svg")] -type PanicHookFn = Box) + Sync + Send + 'static>; - -#[cfg(feature = "svg")] -struct PanicHookGuard { - prev_hook: Option>, -} - -#[cfg(feature = "svg")] -impl Drop for PanicHookGuard { - fn drop(&mut self) { - if let Some(prev) = self.prev_hook.take() { - std::panic::set_hook(Box::new(move |info| prev(info))); - } - } -} - /// Headless SVG export that executes drawing commands and writes an SVG file /// without opening a graphics window and without calling `std::process::exit`. /// @@ -102,55 +85,13 @@ where #[cfg(feature = "svg")] { use std::panic::AssertUnwindSafe; - use std::sync::atomic::{AtomicBool, Ordering}; - use std::sync::Arc; let mut turtle = crate::create_turtle_plan(); - let is_mq_panic = Arc::new(AtomicBool::new(false)); - let is_mq_clone = Arc::clone(&is_mq_panic); - - let prev_hook = Arc::new(std::panic::take_hook()); - let prev_hook_for_closure = Arc::clone(&prev_hook); - - let guard = PanicHookGuard { - prev_hook: Some(prev_hook), - }; - - std::panic::set_hook(Box::new(move |info| { - prev_hook_for_closure(info); - - let msg = if let Some(s) = info.payload().downcast_ref::<&str>() { - *s - } else if let Some(s) = info.payload().downcast_ref::() { - s.as_str() - } else { - "" - }; - - let loc_file = info.location().map_or("", std::panic::Location::file); - let is_context_panic = msg.contains("THREAD_ID.is_some()") - || (loc_file.contains("macroquad") && msg.contains("assertion failed")); - - if is_context_panic { - is_mq_clone.store(true, Ordering::SeqCst); - eprintln!("\n================================================================================"); - eprintln!("Headless SVG Export Note:"); - eprintln!("A Macroquad window/rendering function (e.g. `screen_width()`, `screen_height()`,"); - eprintln!("or input check) was called while running in headless export mode."); - eprintln!("Headless export does not initialize a graphics window. To resolve this:"); - eprintln!(" - Use relative turtle commands or fixed coordinates instead of window queries, or"); - eprintln!(" - Run the program in windowed GUI mode without the `--export-svg` flag."); - eprintln!("================================================================================\n"); - } - })); - let result = std::panic::catch_unwind(AssertUnwindSafe(|| { build_commands(&mut turtle); })); - drop(guard); - match result { Ok(()) => { let mut app = crate::TurtleApp::new(); @@ -159,12 +100,21 @@ where app.export_drawing(filename, crate::export::DrawingFormat::Svg) } Err(payload) => { - let err_msg = if is_mq_panic.load(Ordering::SeqCst) { - "Drawing function called Macroquad window/GUI functions (e.g. `screen_width()`, `screen_height()`) which are unavailable in headless SVG export mode.".to_string() - } else if let Some(s) = payload.downcast_ref::<&str>() { + let raw_msg = if let Some(s) = payload.downcast_ref::<&str>() { (*s).to_string() } else if let Some(s) = payload.downcast_ref::() { s.clone() + } else { + String::new() + }; + + let is_context_panic = raw_msg.contains("THREAD_ID.is_some()") + || raw_msg.contains("macroquad"); + + let err_msg = if is_context_panic { + "Drawing function called Macroquad window/GUI functions (e.g. `screen_width()`, `screen_height()`) which are unavailable in headless SVG export mode.".to_string() + } else if !raw_msg.is_empty() { + raw_msg } else { "Drawing function panicked during execution".to_string() }; @@ -217,6 +167,18 @@ where std::process::exit(0); } Err(e) => { + if let ExportError::Execution(ref msg) = e { + if msg.contains("unavailable in headless SVG export mode") { + eprintln!("\n================================================================================"); + eprintln!("Headless SVG Export Note:"); + eprintln!("A Macroquad window/rendering function (e.g. `screen_width()`, `screen_height()`,"); + eprintln!("or input check) was called while running in headless export mode."); + eprintln!("Headless export does not initialize a graphics window. To resolve this:"); + eprintln!(" - Use relative turtle commands or fixed coordinates instead of window queries, or"); + eprintln!(" - Run the program in windowed GUI mode without the `--export-svg` flag."); + eprintln!("================================================================================\n"); + } + } eprintln!("Error: {e}"); std::process::exit(1); } @@ -303,4 +265,54 @@ mod tests { "SVG export feature is not enabled. Please rebuild with --features svg" ); } + + #[cfg(feature = "svg")] + #[test] + fn test_headless_svg_export_success() { + use crate::Movement; + + let temp_dir = std::env::temp_dir(); + let path = temp_dir.join(format!("turtle_test_export_{}.svg", std::process::id())); + let path_str = path.to_str().expect("valid utf-8 path"); + + let res = run_headless_svg_export(|turtle| { + turtle.forward(10.0); + }, path_str); + + assert!(res.is_ok()); + assert!(path.exists()); + let content = std::fs::read_to_string(&path).expect("read temp svg"); + assert!(content.contains(" { + assert!(msg.contains("unavailable in headless SVG export mode")); + } + other => panic!("expected ExportError::Execution, got {other:?}"), + } + } + + #[cfg(feature = "svg")] + #[test] + fn test_headless_svg_export_custom_panic() { + let res = run_headless_svg_export(|_turtle| { + panic!("custom user panic occurred"); + }, "unused.svg"); + + match res { + Err(ExportError::Execution(msg)) => { + assert_eq!(msg, "custom user panic occurred"); + } + other => panic!("expected ExportError::Execution, got {other:?}"), + } + } }