os: deflake TestPipeEOF and TestFifoEOF

- Consolidate the two test bodies as one helper function.
- Eliminate arbitrary timeout.
- Shorten arbitrary sleeps in short mode.
- Simplify goroutines.
- Mark the tests as parallel.

Fixes #36107.
Updates #24164.

Change-Id: I14fe4395963a7256cb6d2d743d348a1ade077d5b
Reviewed-on: https://go-review.googlesource.com/c/go/+/457336
Reviewed-by: Rob Pike <[email protected]>
Run-TryBot: Bryan Mills <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Reviewed-by: Ian Lance Taylor <[email protected]>
Auto-Submit: Bryan Mills <[email protected]>

Author: Bryan C. Mills

src/os/fifo_test.go

@@ -2,102 +2,60 @@
 // Use of this source code is governed by a BSD-style
 // license that can be found in the LICENSE file.
 
-//go:build darwin || dragonfly || freebsd || linux || netbsd || openbsd
+//go:build darwin || dragonfly || freebsd || (linux && !android) || netbsd || openbsd
 
 package os_test
 
 import (
-	"bufio"
-	"bytes"
-	"fmt"
-	"io"
 	"os"
 	"path/filepath"
-	"runtime"
-	"sync"
 	"syscall"
 	"testing"
-	"time"
 )
 
