From 293fa4b688d8da57c5b8429f13ab9f7593cb7168 Mon Sep 17 00:00:00 2001
From: Lorenz Bauer <lmb@cloudflare.com>
Date: Mon, 25 Jan 2021 15:14:23 +0000
Subject: [PATCH] fix flaky TestFilesAreNonblocking on CI

CI runs test with stdin closed, which leads our victim cat process
to exit quickly. Depending on the ordering of events we might see
the test fail. Change the contract for newOSProcess to include
stdin, stdout and stderr and allocate a blocking pipe as stdin for
the test.
---
 child.go        |  2 +-
 child_test.go   | 12 ++++++------
 process.go      |  3 +--
 process_test.go | 26 ++++++++++++++++++--------
 4 files changed, 26 insertions(+), 17 deletions(-)

diff --git a/child.go b/child.go
index 82ed447..1b5e04d 100644
--- a/child.go
+++ b/child.go
@@ -31,7 +31,7 @@ func startChild(env *env, passedFiles map[fileName]*file) (*child, error) {
 	}
 
 	// Copy passed fds and append the notification pipe
-	fds := []*os.File{readyW, namesR}
+	fds := []*os.File{os.Stdin, os.Stdout, os.Stderr, readyW, namesR}
 	var fdNames [][]string
 	for name, file := range passedFiles {
 		nameSlice := make([]string, len(name))
diff --git a/child_test.go b/child_test.go
index dc15c73..bd32804 100644
--- a/child_test.go
+++ b/child_test.go
@@ -83,8 +83,8 @@ func TestChildPassedFds(t *testing.T) {
 	}
 
 	in := map[fileName]*file{
-		fileName{"r"}: newFile(r.Fd(), fileName{"r"}),
-		fileName{"w"}: newFile(w.Fd(), fileName{"w"}),
+		{"r"}: newFile(r.Fd(), fileName{"r"}),
+		{"w"}: newFile(w.Fd(), fileName{"w"}),
 	}
 
 	if _, err := startChild(env, in); err != nil {
@@ -92,15 +92,15 @@ func TestChildPassedFds(t *testing.T) {
 	}
 
 	proc := <-procs
-	if len(proc.fds) != 2+2 {
-		t.Error("Expected 4 files, got", len(proc.fds))
-	}
-
 	out, _, err := proc.notify()
 	if err != nil {
 		t.Fatal("Notify failed:", err)
 	}
 
+	if len(out) != len(in) {
+		t.Errorf("Expected %d files, got %d", len(in), len(out))
+	}
+
 	for name, inFd := range in {
 		if outFd, ok := out[name]; !ok {
 			t.Error(name, "is missing")
diff --git a/process.go b/process.go
index 516e31d..8b7dc8b 100644
--- a/process.go
+++ b/process.go
@@ -21,13 +21,12 @@ type osProcess struct {
 	finished bool
 }
 
-func newOSProcess(executable string, args []string, extraFiles []*os.File, env []string) (process, error) {
+func newOSProcess(executable string, args []string, files []*os.File, env []string) (process, error) {
 	executable, err := exec.LookPath(executable)
 	if err != nil {
 		return nil, err
 	}
 
-	files := append([]*os.File{os.Stdin, os.Stdout, os.Stderr}, extraFiles...)
 	fds := make([]uintptr, 0, len(files))
 	for _, file := range files {
 		fd, err := sysConnFd(file)
diff --git a/process_test.go b/process_test.go
index c8654b7..105f9b4 100644
--- a/process_test.go
+++ b/process_test.go
@@ -12,18 +12,28 @@ import (
 )
 
 func TestFilesAreNonblocking(t *testing.T) {
-	r, w, err := os.Pipe()
-	if err != nil {
-		t.Fatal(err)
+	pipe := func() (r, w *os.File) {
+		r, w, err := os.Pipe()
+		if err != nil {
+			t.Fatal(err)
+		}
+		t.Cleanup(func() {
+			r.Close()
+			w.Close()
+		})
+		return r, w
 	}
-	defer r.Close()
-	defer w.Close()
 
+	// Set up our own blocking stdin since CI runs tests with stdin closed.
+	rStdin, _ := pipe()
+	rStdin.Fd()
+
+	r, _ := pipe()
 	if !isNonblock(t, r) {
 		t.Fatal("Read pipe is blocking")
 	}
 
-	proc, err := newOSProcess("cat", nil, []*os.File{r}, nil)
+	proc, err := newOSProcess("cat", nil, []*os.File{rStdin, os.Stdout, os.Stderr, r}, nil)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -47,7 +57,7 @@ func TestFilesAreNonblocking(t *testing.T) {
 }
 
 func TestArgumentsArePassedCorrectly(t *testing.T) {
-	proc, err := newOSProcess("printf", []string{""}, nil, nil)
+	proc, err := newOSProcess("printf", []string{""}, []*os.File{os.Stdin, os.Stdout, os.Stderr}, nil)
 	if err != nil {
 		t.Fatal("Can't execute printf:", err)
 	}
@@ -103,7 +113,7 @@ func newTestProcess(fds []*os.File, envstr []string) (*testProcess, error) {
 		fds,
 		env{
 			newFile: func(fd uintptr, name string) *os.File {
-				return fds[fd-3]
+				return fds[fd]
 			},
 			getenv: func(key string) string {
 				return environ[key]