Skip to content

safety: draw_terminal holds TERM_STATE lock while calling AppKit drawing functions #45

@rmzi

Description

@rmzi

Description

`draw_terminal` (called from `drawRect:` on the main thread) holds the `TERM_STATE` mutex for the entire duration of terminal rendering, including all `draw_run` calls that allocate `NSString`/`NSDictionary` objects and call into AppKit:

```rust
fn draw_terminal() {
let parser = TERM_STATE.get().unwrap().lock().unwrap(); // lock acquired
let screen = parser.screen();
// ... iterates all rows/cols, calling draw_run for each run
// draw_run calls NSString::from_str, NSDictionary::initWithObjects, ns_str.drawAtPoint
// lock is held for ALL of this
} // lock released here
```

The reader thread (spawned in `spawn_pty_and_reader`) also holds `TERM_STATE` while processing PTY output:

```rust
let mut parser = TERM_STATE.get().unwrap().lock().unwrap();
parser.process(&buf[..n]); // lock held during vt100 parsing
```

This means:

  1. The reader thread is blocked for the full duration of every redraw (which involves AppKit calls, font metrics, and multiple NSObject allocations).
  2. The main thread is blocked from drawing while the reader thread processes a large chunk of PTY output.

For typical terminal output this is fine. For bursty output (e.g., `cat` of a large file), the reader thread can accumulate significant latency while waiting for the draw lock.

This is also a correctness concern: AppKit calls inside a Mutex lock are generally discouraged because AppKit can call back into the app (e.g., via `drawRect:` re-entrancy) and attempt to acquire the same lock, causing deadlock. While `drawRect:` re-entrancy from within `drawRect:` is prevented by AppKit's internal draw lock, other paths (e.g., resize notification during draw) are less obviously safe.

Location

  • `src/tray.rs:217-270` — `draw_terminal` holds `TERM_STATE` lock during AppKit drawing
  • `src/tray.rs:400-411` — reader thread holds `TERM_STATE` lock during PTY parsing

Desired State

Snapshot the terminal state (clone the relevant screen data) while holding the lock, then release the lock before performing AppKit drawing:

```rust
fn draw_terminal() {
let snapshot: Vec = {
let parser = TERM_STATE.get().unwrap().lock().unwrap();
// collect rows into owned data
collect_snapshot(parser.screen())
}; // lock released
// perform all AppKit drawing with snapshot
}
```

This trades a snapshot allocation for lock-hold reduction and eliminates the AppKit-under-lock concern.

Effort / Priority

medium / medium

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions