os: kick FIFOs with O_NONBLOCK out of the kqueue on Darwin/iOS

Fixes #66239

Change-Id: I8210682c0cf4285b950e9fabe687b7ad2369835c
Reviewed-on: https://go-review.googlesource.com/c/go/+/570397
Auto-Submit: Ian Lance Taylor <iant@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Run-TryBot: Andy Pan <panjf2000@gmail.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: David Chase <drchase@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>

5 files changed, 88 insertions, 35 deletions

diff --git a/src/os/fifo_test.go b/src/os/fifo_test.go
index e0386a2d28..3b7e5eac19 100644
--- a/src/os/fifo_test.go
+++ b/src/os/fifo_test.go
@@ -10,6 +10,7 @@ import (
 	"errors"
 	"internal/syscall/unix"
 	"internal/testenv"
+	"io"
 	"io/fs"
 	"os"
 	"path/filepath"
@@ -17,6 +18,7 @@ import (
 	"sync"
 	"syscall"
 	"testing"
+	"time"
 )
 
 func TestFifoEOF(t *testing.T) {
@@ -206,3 +208,56 @@ func TestNewFileNonBlocking(t *testing.T) {
 		t.Error("pipe blocking after Fd")
 	}
 }
+
+func TestFIFONonBlockingEOF(t *testing.T) {
+	fifoName := filepath.Join(t.TempDir(), "issue-66239-fifo")
+	if err := syscall.Mkfifo(fifoName, 0600); err != nil {
+		t.Fatalf("Error creating fifo: %v", err)
+	}
+
+	r, err := os.OpenFile(fifoName, os.O_RDONLY|syscall.O_NONBLOCK, os.ModeNamedPipe)
+	if err != nil {
+		t.Fatalf("Error opening fifo for read: %v", err)
+	}
+	defer r.Close()
+
+	w, err := os.OpenFile(fifoName, os.O_WRONLY, os.ModeNamedPipe)
+	if err != nil {
+		t.Fatalf("Error opening fifo for write: %v", err)
+	}
+	defer w.Close()
+
+	data := "Hello Gophers!"
+	if _, err := w.WriteString(data); err != nil {
+		t.Fatalf("Error writing to fifo: %v", err)
+	}
+
+	// Close the writer after a short delay to open a gap for the reader
+	// of FIFO to fall into polling. See https://go.dev/issue/66239#issuecomment-1987620476
+	time.AfterFunc(200*time.Millisecond, func() {
+		if err := w.Close(); err != nil {
+			t.Errorf("Error closing writer: %v", err)
+		}
+	})
+
+	buf := make([]byte, len(data))
+	n, err := io.ReadAtLeast(r, buf, len(data))
+	if n != len(data) || string(buf) != data || err != nil {
+		t.Errorf("ReadAtLeast: %v; got %q, want %q", err, buf, data)
+		return
+	}
+
+	// Loop reading from FIFO until EOF to ensure that the reader
+	// is not blocked infinitely, otherwise there is something wrong
+	// with the netpoller.
+	for {
+		_, err = r.Read(buf)
+		if errors.Is(err, io.EOF) {
+			break
+		}
+		if err != nil && !errors.Is(err, syscall.EAGAIN) {
+			t.Errorf("Error reading bytes from fifo: %v", err)
+			return
+		}
+	}
+}
diff --git a/src/os/file_unix.go b/src/os/file_unix.go
index 5c45014ae5..f36028bfcb 100644
--- a/src/os/file_unix.go
+++ b/src/os/file_unix.go
@@ -108,16 +108,12 @@ func NewFile(fd uintptr, name string) *File {
 		return nil
 	}
 
-	kind := kindNewFile
-	appendMode := false
-	if flags, err := unix.Fcntl(fdi, syscall.F_GETFL, 0); err == nil {
-		if unix.HasNonblockFlag(flags) {
-			kind = kindNonBlock
-		}
-		appendMode = flags&syscall.O_APPEND != 0
+	flags, err := unix.Fcntl(fdi, syscall.F_GETFL, 0)
+	if err != nil {
+		flags = 0
 	}
-	f := newFile(fdi, name, kind)
-	f.appendMode = appendMode
+	f := newFile(fdi, name, kindNewFile, unix.HasNonblockFlag(flags))
+	f.appendMode = flags&syscall.O_APPEND != 0
 	return f
 }
 
@@ -136,9 +132,7 @@ func net_newUnixFile(fd int, name string) *File {
 		panic("invalid FD")
 	}
 
-	f := newFile(fd, name, kindNonBlock)
-	f.nonblock = true // tell Fd to return blocking descriptor
-	return f
+	return newFile(fd, name, kindSock, true)
 }
 
 // newFileKind describes the kind of file to newFile.
