Skip to content

fix: macOS pty race with dropped output #7278

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 25, 2023
Merged

fix: macOS pty race with dropped output #7278

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 18 additions & 10 deletions pty/start_other.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,17 +50,25 @@ func startPty(cmd *exec.Cmd, opt ...StartOption) (retPTY *otherPty, proc Process
}
return nil, nil, xerrors.Errorf("start: %w", err)
}
// Now that we've started the command, and passed the TTY to it, close our
// file so that the other process has the only open file to the TTY. Once
// the process closes the TTY (usually on exit), there will be no open
// references and the OS kernel returns an error when trying to read or
// write to our PTY end. Without this, reading from the process output
// will block until we close our TTY.
if err := opty.tty.Close(); err != nil {
_ = cmd.Process.Kill()
return nil, nil, xerrors.Errorf("close tty: %w", err)
if runtime.GOOS == "linux" {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if, down the line, this might give us a bad time trying to support other OSs (e.g. BSD variants).

I found a SO post that confirms Darwin like behavior on FreeBSD (8.2) as well, but apparently the behavior may be closer to Linux in 10.0 which could mean we may block on output copy.

https://stackoverflow.com/a/25123701

A comment mentions checking tcgetattr on master to decide if we keep both ends open but no idea hire reliable that is: https://stackoverflow.com/questions/23458160/final-output-on-slave-pty-is-lost-if-it-was-not-closed-in-parent-why/25123701#comment36273878_23458160

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It definitely might!

But, the unit test we have should fail if the BSD version we intend to support acts like Linux and doesn't get the close, so I guess we'll just cross that bridge if we come to it.

// Now that we've started the command, and passed the TTY to it, close
// our file so that the other process has the only open file to the TTY.
// Once the process closes the TTY (usually on exit), there will be no
// open references and the OS kernel returns an error when trying to
// read or write to our PTY end. Without this (on Linux), reading from
// the process output will block until we close our TTY.
//
// Note that on darwin, reads on the PTY don't block even if we keep the
// TTY file open, and keeping it open seems to prevent race conditions
// where we lose output. Couldn't find official documentation
// confirming this, but I did find a thread of someone else's
// observations: https://developer.apple.com/forums/thread/663632
if err := opty.tty.Close(); err != nil {
_ = cmd.Process.Kill()
return nil, nil, xerrors.Errorf("close tty: %w", err)
}
opty.tty = nil // remove so we don't attempt to close it again.
}
opty.tty = nil // remove so we don't attempt to close it again.
oProcess := &otherProcess{
pty: opty.pty,
cmd: cmd,
Expand Down