diff --git a/src/tui/input.rs b/src/tui/input.rs index 5e01daa..d11caa8 100644 --- a/src/tui/input.rs +++ b/src/tui/input.rs @@ -1,5 +1,6 @@ //! Single-owner terminal input. Create only after capability queries finish; //! drop (and join) before restoring the terminal or handing stdin to a child. +//! The join is bounded: a reader stuck on a hung-up terminal is detached. //! Image/keyboard capability queries may read synchronously during setup, before //! this owner exists. During its lifetime the UI only consumes the bounded queue: //! it never polls the OS input reader or acquires crossterm's input lock. @@ -9,6 +10,7 @@ use std::{ sync::{ Arc, atomic::{AtomicBool, Ordering}, + mpsc::{self as exit, RecvTimeoutError}, }, task::{Context, Poll}, thread::{self, JoinHandle}, @@ -19,10 +21,16 @@ use crossterm::event::{self, Event}; use futures_util::Stream; use tokio::sync::mpsc; +/// A stopped reader returns within one poll interval. On a terminal that hung +/// up, crossterm's poll retries the dead descriptor forever and never returns, +/// so teardown abandons the reader after this long instead of joining it. +const JOIN_TIMEOUT: Duration = Duration::from_secs(2); + pub(super) struct Events { receiver: mpsc::Receiver>, stopped: Arc, reader: Option>, + exited: exit::Receiver<()>, } impl Events { @@ -30,9 +38,12 @@ impl Events { let (sender, receiver) = mpsc::channel(256); let stopped = Arc::new(AtomicBool::new(false)); let reader_stopped = Arc::clone(&stopped); + let (exiting, exited) = exit::channel(); let reader = thread::Builder::new() .name("kit-terminal-input".into()) .spawn(move || { + // Dropped when this thread returns or unwinds; never sent on. + let _exiting = exiting; // No other EventStream or synchronous reader may coexist with this owner. // poll and read always execute on this same OS thread. while !reader_stopped.load(Ordering::Acquire) { @@ -59,6 +70,7 @@ impl Events { receiver, stopped, reader: Some(reader), + exited, }) } @@ -83,6 +95,13 @@ impl Drop for Events { self.receiver.close(); self.stopped.store(true, Ordering::Release); if let Some(reader) = self.reader.take() { + // A reader that outlives the timeout is stuck inside crossterm, + // in practice on a dead terminal with no input left to steal. + // Waiting would keep the process alive and spinning after its + // terminal closed. Detach it so the process can exit. + if self.exited.recv_timeout(JOIN_TIMEOUT) == Err(RecvTimeoutError::Timeout) { + return; + } // A worker panic is already reported by the panic hook and closes // the stream. Never turn cleanup (possibly unwinding) into a panic. let _ = reader.join(); diff --git a/src/tui/input_tests.rs b/src/tui/input_tests.rs index ea98b17..b550e40 100644 --- a/src/tui/input_tests.rs +++ b/src/tui/input_tests.rs @@ -108,6 +108,28 @@ fn child_case(case: &str) { } drop(terminal); } + "hangup" => { + // The parent closes the PTY master, as a closed terminal window + // does. Take the hangup through the real stop boundary, then tear + // down: both steps must return although the terminal is gone. + let runtime = tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .unwrap(); + runtime.block_on(async { + let mut stop = crate::tui::Stop::new().unwrap(); + let (mut terminal, _images) = enter().unwrap(); + let events = Events::new().unwrap(); + marker("HANGUP"); + assert!(stop.until(std::future::pending::<()>()).await.is_none()); + drop(events); + // Restoring a dead terminal fails; it must not panic either. + leave(&mut terminal); + // Nothing is left to report to; only the exit status still + // reaches the parent. + std::process::exit(0); + }); + } "cancel" => { // Poll through actual terminal setup and input acquisition, suspend // at an await, then cancel by dropping the owning future. @@ -211,6 +233,7 @@ fn terminal_input_lifecycle() { "unwind", "worker_unwind", "setup_failure", + "hangup", ] { run_pty(case); } @@ -231,6 +254,13 @@ fn run_pty(case: &str) { }, 0 ); + // The child must not inherit the master, or the parent closing it would + // never hang the terminal up. + // SAFETY: master_fd is the open descriptor openpty just returned. + assert_eq!( + unsafe { libc::fcntl(master_fd, libc::F_SETFD, libc::FD_CLOEXEC) }, + 0 + ); let (mut master, slave) = unsafe { (File::from_raw_fd(master_fd), File::from_raw_fd(slave_fd)) }; let mut command = Command::new(std::env::current_exe().unwrap()); @@ -313,6 +343,18 @@ fn run_pty(case: &str) { sent.push(name); } } + if case == "hangup" && output.contains("INPUT_TEST:HANGUP") { + drop(master); + let status = loop { + assert!(Instant::now() < deadline, "{case} never exited: {output:?}"); + if let Some(status) = child.0.try_wait().unwrap() { + break status; + } + std::thread::sleep(Duration::from_millis(5)); + }; + assert!(status.success(), "{case} failed ({status}): {output:?}"); + return; + } if let Some(status) = child.0.try_wait().unwrap() { assert!(status.success(), "{case} failed ({status}): {output:?}"); assert!(output.contains("INPUT_TEST:DONE"), "{case}: {output:?}"); diff --git a/src/tui/mod.rs b/src/tui/mod.rs index 93ef110..07fbde6 100644 --- a/src/tui/mod.rs +++ b/src/tui/mod.rs @@ -3804,7 +3804,7 @@ fn draw_frame( } /// Owns terminal modes across fallible setup, dropped futures, and unwind. -/// Declare input after this guard so its reader joins before mode restoration. +/// Declare input after this guard so its reader stops before mode restoration. struct TerminalSession { terminal: DefaultTerminal, active: bool, @@ -3832,7 +3832,7 @@ impl Drop for TerminalSession { } fn enter() -> std::io::Result<(TerminalSession, image::ImageRuntime)> { - // Unwinding restores through TerminalSession, after Events has joined. + // Unwinding restores through TerminalSession, after Events has dropped. // A process-global unwind hook would restore too early, including for an // unrelated worker panic whose terminal owner remains alive. #[cfg(panic = "abort")] @@ -4036,7 +4036,14 @@ fn leave(terminal: &mut TerminalSession) { terminal.active = false; restore_modes(); let _ = terminal.show_cursor(); - ratatui::restore(); + if let Err(error) = ratatui::try_restore() { + // After a hangup stderr is the same dead terminal: `eprintln!` would + // panic there and skip the session close that follows teardown. + let _ = std::io::Write::write_all( + &mut std::io::stderr(), + format!("Failed to restore terminal: {error}\n").as_bytes(), + ); + } TERMINAL_ACTIVE.store(false, Ordering::Relaxed); }