@@ -148,13 +142,13 @@ const (
 	// kindNewFile means that the descriptor was passed to us via NewFile.
 	kindNewFile newFileKind = iota
 	// kindOpenFile means that the descriptor was opened using
-	// Open, Create, or OpenFile (without O_NONBLOCK).
+	// Open, Create, or OpenFile.
 	kindOpenFile
 	// kindPipe means that the descriptor was opened using Pipe.
 	kindPipe
-	// kindNonBlock means that the descriptor is already in
-	// non-blocking mode.
-	kindNonBlock
+	// kindSock means that the descriptor is a network file descriptor
+	// that was created from net package and was opened using net_newUnixFile.
+	kindSock
 	// kindNoPoll means that we should not put the descriptor into
 	// non-blocking mode, because we know it is not a pipe or FIFO.
 	// Used by openFdAt and openDirNolog for directories.
@@ -164,7 +158,7 @@ const (
 // newFile is like NewFile, but if called from OpenFile or Pipe
 // (as passed in the kind parameter) it tries to add the file to
 // the runtime poller.
-func newFile(fd int, name string, kind newFileKind) *File {
+func newFile(fd int, name string, kind newFileKind, nonBlocking bool) *File {
 	f := &File{&file{
 		pfd: poll.FD{
 			Sysfd:         fd,
@@ -175,11 +169,16 @@ func newFile(fd int, name string, kind newFileKind) *File {
 		stdoutOrErr: fd == 1 || fd == 2,
 	}}
 
-	pollable := kind == kindOpenFile || kind == kindPipe || kind == kindNonBlock
+	pollable := kind == kindOpenFile || kind == kindPipe || kind == kindSock || nonBlocking
 
-	// If the caller passed a non-blocking filedes (kindNonBlock),
-	// we assume they know what they are doing so we allow it to be
-	// used with kqueue.
+	// Things like regular files and FIFOs in kqueue on *BSD/Darwin
+	// may not work properly (or accurately according to its manual).
+	// As a result, we should avoid adding those to the kqueue-based
+	// netpoller. Check out #19093, #24164, and #66239 for more contexts.
+	//
+	// If the fd was passed to us via any path other than OpenFile,
+	// we assume those callers know what they were doing, so we won't
+	// perform this check and allow it to be added to the kqueue.
 	if kind == kindOpenFile {
 		switch runtime.GOOS {
 		case "darwin", "ios", "dragonfly", "freebsd", "netbsd", "openbsd":
@@ -211,10 +210,14 @@ func newFile(fd int, name string, kind newFileKind) *File {
 
 	clearNonBlock := false
 	if pollable {
-		if kind == kindNonBlock {
-			// The descriptor is already in non-blocking mode.
-			// We only set f.nonblock if we put the file into
-			// non-blocking mode.
+		// The descriptor is already in non-blocking mode.
+		// We only set f.nonblock if we put the file into
+		// non-blocking mode.
+		if nonBlocking {
+			// See the comments on net_newUnixFile.
+			if kind == kindSock {
+				f.nonblock = true // tell Fd to return blocking descriptor
+			}
 		} else if err := syscall.SetNonblock(fd, true); err == nil {
 			f.nonblock = true
 			clearNonBlock = true
@@ -290,12 +293,7 @@ func openFileNolog(name string, flag int, perm FileMode) (*File, error) {
 		syscall.CloseOnExec(r)
 	}
 
-	kind := kindOpenFile
-	if unix.HasNonblockFlag(flag) {
-		kind = kindNonBlock
-	}
-
-	f := newFile(r, name, kind)
+	f := newFile(r, name, kindOpenFile, unix.HasNonblockFlag(flag))
 	f.pfd.SysFile = s
 	return f, nil
 }
@@ -318,7 +316,7 @@ func openDirNolog(name string) (*File, error) {
 		syscall.CloseOnExec(r)
 	}
 
-	f := newFile(r, name, kindNoPoll)
+	f := newFile(r, name, kindNoPoll, false)
 	f.pfd.SysFile = s
 	return f, nil
 }
diff --git a/src/os/pipe2_unix.go b/src/os/pipe2_unix.go
index 2d293fdb4d..dca83a529b 100644
--- a/src/os/pipe2_unix.go
+++ b/src/os/pipe2_unix.go
@@ -18,5 +18,5 @@ func Pipe() (r *File, w *File, err error) {
 		return nil, nil, NewSyscallError("pipe2", e)
 	}
 
-	return newFile(p[0], "|0", kindPipe), newFile(p[1], "|1", kindPipe), nil
+	return newFile(p[0], "|0", kindPipe, false), newFile(p[1], "|1", kindPipe, false), nil
 }
diff --git a/src/os/pipe_unix.go b/src/os/pipe_unix.go
index 2eb11a04cb..5c1a953fda 100644
--- a/src/os/pipe_unix.go
+++ b/src/os/pipe_unix.go
@@ -24,5 +24,5 @@ func Pipe() (r *File, w *File, err error) {
 	syscall.CloseOnExec(p[1])
 	syscall.ForkLock.RUnlock()
 
-	return newFile(p[0], "|0", kindPipe), newFile(p[1], "|1", kindPipe), nil
+	return newFile(p[0], "|0", kindPipe, false), newFile(p[1], "|1", kindPipe, false), nil
 }
diff --git a/src/os/removeall_at.go b/src/os/removeall_at.go
index 8ea5df4117..87c4d805c3 100644
--- a/src/os/removeall_at.go
+++ b/src/os/removeall_at.go
@@ -195,5 +195,5 @@ func openFdAt(dirfd int, name string) (*File, error) {
 	}
 
 	// We use kindNoPoll because we know that this is a directory.
-	return newFile(r, name, kindNoPoll), nil
+	return newFile(r, name, kindNoPoll, false), nil
 }