fix-deviant(termio): stop waiting for a child that died before setsid
Intentional deviation from Ghostty 1.3.1 (DEVIANT_FIXES.md row 37). On shutdown Zig's termio/Exec.zig:1188-1224 (getpgid) loops until the child's process group differs from ours, i.e. until the child has called setsid. If the child exits before ever calling setsid and is not yet reaped, its pgid stays ours forever and Zig spins indefinitely. The Rust port previously capped the wait at ~5 s (which also differed from Zig for slow children). It now waits without a cap like Zig, but stops as soon as waitid(WNOWAIT) shows the child has exited. Test Plan: - cargo test -p ghostty-termio getpgid_waits_for_late_setsid getpgid_gives_up_on_exited_child_without_setsid
This commit is contained in:
1 parent
1431a8a1d8
commit
5c5962ea95
2 files changed
+63
-3
No files matched your search
@@ -1344,11 +1344,18 @@ fn getpgid(pid: Pid) -> Option<Pid> {
|
||||
// We loop while pgid == my_pgid. The expectation if we have a valid
|
||||
// pid is that setsid will eventually be called because it is the
|
||||
// FIRST thing the child process does and as far as I can tell,
|
||||
// setsid cannot fail. (Zig loops forever; we give up after ~5s.)
|
||||
for _ in 0..500 {
|
||||
// setsid cannot fail. Like Zig we wait without a cap, with one
|
||||
// exception (DEVIANT_FIXES.md): if the child already exited without
|
||||
// ever creating its own group (still an unreaped zombie, whose pgid
|
||||
// stays ours forever) Zig would spin here forever.
|
||||
loop {
|
||||
match rustix::process::getpgid(Some(pid)) {
|
||||
Ok(pgid) => {
|
||||
if Some(pgid) == my_pgid {
|
||||
if child_exited(pid) {
|
||||
log::warn!("child exited before creating its process group");
|
||||
return None;
|
||||
}
|
||||
log::warn!("pgid is our own, retrying");
|
||||
std::thread::sleep(Duration::from_millis(10));
|
||||
continue;
|
||||
@@ -1364,7 +1371,20 @@ fn getpgid(pid: Pid) -> Option<Pid> {
|
||||
}
|
||||
}
|
||||
}
|
||||
None
|
||||
}
|
||||
|
||||
/// Whether `pid` has terminated (without reaping it).
|
||||
fn child_exited(pid: Pid) -> bool {
|
||||
use rustix::process::{WaitId, WaitIdOptions, waitid};
|
||||
match waitid(
|
||||
WaitId::Pid(pid),
|
||||
WaitIdOptions::EXITED | WaitIdOptions::NOHANG | WaitIdOptions::NOWAIT,
|
||||
) {
|
||||
Ok(Some(_)) => true,
|
||||
Ok(None) => false,
|
||||
// Not our child (or already reaped): nothing more to wait for.
|
||||
Err(_) => true,
|
||||
}
|
||||
}
|
||||
|
||||
/// Builds the argv array for the process we should exec for the
|
||||
@@ -1740,4 +1760,43 @@ mod tests {
|
||||
assert_eq!(exit_code(Exit::Exited(3)), 3);
|
||||
assert_eq!(exit_code(Exit::Signal(9)), 9);
|
||||
}
|
||||
|
||||
#[cfg(unix)]
|
||||
fn pid_of(child: &std::process::Child) -> Pid {
|
||||
Pid::from_raw(i32::try_from(child.id()).unwrap()).unwrap()
|
||||
}
|
||||
|
||||
/// Like Zig, there is no cap on waiting for the child's `setsid`: a
|
||||
/// child that creates its group later than the old ~5 s cap's
|
||||
/// granularity is still found (a short delay here keeps the test fast).
|
||||
#[cfg(unix)]
|
||||
#[test]
|
||||
fn getpgid_waits_for_late_setsid() {
|
||||
let mut child = std::process::Command::new("sh")
|
||||
.args(["-c", "sleep 0.4; exec setsid sleep 3"])
|
||||
.spawn()
|
||||
.unwrap();
|
||||
let pid = pid_of(&child);
|
||||
let start = std::time::Instant::now();
|
||||
let pgid = getpgid(pid);
|
||||
assert_eq!(pgid, Some(pid));
|
||||
assert!(start.elapsed() >= Duration::from_millis(300));
|
||||
let _ = child.kill();
|
||||
let _ = child.wait();
|
||||
}
|
||||
|
||||
/// Deviation from Zig (DEVIANT_FIXES.md): a child that exited without
|
||||
/// ever calling `setsid` (unreaped zombie, pgid == ours) does not make
|
||||
/// the wait spin forever.
|
||||
#[cfg(unix)]
|
||||
#[test]
|
||||
fn getpgid_gives_up_on_exited_child_without_setsid() {
|
||||
let mut child = std::process::Command::new("true").spawn().unwrap();
|
||||
let pid = pid_of(&child);
|
||||
std::thread::sleep(Duration::from_millis(200));
|
||||
let start = std::time::Instant::now();
|
||||
assert_eq!(getpgid(pid), None);
|
||||
assert!(start.elapsed() < Duration::from_secs(2));
|
||||
let _ = child.wait();
|
||||
}
|
||||
}
|
||||
@@ -62,6 +62,7 @@ when this file was written.
|
||||
| 34 | Renderer (Metal): buffer reallocation size | `renderer/metal/buffer.zig:89` (`Buffer.sync`) and `:146` (`syncFromArrayLists`) | `size = req_bytes * 2` is already in bytes, then `size * @sizeOf(T)` is passed to `newBufferWithLength:options:`, so the new buffer is `sizeof(T)` times larger than intended (GPU-memory over-allocation). | The new buffer is exactly `2 * req_bytes` (`metal_layout::grown_buffer_bytes`); the element count is `2 * total_len`. | bab49ee; registered + test in `2040677` fix-deviant(renderer) | `metal_layout::tests::grown_buffer_bytes_is_double_the_required_bytes_not_scaled_by_size_of_t` (runs on any host; the Metal buffer itself is compile-checked only) |
|
||||
| 35 | Termio: PTY hangup drops unread output | `termio/Exec.zig` `ReadThread.threadMainPosix` (exits as soon as `POLL.HUP` is seen) | `POLLIN \| POLLHUP` can arrive together with unread final output (a Linux PTY check confirmed this); exiting immediately loses the tail of the child's output. | After HUP the read thread makes one more pass over the fd, so the remaining output is delivered before exit. | 6629fb0 | none (needs a live PTY; verified by RV18 on Linux) |
|
||||
| 36 | Font (wasm/browser Canvas): atlas pixel format | `font/face/web_canvas.zig` (atlas write branch on `.rgba`) | The branch tests a stale `.rgba` format against an enum that contains `.bgra`, so BGRA atlases are never converted. | The BGRA atlas conversion is implemented. No claim of equivalence with a real browser (not testable here). | d89691b | `ghostty-font` web_canvas tests (compile/Node level) |
|
||||
| 37 | Termio: subprocess cleanup | `termio/Exec.zig:1188-1224` (`getpgid`) | Loops forever while `getpgid(child) == our pgid`, i.e. also when the child died before `setsid` and is an unreaped zombie (its pgid stays ours), hanging the surface close. | Still waits without a cap (no 5 s limit) while the child is alive, but stops (no SIGHUP, nothing to kill) once `waitid(WNOWAIT)` shows the child exited. | uncommitted (P3) | `getpgid_gives_up_on_exited_child_without_setsid`, `getpgid_waits_for_late_setsid` (`ghostty-termio/src/exec.rs`) |
|
||||
|
||||
## Upstream quirks deliberately kept (not fixed)
|
||||
|
||||
|
||||
Reference in new issue
Block a user