-// Issue 24164.
 func TestFifoEOF(t *testing.T) {
-	switch runtime.GOOS {
-	case "android":
-		t.Skip("skipping on Android; mkfifo syscall not available")
-	}
+	t.Parallel()
 
 	dir := t.TempDir()
 	fifoName := filepath.Join(dir, "fifo")
 	if err := syscall.Mkfifo(fifoName, 0600); err != nil {
 		t.Fatal(err)
 	}
 
-	var wg sync.WaitGroup
-	wg.Add(1)
+	// Per https://pubs.opengroup.org/onlinepubs/9699919799/functions/open.html#tag_16_357_03:
+	//
+	// - “If O_NONBLOCK is clear, an open() for reading-only shall block the
+	//   calling thread until a thread opens the file for writing. An open() for
+	//   writing-only shall block the calling thread until a thread opens the file
+	//   for reading.”
+	//
+	// In order to unblock both open calls, we open the two ends of the FIFO
+	// simultaneously in separate goroutines.
+
+	rc := make(chan *os.File, 1)
 	go func() {
-		defer wg.Done()
-
-		w, err := os.OpenFile(fifoName, os.O_WRONLY, 0)
+		r, err := os.Open(fifoName)
 		if err != nil {
 			t.Error(err)
-			return
 		}
-
-		defer func() {
-			if err := w.Close(); err != nil {
-				t.Errorf("error closing writer: %v", err)
-			}
-		}()
-
-		for i := 0; i < 3; i++ {
-			time.Sleep(10 * time.Millisecond)
-			_, err := fmt.Fprintf(w, "line %d\n", i)
-			if err != nil {
-				t.Errorf("error writing to fifo: %v", err)
-				return
-			}
-		}
-		time.Sleep(10 * time.Millisecond)
+		rc <- r
 	}()
 
-	defer wg.Wait()
-
-	r, err := os.Open(fifoName)
+	w, err := os.OpenFile(fifoName, os.O_WRONLY, 0)
 	if err != nil {
-		t.Fatal(err)
+		t.Error(err)
 	}
 
-	done := make(chan bool)
-	go func() {
-		defer close(done)
-
-		defer func() {
-			if err := r.Close(); err != nil {
-				t.Errorf("error closing reader: %v", err)
-			}
-		}()
-
-		rbuf := bufio.NewReader(r)
-		for {
-			b, err := rbuf.ReadBytes('\n')
-			if err == io.EOF {
-				break
-			}
-			if err != nil {
-				t.Error(err)
-				return
-			}
-			t.Logf("%s\n", bytes.TrimSpace(b))
+	r := <-rc
+	if t.Failed() {
+		if r != nil {
+			r.Close()
 		}
-	}()
-
-	select {
-	case <-done:
-		// Test succeeded.
-	case <-time.After(time.Second):
-		t.Error("timed out waiting for read")
-		// Close the reader to force the read to complete.
-		r.Close()
+		if w != nil {
+			w.Close()
+		}
+		return
 	}
+
+	testPipeEOF(t, r, w)
 }

src/os/pipe_test.go

@@ -339,68 +339,72 @@ func testCloseWithBlockingRead(t *testing.T, r, w *os.File) {
 	wg.Wait()
 }
 
-// Issue 24164, for pipes.
 func TestPipeEOF(t *testing.T) {
+	t.Parallel()
+
 	r, w, err := os.Pipe()
 	if err != nil {
 		t.Fatal(err)
 	}
 
-	var wg sync.WaitGroup
-	wg.Add(1)
-	go func() {
-		defer wg.Done()
-
-		defer func() {
-			if err := w.Close(); err != nil {
-				t.Errorf("error closing writer: %v", err)
-			}
-		}()
+	testPipeEOF(t, r, w)
+}
 
-		for i := 0; i < 3; i++ {
-			time.Sleep(10 * time.Millisecond)
-			_, err := fmt.Fprintf(w, "line %d\n", i)
-			if err != nil {
-				t.Errorf("error writing to fifo: %v", err)
-				return
-			}
+// testPipeEOF tests that when the write side of a pipe or FIFO is closed,
+// a blocked Read call on the reader side returns io.EOF.
+//
+// This scenario previously failed to unblock the Read call on darwin.
+// (See https://go.dev/issue/24164.)
+func testPipeEOF(t *testing.T, r io.ReadCloser, w io.WriteCloser) {
+	// parkDelay is an arbitrary delay we wait for a pipe-reader goroutine to park
+	// before issuing the corresponding write. The test should pass no matter what
+	// delay we use, but with a longer delay is has a higher chance of detecting
+	// poller bugs.
+	parkDelay := 10 * time.Millisecond
+	if testing.Short() {
+		parkDelay = 100 * time.Microsecond
+	}
+	writerDone := make(chan struct{})
+	defer func() {
+		if err := r.Close(); err != nil {
+			t.Errorf("error closing reader: %v", err)
 		}
-		time.Sleep(10 * time.Millisecond)
+		<-writerDone
 	}()
 
-	defer wg.Wait()
-
-	done := make(chan bool)
+	write := make(chan int, 1)
 	go func() {
-		defer close(done)
-
-		defer func() {
-			if err := r.Close(); err != nil {
-				t.Errorf("error closing reader: %v", err)
-			}
-		}()
+		defer close(writerDone)
 
-		rbuf := bufio.NewReader(r)
-		for {
-			b, err := rbuf.ReadBytes('\n')
-			if err == io.EOF {
-				break
-			}
+		for i := range write {
+			time.Sleep(parkDelay)
+			_, err := fmt.Fprintf(w, "line %d\n", i)
 			if err != nil {
-				t.Error(err)
+				t.Errorf("error writing to fifo: %v", err)
 				return
 			}
-			t.Logf("%s\n", bytes.TrimSpace(b))
+		}
+
+		time.Sleep(parkDelay)
+		if err := w.Close(); err != nil {
+			t.Errorf("error closing writer: %v", err)
 		}
 	}()
 
-	select {
-	case <-done:
-		// Test succeeded.
-	case <-time.After(time.Second):
-		t.Error("timed out waiting for read")
-		// Close the reader to force the read to complete.
-		r.Close()
+	rbuf := bufio.NewReader(r)
+	for i := 0; i < 3; i++ {
+		write <- i
+		b, err := rbuf.ReadBytes('\n')
+		if err != nil {
+			t.Fatal(err)
+		}
+		t.Logf("%s\n", bytes.TrimSpace(b))
+	}
+
+	close(write)
+	b, err := rbuf.ReadBytes('\n')
+	if err != io.EOF || len(b) != 0 {
+		t.Errorf(`ReadBytes: %q, %v; want "", io.EOF`, b, err)
 	}
 }