os/exec: clean up pipe-closing logic

Change the childFiles field to a local variable, since it was
populated during Start and (as far as I can determine) has no purpose
after Start returns.

Rename closeAfterStart and closeAfterWait to childIOFiles and
parentIOPipes respectively. That makes their contents clearer, and also
helps to clarify what should happen on error (when, for example, Wait
shouldn't be called at all).

Use a deferred call instead of individual calls to close child (and,
if necessary, pipe) FDs after Start. That helps to clarify the
invariants around when they are closed, and also makes the function a
bit more robust for future refactoring.

Also nil out the slices containing the file closers so that they can
be collected earlier.

This CL is intended as a pure refactor in preparation for #50436.

Change-Id: I05d13fa91d539b95b84b2ba923c1733f9a6203e5
Reviewed-on: https://go-review.googlesource.com/c/go/+/401834
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>

src/os/exec/exec.go

@@ -219,9 +219,19 @@ type Cmd struct {
 
 	ctx context.Context // nil means none
 	Err error           // LookPath error, if any.
-	childFiles      []*os.File
-	closeAfterStart []io.Closer
-	closeAfterWait  []io.Closer
+
+	// childIOFiles holds closers for any of the child process's
+	// stdin, stdout, and/or stderr files that were opened by the Cmd itself
+	// (not supplied by the caller). These should be closed as soon as they
+	// are inherited by the child process.
+	childIOFiles []io.Closer
+
+	// parentIOPipes holds closers for the parent's end of any pipes
+	// connected to the child's stdin, stdout, and/or stderr streams
+	// that were opened by the Cmd itself (not supplied by the caller).
+	// These should be closed after Wait sees the command exit.
+	parentIOPipes []io.Closer
+
 	goroutine     []func() error
 	goroutineErrs <-chan error // one receive per goroutine
 	ctxErr        <-chan error // if non nil, receives the error from watchCtx exactly once
@@ -337,14 +347,14 @@ func (c *Cmd) argv() []string {
 	return []string{c.Path}
 }
 
-func (c *Cmd) stdin() (f *os.File, err error) {
+func (c *Cmd) childStdin() (*os.File, error) {
 	if c.Stdin == nil {
-		f, err = os.Open(os.DevNull)
+		f, err := os.Open(os.DevNull)
 		if err != nil {
-			return
+			return nil, err
 		}
-		c.closeAfterStart = append(c.closeAfterStart, f)
-		return
+		c.childIOFiles = append(c.childIOFiles, f)
+		return f, nil
 	}
 
 	if f, ok := c.Stdin.(*os.File); ok {
@@ -353,11 +363,11 @@ func (c *Cmd) stdin() (f *os.File, err error) {
 
 	pr, pw, err := os.Pipe()
 	if err != nil {
-		return
+		return nil, err
 	}
 
-	c.closeAfterStart = append(c.closeAfterStart, pr)
-	c.closeAfterWait = append(c.closeAfterWait, pw)
+	c.childIOFiles = append(c.childIOFiles, pr)
+	c.parentIOPipes = append(c.parentIOPipes, pw)
 	c.goroutine = append(c.goroutine, func() error {
 		_, err := io.Copy(pw, c.Stdin)
 		if skipStdinCopyError(err) {
@@ -371,25 +381,29 @@ func (c *Cmd) stdin() (f *os.File, err error) {
 	return pr, nil
 }
 
-func (c *Cmd) stdout() (f *os.File, err error) {
+func (c *Cmd) childStdout() (*os.File, error) {
 	return c.writerDescriptor(c.Stdout)
 }
 
-func (c *Cmd) stderr() (f *os.File, err error) {
+func (c *Cmd) childStderr(childStdout *os.File) (*os.File, error) {
 	if c.Stderr != nil && interfaceEqual(c.Stderr, c.Stdout) {
-		return c.childFiles[1], nil
+		return childStdout, nil
 	}
 	return c.writerDescriptor(c.Stderr)
 }
 
-func (c *Cmd) writerDescriptor(w io.Writer) (f *os.File, err error) {
+// writerDescriptor returns an os.File to which the child process
+// can write to send data to w.
+//
+// If w is nil, writerDescriptor returns a File that writes to os.DevNull.
+func (c *Cmd) writerDescriptor(w io.Writer) (*os.File, error) {
 	if w == nil {
-		f, err = os.OpenFile(os.DevNull, os.O_WRONLY, 0)
+		f, err := os.OpenFile(os.DevNull, os.O_WRONLY, 0)
 		if err != nil {
-			return
+			return nil, err
 		}
-		c.closeAfterStart = append(c.closeAfterStart, f)
-		return
+		c.childIOFiles = append(c.childIOFiles, f)
+		return f, nil
 	}
 
 	if f, ok := w.(*os.File); ok {
@@ -398,11 +412,11 @@ func (c *Cmd) writerDescriptor(w io.Writer) (f *os.File, err error) {
 
 	pr, pw, err := os.Pipe()
 	if err != nil {
-		return
+		return nil, err
 	}
 
-	c.closeAfterStart = append(c.closeAfterStart, pw)
-	c.closeAfterWait = append(c.closeAfterWait, pr)
+	c.childIOFiles = append(c.childIOFiles, pw)
+	c.parentIOPipes = append(c.parentIOPipes, pr)
 	c.goroutine = append(c.goroutine, func() error {
 		_, err := io.Copy(w, pr)
 		pr.Close() // in case io.Copy stopped due to write error
@@ -470,12 +484,21 @@ func lookExtensions(path, dir string) (string, error) {
 // After a successful call to Start the Wait method must be called in
 // order to release associated system resources.
 func (c *Cmd) Start() error {
+	started := false
+	defer func() {
+		c.closeDescriptors(c.childIOFiles)
+		c.childIOFiles = nil
+
+		if !started {
+			c.closeDescriptors(c.parentIOPipes)
+			c.parentIOPipes = nil
+		}
+	}()
+
 	if c.Path == "" && c.Err == nil && c.lookPathErr == nil {
 		c.Err = errors.New("exec: no command")
 	}
 	if c.Err != nil || c.lookPathErr != nil {
-		c.closeDescriptors(c.closeAfterStart)
-		c.closeDescriptors(c.closeAfterWait)
 		if c.lookPathErr != nil {
 			return c.lookPathErr
 		}
@@ -484,8 +507,6 @@ func (c *Cmd) Start() error {
 	if runtime.GOOS == "windows" {
 		lp, err := lookExtensions(c.Path, c.Dir)
 		if err != nil {
-			c.closeDescriptors(c.closeAfterStart)
-			c.closeDescriptors(c.closeAfterWait)
 			return err
 		}
 		c.Path = lp
@@ -496,25 +517,28 @@ func (c *Cmd) Start() error {
 	if c.ctx != nil {
 		select {
 		case <-c.ctx.Done():
-			c.closeDescriptors(c.closeAfterStart)
-			c.closeDescriptors(c.closeAfterWait)
 			return c.ctx.Err()
 		default:
 		}
 	}
 
-	c.childFiles = make([]*os.File, 0, 3+len(c.ExtraFiles))
-	type F func(*Cmd) (*os.File, error)
-	for _, setupFd := range []F{(*Cmd).stdin, (*Cmd).stdout, (*Cmd).stderr} {
-		fd, err := setupFd(c)
+	childFiles := make([]*os.File, 0, 3+len(c.ExtraFiles))
+	stdin, err := c.childStdin()
 	if err != nil {
-			c.closeDescriptors(c.closeAfterStart)
-			c.closeDescriptors(c.closeAfterWait)
 		return err
 	}
-		c.childFiles = append(c.childFiles, fd)
+	childFiles = append(childFiles, stdin)
+	stdout, err := c.childStdout()
+	if err != nil {
+		return err
 	}
-	c.childFiles = append(c.childFiles, c.ExtraFiles...)
+	childFiles = append(childFiles, stdout)
+	stderr, err := c.childStderr(stdout)
+	if err != nil {
+		return err
+	}
+	childFiles = append(childFiles, stderr)
+	childFiles = append(childFiles, c.ExtraFiles...)
 
 	env, err := c.environ()
 	if err != nil {
@@ -523,17 +547,14 @@ func (c *Cmd) Start() error {
 
 	c.Process, err = os.StartProcess(c.Path, c.argv(), &os.ProcAttr{
 		Dir:   c.Dir,
-		Files: c.childFiles,
+		Files: childFiles,
 		Env:   env,
 		Sys:   c.SysProcAttr,
 	})
 	if err != nil {
-		c.closeDescriptors(c.closeAfterStart)
-		c.closeDescriptors(c.closeAfterWait)
 		return err
 	}
-
-	c.closeDescriptors(c.closeAfterStart)
+	started = true
 
 	// Don't allocate the goroutineErrs channel unless there are goroutines to fire.
 	if len(c.goroutine) > 0 {
@@ -626,8 +647,8 @@ func (c *Cmd) Wait() error {
 		err = copyError
 	}
 
-	c.closeDescriptors(c.closeAfterWait)
-	c.closeAfterWait = nil
+	c.closeDescriptors(c.parentIOPipes)
+	c.parentIOPipes = nil
 
 	return err
 }
@@ -726,9 +747,9 @@ func (c *Cmd) StdinPipe() (io.WriteCloser, error) {
 		return nil, err
 	}
 	c.Stdin = pr
-	c.closeAfterStart = append(c.closeAfterStart, pr)
+	c.childIOFiles = append(c.childIOFiles, pr)
 	wc := &closeOnce{File: pw}
-	c.closeAfterWait = append(c.closeAfterWait, wc)
+	c.parentIOPipes = append(c.parentIOPipes, wc)
 	return wc, nil
 }
 
@@ -768,8 +789,8 @@ func (c *Cmd) StdoutPipe() (io.ReadCloser, error) {
 		return nil, err
 	}
 	c.Stdout = pw
-	c.closeAfterStart = append(c.closeAfterStart, pw)
-	c.closeAfterWait = append(c.closeAfterWait, pr)
+	c.childIOFiles = append(c.childIOFiles, pw)
+	c.parentIOPipes = append(c.parentIOPipes, pr)
 	return pr, nil
 }
 
@@ -793,8 +814,8 @@ func (c *Cmd) StderrPipe() (io.ReadCloser, error) {
 		return nil, err
 	}
 	c.Stderr = pw
-	c.closeAfterStart = append(c.closeAfterStart, pw)
-	c.closeAfterWait = append(c.closeAfterWait, pr)
+	c.childIOFiles = append(c.childIOFiles, pw)
+	c.parentIOPipes = append(c.parentIOPipes, pr)
 	return pr, nil
 }