os/exec: split parent I/O pipes by whether they are pumped by user code or internal...

The pipes pumped by goroutines can be closed as soon as their
respective goroutines are done.

The pipes pumped by user code, however, are documented to be closed in
Wait. When we add the WaitDelay field, it isn't obvious that we should
terminate the user-pumped pipes when the WaitDelay expires, since Wait
itself isn't going to wait for those user-controlled goroutines to
complete.

(It's a bit more complicated than that because the documentation
currently states that Wait must not be called while the pipes are
being read — but it isn't obvious to me that that advice is entirely
correct.)

For #50436.

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

diff --git a/src/os/exec/exec.go b/src/os/exec/exec.go
index 9bef38533e5819d9146f79a572d89df4dd717460..67dd379b719b1170ecd0c098448bf11497351613 100644 (file)
--- a/src/os/exec/exec.go
+++ b/src/os/exec/exec.go
@@ -226,11 +226,17 @@ type Cmd struct {
        // are inherited by the child process.
        childIOFiles []io.Closer
 
-       // parentIOPipes holds closers for the parent's end of any pipes
+       // goroutinePipes 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
+       // that are pumped (and ultimately closed) by goroutines controlled by
+       // the Cmd itself (not supplied by or returned to the caller).
+       goroutinePipes []io.Closer
+
+       // userPipes holds closers for the parent's end of any pipes
+       // connected to the child's stdin, stdout and/or stderr streams
+       // that were opened and returned by the Cmd's Pipe methods.
+       // These should be closed when Wait completes.
+       userPipes []io.Closer
 
        goroutine     []func() error
        goroutineErrs <-chan error // one receive per goroutine
@@ -367,7 +373,7 @@ func (c *Cmd) childStdin() (*os.File, error) {
        }
 
        c.childIOFiles = append(c.childIOFiles, pr)
-       c.parentIOPipes = append(c.parentIOPipes, pw)
+       c.goroutinePipes = append(c.goroutinePipes, pw)
        c.goroutine = append(c.goroutine, func() error {
                _, err := io.Copy(pw, c.Stdin)
                if skipStdinCopyError(err) {
@@ -416,7 +422,7 @@ func (c *Cmd) writerDescriptor(w io.Writer) (*os.File, error) {
        }
 
        c.childIOFiles = append(c.childIOFiles, pw)
-       c.parentIOPipes = append(c.parentIOPipes, pr)
+       c.goroutinePipes = append(c.goroutinePipes, pr)
        c.goroutine = append(c.goroutine, func() error {
                _, err := io.Copy(w, pr)
                pr.Close() // in case io.Copy stopped due to write error
@@ -490,8 +496,10 @@ func (c *Cmd) Start() error {
                c.childIOFiles = nil
 
                if !started {
-                       c.closeDescriptors(c.parentIOPipes)
-                       c.parentIOPipes = nil
+                       c.closeDescriptors(c.goroutinePipes)
+                       c.goroutinePipes = nil
+                       c.closeDescriptors(c.userPipes)
+                       c.userPipes = nil
                }
        }()
 
@@ -630,7 +638,8 @@ func (c *Cmd) Wait() error {
                        copyError = err
                }
        }
-       c.goroutine = nil // Allow the goroutines' closures to be GC'd.
+       c.goroutine = nil      // Allow the goroutines' closures to be GC'd.
+       c.goroutinePipes = nil // Already closed by their respective goroutines.
 
        if c.ctxErr != nil {
                interruptErr := <-c.ctxErr
@@ -647,8 +656,8 @@ func (c *Cmd) Wait() error {
                err = copyError
        }
 
-       c.closeDescriptors(c.parentIOPipes)
-       c.parentIOPipes = nil
+       c.closeDescriptors(c.userPipes)
+       c.userPipes = nil
 
        return err
 }
@@ -749,7 +758,7 @@ func (c *Cmd) StdinPipe() (io.WriteCloser, error) {
        c.Stdin = pr
        c.childIOFiles = append(c.childIOFiles, pr)
        wc := &closeOnce{File: pw}
-       c.parentIOPipes = append(c.parentIOPipes, wc)
+       c.userPipes = append(c.userPipes, wc)
        return wc, nil
 }
 
@@ -790,7 +799,7 @@ func (c *Cmd) StdoutPipe() (io.ReadCloser, error) {
        }
        c.Stdout = pw
        c.childIOFiles = append(c.childIOFiles, pw)
-       c.parentIOPipes = append(c.parentIOPipes, pr)
+       c.userPipes = append(c.userPipes, pr)
        return pr, nil
 }
 
@@ -815,7 +824,7 @@ func (c *Cmd) StderrPipe() (io.ReadCloser, error) {
        }
        c.Stderr = pw
        c.childIOFiles = append(c.childIOFiles, pw)
-       c.parentIOPipes = append(c.parentIOPipes, pr)
+       c.userPipes = append(c.userPipes, pr)
        return pr, nil
 }