Apr 25, 2023 · Apr 25, 2023 · Apr 25, 2023 · Apr 25, 2023
diff --git a/pty/start_other.go b/pty/start_other.go
 }
 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" {
 // 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,
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 mafredriApr 25, 2023 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 ContributorAuthor spikecurtisApr 25, 2023 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. mafredri reacted with thumbs up emoji
		// 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