]> Cypherpunks repositories - gostls13.git/commitdiff
os: kick FIFOs with O_NONBLOCK out of the kqueue on Darwin/iOS
authorAndy Pan <panjf2000@gmail.com>
Wed, 13 Mar 2024 02:15:19 +0000 (10:15 +0800)
committerGopher Robot <gobot@golang.org>
Tue, 19 Mar 2024 11:47:23 +0000 (11:47 +0000)
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>

src/os/fifo_test.go
src/os/file_unix.go
src/os/pipe2_unix.go
src/os/pipe_unix.go
src/os/removeall_at.go

index e0386a2d28cac3033c6b02301d484f72eebfabb8..3b7e5eac197c257e9b3263046dc94565209412e3 100644 (file)
@@ -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
+               }
+       }
+}
index 5c45014ae5cad27069dfd6e24b18567ef2a83d50..f36028bfcbdaea9bd22dd5eada994481cdb991db 100644 (file)
@@ -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
 }
index 2d293fdb4d9665fca9034e989e9bf127bc6f6d0a..dca83a529bb0a56894baecd727e059f73e47e795 100644 (file)
@@ -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
 }
index 2eb11a04cb2d929ac9f11769f0a18a59ab8b1cba..5c1a953fda4c659b47792a7952b5eb372b8f8ab6 100644 (file)
@@ -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
 }
index 8ea5df411740fd7dfbb5025b11b478502ca1ff71..87c4d805c3d8f8206877e062d09a9790f11d12f9 100644 (file)
@@ -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
 }