os/exec: deflake TestPipeLookPathLeak

The number of open file descriptors reported by lsof is unreliable
because it depends on whether the parent process (the test) closed
the file descriptors it passed into the child process (lsof) before
lsof runs.

Reading /proc/self/fd directly on Linux appears to be much more
reliable and still detects any file descriptor leaks originating
from attempting to run an executable that cannot be found (issue
#5071). If /proc/self/fd is not available (e.g. on Darwin) then we
fall back to lsof and tolerate small differences in open file
descriptor counts.

Fixes #19243.

Change-Id: I052b0c129e609010f1083e43a9911cba154117bf
Reviewed-on: https://go-review.googlesource.com/37343
Run-TryBot: Michael Munday <munday@ca.ibm.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>

diff --git a/src/os/exec/exec_test.go b/src/os/exec/exec_test.go
index 5b8627db54b0d4ea40da016a1c11c678848dfa5e..13bc39794b08a3af194292ef1957f156f3ff9f18 100644 (file)
--- a/src/os/exec/exec_test.go
+++ b/src/os/exec/exec_test.go
@@ -289,8 +289,51 @@ func TestStdinCloseRace(t *testing.T) {
 
 // Issue 5071
 func TestPipeLookPathLeak(t *testing.T) {
-       fd0, lsof0 := numOpenFDS(t)
-       for i := 0; i < 4; i++ {
+       // If we are reading from /proc/self/fd we (should) get an exact result.
+       tolerance := 0
+
+       // Reading /proc/self/fd is more reliable than calling lsof, so try that
+       // first.
+       numOpenFDs := func() (int, []byte, error) {
+               fds, err := ioutil.ReadDir("/proc/self/fd")
+               if err != nil {
+                       return 0, nil, err
+               }
+               return len(fds), nil, nil
+       }
+       want, before, err := numOpenFDs()
+       if err != nil {
+               // We encountered a problem reading /proc/self/fd (we might be on
+               // a platform that doesn't have it). Fall back onto lsof.
+               t.Logf("using lsof because: %v", err)
+               numOpenFDs = func() (int, []byte, error) {
+                       // Android's stock lsof does not obey the -p option,
+                       // so extra filtering is needed.
+                       // https://golang.org/issue/10206
+                       if runtime.GOOS == "android" {
+                               // numOpenFDsAndroid handles errors itself and
+                               // might skip or fail the test.
+                               n, lsof := numOpenFDsAndroid(t)
+                               return n, lsof, nil
+                       }
+                       lsof, err := exec.Command("lsof", "-b", "-n", "-p", strconv.Itoa(os.Getpid())).Output()
+                       return bytes.Count(lsof, []byte("\n")), lsof, err
+               }
+
+               // lsof may see file descriptors associated with the fork itself,
+               // so we allow some extra margin if we have to use it.
+               // https://golang.org/issue/19243
+               tolerance = 5
+
+               // Retry reading the number of open file descriptors.
+               want, before, err = numOpenFDs()
+               if err != nil {
+                       t.Log(err)
+                       t.Skipf("skipping test; error finding or running lsof")
+               }
+       }
+
+       for i := 0; i < 6; i++ {
                cmd := exec.Command("something-that-does-not-exist-binary")
                cmd.StdoutPipe()
                cmd.StderrPipe()
@@ -299,36 +342,20 @@ func TestPipeLookPathLeak(t *testing.T) {
                        t.Fatal("unexpected success")
                }
        }
-       for triesLeft := 3; triesLeft >= 0; triesLeft-- {
-               open, lsof := numOpenFDS(t)
-               fdGrowth := open - fd0
-               if fdGrowth > 2 {
-                       if triesLeft > 0 {
-                               // Work around what appears to be a race with Linux's
-                               // proc filesystem (as used by lsof). It seems to only
-                               // be eventually consistent. Give it awhile to settle.
-                               // See golang.org/issue/7808
-                               time.Sleep(100 * time.Millisecond)
-                               continue
-                       }
-                       t.Errorf("leaked %d fds; want ~0; have:\n%s\noriginally:\n%s", fdGrowth, lsof, lsof0)
-               }
-               break
-       }
-}
-
-func numOpenFDS(t *testing.T) (n int, lsof []byte) {
-       if runtime.GOOS == "android" {
-               // Android's stock lsof does not obey the -p option,
-               // so extra filtering is needed. (golang.org/issue/10206)
-               return numOpenFDsAndroid(t)
-       }
-
-       lsof, err := exec.Command("lsof", "-b", "-n", "-p", strconv.Itoa(os.Getpid())).Output()
+       got, after, err := numOpenFDs()
        if err != nil {
-               t.Skip("skipping test; error finding or running lsof")
+               // numOpenFDs has already succeeded once, it should work here.
+               t.Errorf("unexpected failure: %v", err)
+       }
+       if got-want > tolerance {
+               t.Errorf("number of open file descriptors changed: got %v, want %v", got, want)
+               if before != nil {
+                       t.Errorf("before:\n%v\n", before)
+               }
+               if after != nil {
+                       t.Errorf("after:\n%v\n", after)
+               }
        }
-       return bytes.Count(lsof, []byte("\n")), lsof
 }
 
 func numOpenFDsAndroid(t *testing.T) (n int, lsof []byte) {