os: make File.Readdir et al concurrency-safe

Before, all methods of File (including Close) were
safe for concurrent use (I checked), except the three
variants of ReadDir.

This change makes the ReadDir operations
atomic too, and documents explicitly that all methods
of File have this property, which was already implied
by the package documentation.

Fixes #66498

Change-Id: I05c88b4e60b44c702062e99ed8f4a32e7945927a
Reviewed-on: https://go-review.googlesource.com/c/go/+/578322
Reviewed-by: Damien Neil <dneil@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

diff --git a/src/os/dir_darwin.go b/src/os/dir_darwin.go
index e6d5bda24bdd490c2744c5cf5e4782fd72f1a709..91b67d8d61d1fc0f23a4cf0fce2db8364401efa8 100644 (file)
--- a/src/os/dir_darwin.go
+++ b/src/os/dir_darwin.go
@@ -25,16 +25,24 @@ func (d *dirInfo) close() {
 }
 
 func (f *File) readdir(n int, mode readdirMode) (names []string, dirents []DirEntry, infos []FileInfo, err error) {
-       if f.dirinfo == nil {
+       // If this file has no dirinfo, create one.
+       var d *dirInfo
+       for {
+               d = f.dirinfo.Load()
+               if d != nil {
+                       break
+               }
                dir, call, errno := f.pfd.OpenDir()
                if errno != nil {
                        return nil, nil, nil, &PathError{Op: call, Path: f.name, Err: errno}
                }
-               f.dirinfo = &dirInfo{
-                       dir: dir,
+               d = &dirInfo{dir: dir}
+               if f.dirinfo.CompareAndSwap(nil, d) {
+                       break
                }
+               // We lost the race: try again.
+               d.close()
        }
-       d := f.dirinfo
 
        size := n
        if size <= 0 {

diff --git a/src/os/dir_plan9.go b/src/os/dir_plan9.go
index 6ea5940e71df9fb58b6ce61fba086663cd33e60d..ab5c1efce5b075294c6964d64f812779b235ff4c 100644 (file)
--- a/src/os/dir_plan9.go
+++ b/src/os/dir_plan9.go
@@ -12,10 +12,14 @@ import (
 
 func (file *File) readdir(n int, mode readdirMode) (names []string, dirents []DirEntry, infos []FileInfo, err error) {
        // If this file has no dirinfo, create one.
-       if file.dirinfo == nil {
-               file.dirinfo = new(dirInfo)
+       d := file.dirinfo.Load()
+       if d == nil {
+               d = new(dirInfo)
+               file.dirinfo.Store(d)
        }
-       d := file.dirinfo
+       d.mu.Lock()
+       defer d.mu.Unlock()
+
        size := n
        if size <= 0 {
                size = 100

diff --git a/src/os/dir_unix.go b/src/os/dir_unix.go
index e14edc13dc7089ad788cf1c15f47729485c469a7..7680be7799c0e6cd3e65ae34ad83f7a8c45c73dd 100644 (file)
--- a/src/os/dir_unix.go
+++ b/src/os/dir_unix.go
@@ -17,6 +17,7 @@ import (
 
 // Auxiliary information if the File describes a directory
 type dirInfo struct {
+       mu   sync.Mutex
        buf  *[]byte // buffer for directory I/O
        nbuf int     // length of buf; return value from Getdirentries
        bufp int     // location of next record in buf.
@@ -43,13 +44,16 @@ func (d *dirInfo) close() {
 }
 
 func (f *File) readdir(n int, mode readdirMode) (names []string, dirents []DirEntry, infos []FileInfo, err error) {
-       // If this file has no dirinfo, create one.
-       if f.dirinfo == nil {
-               f.dirinfo = new(dirInfo)
+       // If this file has no dirInfo, create one.
+       d := f.dirinfo.Load()
+       if d == nil {
+               d = new(dirInfo)
+               f.dirinfo.Store(d)
        }
-       d := f.dirinfo
+       d.mu.Lock()
+       defer d.mu.Unlock()
        if d.buf == nil {
-               f.dirinfo.buf = dirBufPool.Get().(*[]byte)
+               d.buf = dirBufPool.Get().(*[]byte)
        }
 
        // Change the meaning of n for the implementation below.

diff --git a/src/os/dir_windows.go b/src/os/dir_windows.go
index 0dbc3aec3e4ee9b8645c0515d7f0da5f09a5d7da..52d5acda2afd2188fa3a52e015df845f780a2f59 100644 (file)
--- a/src/os/dir_windows.go
+++ b/src/os/dir_windows.go
@@ -16,6 +16,7 @@ import (
 
 // Auxiliary information if the File describes a directory
 type dirInfo struct {
+       mu sync.Mutex
        // buf is a slice pointer so the slice header
        // does not escape to the heap when returning
        // buf to dirBufPool.
@@ -93,14 +94,27 @@ func (d *dirInfo) init(h syscall.Handle) {
 }
 
 func (file *File) readdir(n int, mode readdirMode) (names []string, dirents []DirEntry, infos []FileInfo, err error) {
-       if file.dirinfo == nil {
-               file.dirinfo = new(dirInfo)
-               file.dirinfo.init(file.pfd.Sysfd)
+       // If this file has no dirInfo, create one.
+       var d *dirInfo
+       for {
+               d = file.dirinfo.Load()
+               if d != nil {
+                       break
+               }
+               d = new(dirInfo)
+               d.init(file.pfd.Sysfd)
+               if file.dirinfo.CompareAndSwap(nil, d) {
+                       break
+               }
+               // We lost the race: try again.
+               d.close()
        }
-       d := file.dirinfo
+       d.mu.Lock()
+       defer d.mu.Unlock()
        if d.buf == nil {
                d.buf = dirBufPool.Get().(*[]byte)
        }
+
        wantAll := n <= 0
        if wantAll {
                n = -1

diff --git a/src/os/file.go b/src/os/file.go
index a41aac9bb387831201697f407ef1f3a91b59a190..ec8ad706607865da9f084fc41bbd77eebe3e8709 100644 (file)
--- a/src/os/file.go
+++ b/src/os/file.go
@@ -34,9 +34,13 @@
 //     }
 //     fmt.Printf("read %d bytes: %q\n", count, data[:count])
 //
-// Note: The maximum number of concurrent operations on a File may be limited by
-// the OS or the system. The number should be high, but exceeding it may degrade
-// performance or cause other issues.
+// # Concurrency
+//
+// The methods of [File] correspond to file system operations. All are
+// safe for concurrent use. The maximum number of concurrent
+// operations on a File may be limited by the OS or the system. The
+// number should be high, but exceeding it may degrade performance or
+// cause other issues.
 package os
 
 import (
@@ -53,6 +57,8 @@ import (
 )
 
 // Name returns the name of the file as presented to Open.
+//
+// It is safe to call Name after [Close].
 func (f *File) Name() string { return f.name }
 
 // Stdin, Stdout, and Stderr are open Files pointing to the standard input,
@@ -279,7 +285,7 @@ func (f *File) Seek(offset int64, whence int) (ret int64, err error) {
                return 0, err
        }
        r, e := f.seek(offset, whence)
-       if e == nil && f.dirinfo != nil && r != 0 {
+       if e == nil && f.dirinfo.Load() != nil && r != 0 {
                e = syscall.EISDIR
        }
        if e != nil {

diff --git a/src/os/file_plan9.go b/src/os/file_plan9.go
index 477674b80a95c81ce7bcf7335462822eab3b55ef..fc9c89f09a1aaf805dacdad0ba241db34ddcb6ea 100644 (file)
--- a/src/os/file_plan9.go
+++ b/src/os/file_plan9.go
@@ -9,6 +9,8 @@ import (
        "internal/poll"
        "io"
        "runtime"
+       "sync"
+       "sync/atomic"
        "syscall"
        "time"
 )
@@ -26,8 +28,8 @@ type file struct {
        fdmu       poll.FDMutex
        fd         int
        name       string
-       dirinfo    *dirInfo // nil unless directory being read
-       appendMode bool     // whether file is opened for appending
+       dirinfo    atomic.Pointer[dirInfo] // nil unless directory being read
+       appendMode bool                    // whether file is opened for appending
 }
 
 // Fd returns the integer Plan 9 file descriptor referencing the open file.
@@ -60,6 +62,7 @@ func NewFile(fd uintptr, name string) *File {
 
 // Auxiliary information if the File describes a directory
 type dirInfo struct {
+       mu   sync.Mutex
        buf  [syscall.STATMAX]byte // buffer for directory I/O
        nbuf int                   // length of buf; return value from Read
        bufp int                   // location of next record in buf.
@@ -349,11 +352,9 @@ func (f *File) seek(offset int64, whence int) (ret int64, err error) {
                return 0, err
        }
        defer f.decref()
-       if f.dirinfo != nil {
-               // Free cached dirinfo, so we allocate a new one if we
-               // access this file as a directory again. See #35767 and #37161.
-               f.dirinfo = nil
-       }
+       // Free cached dirinfo, so we allocate a new one if we
+       // access this file as a directory again. See #35767 and #37161.
+       f.dirinfo.Store(nil)
        return syscall.Seek(f.fd, offset, whence)
 }
 

diff --git a/src/os/file_unix.go b/src/os/file_unix.go
index f36028bfcbdaea9bd22dd5eada994481cdb991db..8ecbffa81f7b6a5ded1645f362d967d3c01055b0 100644 (file)
--- a/src/os/file_unix.go
+++ b/src/os/file_unix.go
@@ -11,6 +11,7 @@ import (
        "internal/syscall/unix"
        "io/fs"
        "runtime"
+       "sync/atomic"
        "syscall"
        _ "unsafe" // for go:linkname
 )
@@ -58,10 +59,10 @@ func rename(oldname, newname string) error {
 type file struct {
        pfd         poll.FD
        name        string
-       dirinfo     *dirInfo // nil unless directory being read
-       nonblock    bool     // whether we set nonblocking mode
-       stdoutOrErr bool     // whether this is stdout or stderr
-       appendMode  bool     // whether file is opened for appending
+       dirinfo     atomic.Pointer[dirInfo] // nil unless directory being read
+       nonblock    bool                    // whether we set nonblocking mode
+       stdoutOrErr bool                    // whether this is stdout or stderr
+       appendMode  bool                    // whether file is opened for appending
 }
 
 // Fd returns the integer Unix file descriptor referencing the open file.
@@ -325,9 +326,8 @@ func (file *file) close() error {
        if file == nil {
                return syscall.EINVAL
        }
-       if file.dirinfo != nil {
-               file.dirinfo.close()
-               file.dirinfo = nil
+       if info := file.dirinfo.Swap(nil); info != nil {
+               info.close()
        }
        var err error
        if e := file.pfd.Close(); e != nil {
@@ -347,11 +347,10 @@ func (file *file) close() error {
 // relative to the current offset, and 2 means relative to the end.
 // It returns the new offset and an error, if any.
 func (f *File) seek(offset int64, whence int) (ret int64, err error) {
-       if f.dirinfo != nil {
+       if info := f.dirinfo.Swap(nil); info != nil {
                // Free cached dirinfo, so we allocate a new one if we
                // access this file as a directory again. See #35767 and #37161.
-               f.dirinfo.close()
-               f.dirinfo = nil
+               info.close()
        }
        ret, err = f.pfd.Seek(offset, whence)
        runtime.KeepAlive(f)

diff --git a/src/os/file_windows.go b/src/os/file_windows.go
index 6ee15eb9932508495e441ecfceddeab8cf0e5b4e..d883eb5cb21e5680ebf324a5f66e69ee33d66e5a 100644 (file)
--- a/src/os/file_windows.go
+++ b/src/os/file_windows.go
@@ -11,6 +11,7 @@ import (
        "internal/syscall/windows"
        "runtime"
        "sync"
+       "sync/atomic"
        "syscall"
        "unsafe"
 )
@@ -25,8 +26,8 @@ const _UTIME_OMIT = -1
 type file struct {
        pfd        poll.FD
        name       string
-       dirinfo    *dirInfo // nil unless directory being read
-       appendMode bool     // whether file is opened for appending
+       dirinfo    atomic.Pointer[dirInfo] // nil unless directory being read
+       appendMode bool                    // whether file is opened for appending
 }
 
 // Fd returns the Windows handle referencing the open file.
@@ -127,9 +128,8 @@ func (file *file) close() error {
        if file == nil {
                return syscall.EINVAL
        }
-       if file.dirinfo != nil {
-               file.dirinfo.close()
-               file.dirinfo = nil
+       if info := file.dirinfo.Swap(nil); info != nil {
+               info.close()
        }
        var err error
        if e := file.pfd.Close(); e != nil {
@@ -149,11 +149,10 @@ func (file *file) close() error {
 // relative to the current offset, and 2 means relative to the end.
 // It returns the new offset and an error, if any.
 func (f *File) seek(offset int64, whence int) (ret int64, err error) {
-       if f.dirinfo != nil {
+       if info := f.dirinfo.Swap(nil); info != nil {
                // Free cached dirinfo, so we allocate a new one if we
                // access this file as a directory again. See #35767 and #37161.
-               f.dirinfo.close()
-               f.dirinfo = nil
+               info.close()
        }
        ret, err = f.pfd.Seek(offset, whence)
        runtime.KeepAlive(f)

diff --git a/src/os/types.go b/src/os/types.go
index 66eb8bc8cbfc6c5dd1e31c90197ef7bab4713463..d51a458f44f9fbff30cf25377e40f46591c55a19 100644 (file)
--- a/src/os/types.go
+++ b/src/os/types.go
@@ -13,6 +13,8 @@ import (
 func Getpagesize() int { return syscall.Getpagesize() }
 
 // File represents an open file descriptor.
+//
+// The methods of File are safe for concurrent use.
 type File struct {
        *file // os specific
 }