os/exec: return idempotent Closer from StdinPipe

Before this fix, it was always an error to use the Close method on the
io.WriteCloser obtained from Cmd.StdinPipe, as it would race with the
Close performed by Cmd.Wait.

Fixes #6270.

R=golang-dev, r, remyoudompheng, bradfitz, dsymonds
CC=golang-dev
https://golang.org/cl/13329043

diff --git a/src/pkg/os/exec/exec.go b/src/pkg/os/exec/exec.go
index a3bbcf3005a44d0fe6e7c7296431eacff23c77b6..582930f2c42282eaf3c53df4cc68c68b84edc1d3 100644 (file)
--- a/src/pkg/os/exec/exec.go
+++ b/src/pkg/os/exec/exec.go
@@ -13,6 +13,7 @@ import (
        "io"
        "os"
        "strconv"
+       "sync"
        "syscall"
 )
 
@@ -357,6 +358,8 @@ func (c *Cmd) CombinedOutput() ([]byte, error) {
 
 // StdinPipe returns a pipe that will be connected to the command's
 // standard input when the command starts.
+// If the returned WriteCloser is not closed before Wait is called,
+// Wait will close it.
 func (c *Cmd) StdinPipe() (io.WriteCloser, error) {
        if c.Stdin != nil {
                return nil, errors.New("exec: Stdin already set")
@@ -370,8 +373,23 @@ func (c *Cmd) StdinPipe() (io.WriteCloser, error) {
        }
        c.Stdin = pr
        c.closeAfterStart = append(c.closeAfterStart, pr)
-       c.closeAfterWait = append(c.closeAfterWait, pw)
-       return pw, nil
+       wc := &closeOnce{File: pw}
+       c.closeAfterWait = append(c.closeAfterWait, wc)
+       return wc, nil
+}
+
+type closeOnce struct {
+       *os.File
+
+       close    sync.Once
+       closeErr error
+}
+
+func (c *closeOnce) Close() error {
+       c.close.Do(func() {
+               c.closeErr = c.File.Close()
+       })
+       return c.closeErr
 }
 
 // StdoutPipe returns a pipe that will be connected to the command's

diff --git a/src/pkg/os/exec/exec_test.go b/src/pkg/os/exec/exec_test.go
index d7e8573a084137cf1987820264e2b26ab1c0d0eb..c380d6506c4bb6e87933f9dd3d181433d5c73e28 100644 (file)
--- a/src/pkg/os/exec/exec_test.go
+++ b/src/pkg/os/exec/exec_test.go
@@ -152,6 +152,34 @@ func TestPipes(t *testing.T) {
        check("Wait", err)
 }
 
+const stdinCloseTestString = "Some test string."
+
+// Issue 6270.
+func TestStdinClose(t *testing.T) {
+       check := func(what string, err error) {
+               if err != nil {
+                       t.Fatalf("%s: %v", what, err)
+               }
+       }
+       cmd := helperCommand("stdinClose")
+       stdin, err := cmd.StdinPipe()
+       check("StdinPipe", err)
+       // Check that we can access methods of the underlying os.File.`
+       if _, ok := stdin.(interface {
+               Fd() uintptr
+       }); !ok {
+               t.Error("can't access methods of underlying *os.File")
+       }
+       check("Start", cmd.Start())
+       go func() {
+               _, err := io.Copy(stdin, strings.NewReader(stdinCloseTestString))
+               check("Copy", err)
+               // Before the fix, this next line would race with cmd.Wait.
+               check("Close", stdin.Close())
+       }()
+       check("Wait", cmd.Wait())
+}
+
 // Issue 5071
 func TestPipeLookPathLeak(t *testing.T) {
        fd0 := numOpenFDS(t)
@@ -507,6 +535,17 @@ func TestHelperProcess(*testing.T) {
                                os.Exit(1)
                        }
                }
+       case "stdinClose":
+               b, err := ioutil.ReadAll(os.Stdin)
+               if err != nil {
+                       fmt.Fprintf(os.Stderr, "Error: %v\n", err)
+                       os.Exit(1)
+               }
+               if s := string(b); s != stdinCloseTestString {
+                       fmt.Fprintf(os.Stderr, "Error: Read %q, want %q", s, stdinCloseTestString)
+                       os.Exit(1)
+               }
+               os.Exit(0)
        case "read3": // read fd 3
                fd3 := os.NewFile(3, "fd3")
                bs, err := ioutil.ReadAll(fd3)