From: Bryan C. Mills <bcmills@google.com>
Date: Sat, 23 Apr 2022 01:19:10 +0000 (-0400)
Subject: os/exec: split parent I/O pipes by whether they are pumped by user code or internal... 
X-Git-Tag: go1.20rc1~936
X-Git-Url: http://www.git.cypherpunks.su/?a=commitdiff_plain;h=bd8a5b00fcf71fb711ba8996a880b5b07f7b3634;p=gostls13.git

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

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 9bef38533e..67dd379b71 100644
--- 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
 }