os/exec: remove protection against a duplicate Close on StdinPipe

As of CL 438347, multiple concurrents calls to Close should be safe.

This removes some indirection and may also make some programs that use
type-assertions marginally more efficient. For example, if a program
calls (*exec.Cmd).StdinPipe to obtain a pipe and then sets that as the
Stdout of another command, that program will now allow the second
command to inherit the file descriptor directly instead of copying
everything through a goroutine.

This will also cause calls to Close after the first to return an error
wrapping os.ErrClosed instead of nil. However, it seems unlikely that
programs will depend on that error behavior: if a program is calling
Write in a loop followed by Close, then if a racing Close occurs it is
likely that the Write would have already reported an error. (The only
programs likely to notice a change are those that call Close — without
Write! — after a call to Wait.)

Updates #56043.
Updates #9307.
Updates #6270.

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

diff --git a/src/os/exec/exec.go b/src/os/exec/exec.go
index aa601b6ccc62ca051088b686c730c7c018b7715a..0d7a86bad475ae96d9bfb44841d20deaa2ead249 100644 (file)
--- a/src/os/exec/exec.go
+++ b/src/os/exec/exec.go
@@ -102,7 +102,6 @@ import (
        "runtime"
        "strconv"
        "strings"
-       "sync"
        "syscall"
 )
 
@@ -809,25 +808,8 @@ 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)
-       return wc, nil
-}
-
-type closeOnce struct {
-       *os.File
-
-       once sync.Once
-       err  error
-}
-
-func (c *closeOnce) Close() error {
-       c.once.Do(c.close)
-       return c.err
-}
-
-func (c *closeOnce) close() {
-       c.err = c.File.Close()
+       c.parentIOPipes = append(c.parentIOPipes, pw)
+       return pw, nil
 }
 
 // StdoutPipe returns a pipe that will be connected to the command's

diff --git a/src/os/exec/exec_test.go b/src/os/exec/exec_test.go
index dc8aebd9aa158698e4f081be14f504c1ef83f1ac..d79befa19a13fe10448ec57c5a7d2a746cb6bd7e 100644 (file)
--- a/src/os/exec/exec_test.go
+++ b/src/os/exec/exec_test.go
@@ -11,6 +11,7 @@ import (
        "bufio"
        "bytes"
        "context"
+       "errors"
        "flag"
        "fmt"
        "internal/poll"
@@ -548,12 +549,22 @@ func TestStdinClose(t *testing.T) {
                t.Error("can't access methods of underlying *os.File")
        }
        check("Start", cmd.Start())
+
+       var wg sync.WaitGroup
+       wg.Add(1)
+       defer wg.Wait()
        go func() {
+               defer wg.Done()
+
                _, 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())
+               if err := stdin.Close(); err != nil && !errors.Is(err, os.ErrClosed) {
+                       t.Errorf("Close: %v", err)
+               }
        }()
+
        check("Wait", cmd.Wait())
 }
 
@@ -573,8 +584,15 @@ func TestStdinCloseRace(t *testing.T) {
        }
        if err := cmd.Start(); err != nil {
                t.Fatalf("Start: %v", err)
+
        }
+
+       var wg sync.WaitGroup
+       wg.Add(2)
+       defer wg.Wait()
+
        go func() {
+               defer wg.Done()
                // We don't check the error return of Kill. It is
                // possible that the process has already exited, in
                // which case Kill will return an error "process
@@ -583,17 +601,20 @@ func TestStdinCloseRace(t *testing.T) {
                // doesn't matter whether this Kill succeeds or not.
                cmd.Process.Kill()
        }()
+
        go func() {
+               defer wg.Done()
                // Send the wrong string, so that the child fails even
                // if the other goroutine doesn't manage to kill it first.
                // This test is to check that the race detector does not
                // falsely report an error, so it doesn't matter how the
                // child process fails.
                io.Copy(stdin, strings.NewReader("unexpected string"))
-               if err := stdin.Close(); err != nil {
+               if err := stdin.Close(); err != nil && !errors.Is(err, os.ErrClosed) {
                        t.Errorf("stdin.Close: %v", err)
                }
        }()
+
        if err := cmd.Wait(); err == nil {
                t.Fatalf("Wait: succeeded unexpectedly")
        }