-
Notifications
You must be signed in to change notification settings - Fork 47
Description
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:
- The reader thread is blocked for the full duration of every redraw (which involves AppKit calls, font metrics, and multiple NSObject allocations).
- 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