From 2e5ec06b4e78481126b7af5d7956bd7d5c32f50b Mon Sep 17 00:00:00 2001 From: ddidderr Date: Thu, 15 Oct 2026 18:42:36 +0200 Subject: [PATCH] fix(renderer): prevent display teardown starvation during output floods Closing a busy split or confirming a tab close could freeze GTK while unrealizing the GLArea. Both reported GDB traces show the main thread waiting for the CPU frame-state mutex while the renderer keeps rebuilding dense text. GTK unrealizes the display before stopping the workers, so continuous output can keep the competing renderer active indefinitely. Use parking_lot fair mutexes for CPU frame state and GPU draw state. A waiting lifecycle or draw callback receives ownership instead of losing it to the next frame update. Keep the existing lock order and resource cleanup; no output, scrollback, or frame-rate limits are introduced. The fair-lock handoff adds scheduling overhead when the locks are contended. Correct the threading documentation: the CPU state is also accessed by apprt callbacks, and the Rust-only mutex is therefore contended. Test plan: - Add a mock GPU regression continuously rebuilding 200x150 dense cells while another thread unrealizes and realizes the display. It failed its three-second deadline with the old locks and passes with fair locks. - All 235 renderer/core tests pass, including Mesa EGL rendering tests. - Clippy for renderer/core/GTK, all targets, passes with -D warnings. - Nightly formatting check and release binary build pass. - GTK/Wayland replay with font size 9 and 1 GiB scrollback runs base64 floods in two splits, confirms split and tab closes, then successfully executes and renders a command in the surviving terminal. Docs: docs/rewrite/ARCHITECTURE.md --- Cargo.lock | 1 + crates/ghostty-renderer/Cargo.toml | 1 + crates/ghostty-renderer/src/generic.rs | 80 ++++++++++++----------- crates/ghostty-renderer/tests/mock_api.rs | 74 ++++++++++++++++++++- docs/rewrite/ARCHITECTURE.md | 8 +++ 5 files changed, 124 insertions(+), 40 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 7d2c9ec..f2f0213 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1673,6 +1673,7 @@ dependencies = [ "objc2-metal", "objc2-quartz-core", "onig", + "parking_lot", "png", "thiserror 2.0.21", ] diff --git a/crates/ghostty-renderer/Cargo.toml b/crates/ghostty-renderer/Cargo.toml index 44cce2c..b22d203 100644 --- a/crates/ghostty-renderer/Cargo.toml +++ b/crates/ghostty-renderer/Cargo.toml @@ -23,6 +23,7 @@ khronos-egl = { version = "6.0.0", features = ["dynamic"] } log = "0.4.34" naga = { version = "30.0.1", features = ["glsl-in", "glsl-out", "msl-out", "wgsl-in"] } onig = "6.5.3" +parking_lot = "0.12.5" png = "0.18.1" thiserror = "2.0.21" diff --git a/crates/ghostty-renderer/src/generic.rs b/crates/ghostty-renderer/src/generic.rs index 97c410e..5ce7ff1 100644 --- a/crates/ghostty-renderer/src/generic.rs +++ b/crates/ghostty-renderer/src/generic.rs @@ -24,14 +24,19 @@ //! The renderer state is split to make that safe without `unsafe`: //! //! * `draw` ([`DrawState`], Zig's `draw_mutex`): everything `draw_frame` -//! reads. Held by the renderer thread only for the short "publish" part of -//! `update_frame` (`rebuild_cells`, uniforms) and by `set_*`/`change_config`. -//! * `thread` ([`ThreadState`]): data only the renderer thread touches -//! (`RenderState`, font shaper + cache, overlay, search highlights). It is a -//! `Mutex` only so that `Renderer` is `Sync`; it is uncontended. +//! reads. Held by the renderer thread while rebuilding cells/publishing +//! uniforms in `update_frame`, and by `set_*`/`change_config`. +//! * `thread` ([`ThreadState`]): render snapshots, font shaper + cache, overlay +//! and search highlights. Display teardown and dirty marking also access +//! this state from the apprt thread. //! -//! Lock order is always: renderer mutex (`SharedState`) is *not* held while -//! taking `draw`; `thread` before `draw` before the font grid lock. +//! Both locks are fair: a worker continuously rebuilding dense frames must +//! hand ownership to a waiting apprt callback instead of immediately taking +//! the lock again. In particular, GLArea unrealize runs before worker shutdown +//! when closing tabs/splits and must make progress while output is still live. +//! +//! Lock order is `thread`, terminal (`SharedState`), `draw`, then the font +//! grid lock. Terminal snapshotting releases `SharedState` before shaping. //! //! # GL context requirements //! @@ -88,6 +93,7 @@ use ghostty_terminal::{ terminal::ScrollViewport, }; use ghostty_vt::{color::Rgb, modes::Mode, sgr::Underline}; +use parking_lot::FairMutex; use crate::{ backend::Health, @@ -684,7 +690,7 @@ struct DrawState { shaders: Option, } -/// State only touched by the renderer thread. +/// CPU frame state shared with apprt display lifecycle and dirty marking. struct ThreadState { /// The thread's copy of the derived config (kept in sync with /// `DrawState::config`). @@ -727,8 +733,8 @@ struct ThreadState { /// The generic renderer. See the module docs for the threading model. pub struct Renderer { - draw: Mutex>, - thread: Mutex, + draw: FairMutex>, + thread: FairMutex, /// Health of the most recently completed frame. health: Arc, @@ -1448,8 +1454,8 @@ impl Renderer { Ok(Renderer { display_link: Mutex::new(display_link), - draw: Mutex::new(draw), - thread: Mutex::new(thread), + draw: FairMutex::new(draw), + thread: FairMutex::new(thread), health: Arc::new(AtomicU8::new(Health::Healthy as u8)), has_animations, host: opts.host, @@ -1464,7 +1470,7 @@ impl Renderer { pub fn display_realized_with(&self, realize: impl FnOnce(&mut G)) -> Result<(), Error> { // Lock the draw mutex so that we can // safely reinitialize our GPU resources. - let mut d = lock(&self.draw); + let mut d = self.draw.lock(); // If our API has to do things on realize, let it. realize(&mut d.api); @@ -1495,8 +1501,8 @@ impl Renderer { // Lock the draw mutex so that we can // safely deinitialize our GPU resources. (The thread state is locked // first, per the lock order.) - let mut t = lock(&self.thread); - let mut d = lock(&self.draw); + let mut t = self.thread.lock(); + let mut d = self.draw.lock(); // We deinit our swap chain and shaders. // @@ -1517,7 +1523,7 @@ impl Renderer { /// Mark the full screen as dirty so that we redraw everything. pub fn mark_dirty(&self) { - lock(&self.thread).terminal_state.dirty = Dirty::Full; + self.thread.lock().terminal_state.dirty = Dirty::Full; } /// True if our renderer has animations so that a higher frequency @@ -1539,17 +1545,17 @@ impl Renderer { /// The font grid currently used for rendering (for apprts that need /// to read the primary face, e.g. macOS quicklook). pub fn font_grid(&self) -> Arc { - Arc::clone(&lock(&self.draw).font_grid) + Arc::clone(&self.draw.lock().font_grid) } /// Called by the renderer thread when it begins (`threadEnter`). pub fn thread_enter(&self) { - lock(&self.draw).api.thread_enter(); + self.draw.lock().api.thread_enter(); } /// Called by the renderer thread when it exits (`threadExit`). pub fn thread_exit(&self) { - lock(&self.draw).api.thread_exit(); + self.draw.lock().api.thread_exit(); } /// Called by the renderer thread when it starts the main loop @@ -1559,7 +1565,7 @@ impl Renderer { // If our API has to do things on loop enter, let it. (Metal draws // synchronously from the layer's display callback.) let weak = Arc::downgrade(self); - lock(&self.draw).api.loop_enter(Arc::new(move || { + self.draw.lock().api.loop_enter(Arc::new(move || { if let Some(renderer) = weak.upgrade() && let Err(err) = renderer.draw_frame(true) { @@ -1586,7 +1592,7 @@ impl Renderer { /// (`loopExit`). pub fn loop_exit(&self) { // If our API has to do things on loop exit, let it. - lock(&self.draw).api.loop_exit(); + self.draw.lock().api.loop_exit(); // Stop our display link. If this fails its okay it just means // that we either never started it or the view its attached to @@ -1620,14 +1626,14 @@ impl Renderer { /// The current size of everything as the renderer knows it. pub fn size(&self) -> Size { - lock(&self.draw).size + self.draw.lock().size } /// Callback when the focus changes for the terminal this is rendering. /// /// Must be called on the render thread. pub fn set_focus(&self, focus: bool) { - let mut d = lock(&self.draw); + let mut d = self.draw.lock(); debug_assert_ne!(d.focused, focus); d.focused = focus; @@ -1650,7 +1656,7 @@ impl Renderer { // If we're not visible, then we want to stop the display link // because it is a waste of resources and we can move to pure // change-driven updates. - let focused = lock(&self.draw).focused; + let focused = self.draw.lock().focused; if let Some(link) = lock(&self.display_link).as_ref() { let _ = if visible && focused { link.start() @@ -1664,8 +1670,8 @@ impl Renderer { /// /// Must be called on the render thread. pub fn set_font_grid(&self, grid: Arc) { - let mut t = lock(&self.thread); - let mut d = lock(&self.draw); + let mut t = self.thread.lock(); + let mut d = self.draw.lock(); // Get our metrics from the grid. This doesn't require a lock because // the metrics are never recalculated. @@ -1702,8 +1708,8 @@ impl Renderer { /// Update the configuration. #[allow(clippy::float_cmp)] pub fn change_config(&self, config: DerivedConfig) -> Result<(), Error> { - let mut t = lock(&self.thread); - let mut d = lock(&self.draw); + let mut t = self.thread.lock(); + let mut d = self.draw.lock(); // We always redo the font shaper in case font features changed. We // could check to see if there was an actual config change but this is @@ -1773,7 +1779,7 @@ impl Renderer { /// Resize the screen. pub fn set_screen_size(&self, size: Size) { - let mut d = lock(&self.draw); + let mut d = self.draw.lock(); // We only actually need the padding from this, // everything else is derived elsewhere. @@ -1786,14 +1792,14 @@ impl Renderer { /// Set the search highlights (from the renderer thread mailbox). pub fn set_search_viewport_matches(&self, matches: Vec) { - let mut t = lock(&self.thread); + let mut t = self.thread.lock(); t.search_matches = Some(matches); t.search_matches_dirty = true; } /// Set the selected search match (from the renderer thread mailbox). pub fn set_search_selected_match(&self, m: Option) { - let mut t = lock(&self.thread); + let mut t = self.thread.lock(); t.search_selected_match = m; t.search_matches_dirty = true; } @@ -1836,7 +1842,7 @@ impl Renderer { /// most recently presented frame. Intended for tests and screenshots; /// requires the GL context to be current. pub fn last_frame_pixels(&self) -> Option<(usize, usize, Vec)> { - let d = lock(&self.draw); + let d = self.draw.lock(); let target = d.api.last_target()?; let gl = d.api.gl()?.clone(); let bind = gl.bind_framebuffer( @@ -1877,7 +1883,7 @@ impl Renderer { state: &SharedState, cursor_blink_visible: bool, ) -> Result<(), Error> { - let mut guard = lock(&self.thread); + let mut guard = self.thread.lock(); let t = &mut *guard; // We fully deinit and reset the terminal state every so often @@ -1944,7 +1950,7 @@ impl Renderer { // kitty state on every frame because any cell change can move // an image. { - let mut d = lock(&self.draw); + let mut d = self.draw.lock(); if d.images.kitty_requires_update(&st.terminal) { let cell_size = CellSize { width: d.grid_metrics.cell_width, @@ -2040,7 +2046,7 @@ impl Renderer { // Acquire the draw mutex for all remaining state updates. { - let mut d = lock(&self.draw); + let mut d = self.draw.lock(); // Build our GPU cells let cursor_style = cursor::style( @@ -2113,7 +2119,7 @@ impl Renderer { return Ok(()); } - let size = lock(&self.draw).size; + let size = self.draw.lock().size; // If we had a previous overlay, clear it. Otherwise, init. let reuse = match &t.overlay { @@ -3207,7 +3213,7 @@ impl Renderer { pub fn draw_frame(&self, sync: bool) -> Result<(), Error> { // We hold a the draw mutex to prevent changes to any // data we access while we're in the middle of drawing. - let mut d = lock(&self.draw); + let mut d = self.draw.lock(); let result = self.draw_frame_locked(&mut d, sync); diff --git a/crates/ghostty-renderer/tests/mock_api.rs b/crates/ghostty-renderer/tests/mock_api.rs index b4bec45..a54f539 100644 --- a/crates/ghostty-renderer/tests/mock_api.rs +++ b/crates/ghostty-renderer/tests/mock_api.rs @@ -11,7 +11,7 @@ use std::{ sync::{ Arc, Mutex, - atomic::{AtomicUsize, Ordering}, + atomic::{AtomicBool, AtomicUsize, Ordering}, }, time::Duration, }; @@ -283,6 +283,13 @@ impl Host for TestHost { } fn fixture() -> (Renderer, Arc>, Log, Arc) { + fixture_with_size(20, 5) +} + +fn fixture_with_size( + columns: u16, + rows: u16, +) -> (Renderer, Arc>, Log, Arc) { let config = Config::default(); let set = Arc::new(SharedGridSet::new().unwrap()); let (_key, grid) = set @@ -300,7 +307,10 @@ fn fixture() -> (Renderer, Arc>, Log, Arc) { width: gc.width, height: gc.height, }; - let (w, h) = (cell.width * 20 + 20, cell.height * 5 + 20); + let (w, h) = ( + cell.width * u32::from(columns) + 20, + cell.height * u32::from(rows) + 20, + ); let size = Size { screen: ScreenSize { width: w, @@ -315,7 +325,7 @@ fn fixture() -> (Renderer, Arc>, Log, Arc) { }, }; - let mut term = Terminal::new(TermOptions::new(20, 5)).unwrap(); + let mut term = Terminal::new(TermOptions::new(columns, rows)).unwrap(); term.vt_stream().next_slice(b"hello").unwrap(); let state = State::new(term).into_shared(); @@ -396,3 +406,61 @@ fn display_unrealize_waits_for_inflight_frames() { renderer.draw_frame(true).unwrap(); assert_eq!(completed.load(Ordering::SeqCst), 2); } + +#[test] +fn display_unrealize_makes_progress_during_continuous_updates() { + let (renderer, state, _log, _completed) = fixture_with_size(200, 150); + // Dense, varied text forces actual shaping and glyph rebuilding rather + // than repeatedly taking the clean-frame shortcut. + let mut seed = 1_u32; + let text: Vec<_> = (0..30_000) + .map(|_| { + seed = seed.wrapping_mul(1_664_525).wrapping_add(1_013_904_223); + b"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/" + [(seed >> 24) as usize % 64] + }) + .collect(); + state + .lock() + .unwrap() + .terminal + .vt_stream() + .next_slice(&text) + .unwrap(); + + let renderer = Arc::new(renderer); + let stop = Arc::new(AtomicBool::new(false)); + let (started_tx, started_rx) = std::sync::mpsc::channel(); + let updater = { + let renderer = Arc::clone(&renderer); + let stop = Arc::clone(&stop); + std::thread::spawn(move || { + renderer.update_frame(&state, true).unwrap(); + started_tx.send(()).unwrap(); + while !stop.load(Ordering::Relaxed) { + renderer.mark_dirty(); + renderer.update_frame(&state, true).unwrap(); + } + }) + }; + started_rx.recv_timeout(Duration::from_secs(5)).unwrap(); + + let (done_tx, done_rx) = std::sync::mpsc::channel(); + let lifecycle = { + let renderer = Arc::clone(&renderer); + std::thread::spawn(move || { + // Closing a split/tab unrealizes its GLArea before worker shutdown. + // Reparenting a surviving surface also realizes it again. + renderer.display_unrealized(); + renderer.display_realized_with(|_| {}).unwrap(); + done_tx.send(()).unwrap(); + }) + }; + let result = done_rx.recv_timeout(Duration::from_secs(3)); + // Always release a starved lifecycle thread so a regression fails instead + // of leaving the test process hung indefinitely. + stop.store(true, Ordering::Relaxed); + updater.join().unwrap(); + lifecycle.join().unwrap(); + assert!(result.is_ok(), "display lifecycle starved by frame updates"); +} diff --git a/docs/rewrite/ARCHITECTURE.md b/docs/rewrite/ARCHITECTURE.md index 160ce0f..8448470 100644 --- a/docs/rewrite/ARCHITECTURE.md +++ b/docs/rewrite/ARCHITECTURE.md @@ -32,6 +32,8 @@ ghostty (bin, src/main.rs) CLI `+action` dispatch or GTK app startup │ ├── kitty / search / tmux kitty graphics, search thread, tmux control mode │ └── ghostty-vt byte stream -> Parser -> Stream -> Handler actions ├── ghostty-renderer OpenGL renderer (glow), renderer thread, shaders + │ ├── generic::Renderer CPU frame state + GPU draw state; fair mutexes + │ │ let GTK display teardown run during output │ └── ghostty-font fontconfig/freetype/harfbuzz, sprite font, atlas ├── ghostty-inspector Dear ImGui terminal inspector └── ghostty-config config file/CLI parsing, themes, keybinds @@ -128,6 +130,12 @@ GTK invokes `glib-compile-resources` directly so generated XML/UI files in thread, plus a pty read thread. Mailboxes are `crossbeam-channel` / `std::sync::mpsc`; the GTK main thread is woken via `glib` channels. The `Terminal` is shared behind `Arc>` (Ghostty's renderer mutex). + The generic renderer uses fair mutexes for its CPU frame and GPU draw state: + continuous frame updates hand ownership to waiting GTK lifecycle/draw + callbacks. Closing a tab/split unrealizes its GLArea before worker shutdown; + teardown must acquire these locks while the PTY is still producing output. + This is a Rust synchronization fix; Zig does not have the extra CPU-state + mutex used by the rewrite. `ghostty-core::App` handles at most 64 mailbox messages per tick and wakes the runtime again if work remains. Continuous renderer notifications must yield to GTK input, timers, and drawing instead of extending one tick