Skip to content

fix: close the replaced cancel reader on RestoreTerminal - #1821

Open
drmzperx wants to merge 1 commit into
charmbracelet:mainfrom
drmzperx:fix/close-replaced-cancelreader
Open

drmzperx wants to merge 1 commit into
charmbracelet:mainfrom
drmzperx:fix/close-replaced-cancelreader

Conversation

@drmzperx

Copy link
Copy Markdown
  • I have read CONTRIBUTING.md.
  • I have created a discussion that was approved by a maintainer (for new features).

What

initInputReader replaces p.cancelReader without closing the previous one. ReleaseTerminal only calls Cancel() on it, and RestoreTerminal then creates a new reader over it. The old reader's resources are never released.

On Linux the cancel reader is epoll-based (muesli/cancelreader), so each ReleaseTerminal/RestoreTerminal cycle (every tea.Exec, every suspend/resume) leaks one epoll file descriptor. The reader's cancel pipe is an *os.File and eventually gets reclaimed by its finalizer. The raw epoll fd is a plain int and never does.

We hit this in a long-running TUI that execs into tmux attach whenever the user opens a session. After 9 days the process held 896 open fds, 856 of them anon_inode:[eventpoll]. The same code is in v1.3.10.

Fix

Close the reader that is being replaced, but only once its read loop has exited (readLoopDone is closed). If waitForReadLoop timed out, the loop may still be reading from the old reader, so it is left alone, as before.

shutdown still closes the current reader as it does today, so there is no double close.

Before / after

tty_linux_test.go runs 20 ReleaseTerminal/RestoreTerminal cycles on a program reading from an os.Pipe and counts anon_inode:[eventpoll] entries in /proc/self/fd.

Before:

--- FAIL: TestReleaseRestoreTerminalClosesCancelReader
    tty_linux_test.go:88: epoll file descriptors grew by 20 over 20 release/restore cycles (before=2, after=22)

After: passes (also with -race -count=20).

Also checked: go test -race ./..., golangci-lint v2.9 (0 issues), go vet for linux/windows/darwin/freebsd, and the examples module builds.

initInputReader only cancels the reader it replaces and never closes it,
so every ReleaseTerminal/RestoreTerminal cycle (Exec, suspend) leaks the
reader's resources. On Linux that is one epoll file descriptor per cycle.

Close the old reader once its read loop has exited. If waitForReadLoop
timed out the loop may still be using it, so it is left alone.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant