From 6be953459c497761b41516c8ecd930263c6b5b94 Mon Sep 17 00:00:00 2001 From: mtlcl410 <2368091+mtlcl410@users.noreply.github.com> Date: Sat, 17 Oct 2026 13:25:53 +0800 Subject: [PATCH] fix(print): the options read ends at its deadline and a dead read wakes the dialog (#1818) Review follow-up on #2108: - output_within waited on the reader thread with join, so a process the command started could hold the output pipe, and the wait, long past the limit after the command exited or was killed. The output now comes back over a channel with recv_timeout; such a reader finishes on its own. - the printer-list and options workers ask for a frame from a guard dropped on exit, also when the source panics, so an idle dialog sees the closed channel without waiting for another event. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01RUNQSPVXx3dDBabRZHAnjz --- crates/print/src/spool.rs | 33 +++++++++++++++++++----------- crates/print/src/tests.rs | 25 +++++++++++++++++++++++ crates/ui-egui/src/print_ui.rs | 30 +++++++++++++++++++-------- crates/ui-egui/tests/print.rs | 37 ++++++++++++++++++++++++++++++++++ 4 files changed, 105 insertions(+), 20 deletions(-) diff --git a/crates/print/src/spool.rs b/crates/print/src/spool.rs index f6655174e..3d6576861 100644 --- a/crates/print/src/spool.rs +++ b/crates/print/src/spool.rs @@ -48,32 +48,41 @@ pub(crate) fn output_within(command: &mut std::process::Command, limit: std::tim }; // Read on a thread of its own, so a full pipe never holds the command while the deadline is // kept here. Past the cap the rest is drained and dropped, so the command still finishes. + // The output comes back over a channel rather than through `join`: a process the command + // started can keep the pipe open after the command itself has exited or been killed, and + // the wait for the output must end at the deadline too. Such a reader is left to finish on + // its own when that process closes the pipe. + let (tx, rx) = std::sync::mpsc::channel(); let reader = std::thread::Builder::new().name("command output".into()).spawn(move || { let mut out = Vec::new(); let read = (&mut stdout).take(OUTPUT_CAP).read_to_end(&mut out); let _ = std::io::copy(&mut stdout, &mut std::io::sink()); - read.map(|_| out) + let _ = tx.send(read.map(|_| out)); }); - let reader = match reader { - Ok(reader) => reader, - Err(e) => { - stop(&mut child); - return Err(e); - } - }; + if let Err(e) = reader { + stop(&mut child); + return Err(e); + } + let timed_out = || std::io::Error::new(std::io::ErrorKind::TimedOut, format!("the command did not answer within {limit:?}")); let deadline = std::time::Instant::now() + limit; loop { match child.try_wait() { - Ok(Some(_)) => return reader.join().map_err(|_| std::io::Error::other("the command's output was lost"))?, + Ok(Some(_)) => { + // A moment's grace, so output written just before an exit at the deadline counts. + let left = deadline.saturating_duration_since(std::time::Instant::now()).max(std::time::Duration::from_millis(100)); + return match rx.recv_timeout(left) { + Ok(read) => read, + Err(std::sync::mpsc::RecvTimeoutError::Timeout) => Err(timed_out()), + Err(std::sync::mpsc::RecvTimeoutError::Disconnected) => Err(std::io::Error::other("the command's output was lost")), + }; + } Ok(None) if std::time::Instant::now() >= deadline => { stop(&mut child); - let _ = reader.join(); - return Err(std::io::Error::new(std::io::ErrorKind::TimedOut, format!("the command did not answer within {limit:?}"))); + return Err(timed_out()); } Ok(None) => std::thread::sleep(std::time::Duration::from_millis(10)), Err(e) => { stop(&mut child); - let _ = reader.join(); return Err(e); } } diff --git a/crates/print/src/tests.rs b/crates/print/src/tests.rs index bb55ac397..0822f99da 100644 --- a/crates/print/src/tests.rs +++ b/crates/print/src/tests.rs @@ -781,10 +781,22 @@ mod bounded_commands { match std::env::var(MODE).as_deref() { Ok("answer") => println!("hello from the command"), Ok("hang") => std::thread::sleep(std::time::Duration::from_secs(60)), + // Holds the output pipe it inherited for a while, as a daemon a command starts can. + Ok("linger") => std::thread::sleep(LINGER), + // Starts a lingering process on its own output, then exits or hangs itself. + Ok(mode @ ("orphan-exit" | "orphan-hang")) => { + let _ = command("linger").spawn(); + println!("started"); + if mode == "orphan-hang" { + std::thread::sleep(std::time::Duration::from_secs(60)); + } + } _ => {} } } + const LINGER: std::time::Duration = std::time::Duration::from_secs(8); + fn command(mode: &str) -> std::process::Command { let mut c = std::process::Command::new(std::env::current_exe().unwrap()); c.args(["--exact", "tests::bounded_commands::stand_in_for_a_command", "--nocapture", "--"]).env(MODE, mode); @@ -804,4 +816,17 @@ mod bounded_commands { assert_eq!(err.kind(), std::io::ErrorKind::TimedOut); assert!(started.elapsed() < std::time::Duration::from_secs(20), "stopped soon after the deadline, not when the command gives up"); } + + /// A process the command started keeps the output pipe open after the command has exited, + /// or after it was killed at the deadline: the wait still ends at the deadline (#2108 + /// review), not when that process lets go of the pipe. + #[test] + fn a_process_left_holding_the_output_does_not_extend_the_wait() { + for mode in ["orphan-exit", "orphan-hang"] { + let started = std::time::Instant::now(); + let err = crate::spool::output_within(&mut command(mode), std::time::Duration::from_millis(1500)).unwrap_err(); + assert_eq!(err.kind(), std::io::ErrorKind::TimedOut, "{mode}"); + assert!(started.elapsed() < LINGER / 2, "{mode}: returned after {:?}, when the lingering process let go", started.elapsed()); + } + } } diff --git a/crates/ui-egui/src/print_ui.rs b/crates/ui-egui/src/print_ui.rs index 1aa5137a9..b53faac46 100644 --- a/crates/ui-egui/src/print_ui.rs +++ b/crates/ui-egui/src/print_ui.rs @@ -303,6 +303,22 @@ fn default_printer(printers: &[spool::Printer]) -> Option { printers.iter().find(|p| p.default).or(printers.first()).map(|p| p.name.clone()) } +/// Asks for a frame when a worker thread ends, also when it panics: [`PdfCraftApp::poll_print`] +/// then sees the answer, or the disconnected channel of a worker that died, without waiting +/// for some other event to draw (#2108 review). Declared before the worker's sender, so the +/// sender is dropped first and the frame finds the channel closed. +#[cfg(not(target_arch = "wasm32"))] +struct WakeOnExit(Option); + +#[cfg(not(target_arch = "wasm32"))] +impl Drop for WakeOnExit { + fn drop(&mut self) { + if let Some(ctx) = &self.0 { + ctx.request_repaint(); + } + } +} + impl PdfCraftApp { pub fn open_print(&mut self) { let Some((i, _)) = self.active_ids() else { return }; @@ -394,13 +410,12 @@ impl PdfCraftApp { return; } let (tx, rx) = std::sync::mpsc::channel(); - let ctx = self.ctx.clone(); + let wake = WakeOnExit(self.ctx.clone()); std::thread::spawn(move || { + let _wake = wake; + let tx = tx; // The receiver may be gone (the app quit): nothing to report to then. let _ = tx.send(source()); - if let Some(ctx) = ctx { - ctx.request_repaint(); - } }); self.print_tasks.printers = Some(rx); } @@ -414,14 +429,13 @@ impl PdfCraftApp { } let source: OptionsSource = self.options_source.clone().unwrap_or_else(|| std::sync::Arc::new(|p: &str| spool::printer_options(p))); let (tx, rx) = std::sync::mpsc::channel(); - let ctx = self.ctx.clone(); + let wake = WakeOnExit(self.ctx.clone()); let name = printer.clone(); let spawned = std::thread::Builder::new().name("printer options".into()).spawn(move || { + let _wake = wake; + let tx = tx; // The receiver may be gone (the app quit): nothing to report to then. let _ = tx.send(source(&name)); - if let Some(ctx) = ctx { - ctx.request_repaint(); - } }); match spawned { Ok(_) => self.print_tasks.options.push((printer, rx)), diff --git a/crates/ui-egui/tests/print.rs b/crates/ui-egui/tests/print.rs index ee56bcb1e..31e5e0299 100644 --- a/crates/ui-egui/tests/print.rs +++ b/crates/ui-egui/tests/print.rs @@ -486,3 +486,40 @@ fn printer_options_are_read_off_the_gui_thread_and_a_dead_read_offers_nothing() assert_eq!(h.state().print_draft.driver_options, Some(("Dead".into(), Vec::new())), "a dead read offers nothing, and the panel says so"); h.get_by_label("This printer offers no settings of its own."); } + +/// A read of the printer's options that dies still asks for a frame, so an idle dialog stops +/// saying it is reading without waiting for the mouse to move (#2108 review). +#[cfg(not(target_arch = "wasm32"))] +#[test] +fn a_dead_options_read_wakes_an_idle_dialog() { + use std::sync::atomic::{AtomicBool, Ordering}; + use std::sync::{Arc, Mutex}; + let mut h = harness(); + let gate = Arc::new(Mutex::new(())); + let hold = gate.lock().unwrap(); + let wait = gate.clone(); + let dying = Arc::new(AtomicBool::new(false)); + let flag = dying.clone(); + h.state_mut().options_source = Some(Arc::new(move |_: &str| { + let _open = wait.lock(); + flag.store(true, Ordering::SeqCst); + panic!("the read died"); + })); + assert!(h.state_mut().execute("print.dialog")); + h.run_steps(2); + let d = &mut h.state_mut().print_draft; + d.driver_options = None; + d.driver_options_loading = Some("Office".into()); + d.read_options_for = Some("Office".into()); + d.show_driver_options = true; + h.run(); + assert!(!h.ctx.has_requested_repaint(), "the dialog is idle while the read waits"); + drop(hold); + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(10); + while !(dying.load(Ordering::SeqCst) && h.ctx.has_requested_repaint()) && std::time::Instant::now() < deadline { + std::thread::sleep(std::time::Duration::from_millis(5)); + } + assert!(h.ctx.has_requested_repaint(), "the dead read asked for a frame"); + h.run(); + assert_eq!(h.state().print_draft.driver_options, Some(("Office".into(), Vec::new()))); +}