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 e6d5bda..91b67d8 100644
--- a/src/os/dir_darwin.go
+++ b/src/os/dir_darwin.go
@@ -25,16 +25,24 @@
}
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 6ea5940..ab5c1ef 100644
--- a/src/os/dir_plan9.go
+++ b/src/os/dir_plan9.go
@@ -12,10 +12,14 @@
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 e14edc1..7680be7 100644
--- a/src/os/dir_unix.go
+++ b/src/os/dir_unix.go
@@ -17,6 +17,7 @@
// 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 (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 0dbc3ae..52d5acd 100644
--- a/src/os/dir_windows.go
+++ b/src/os/dir_windows.go
@@ -16,6 +16,7 @@
// 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 (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 a41aac9..ec8ad70 100644
--- 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 @@
)
// 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 @@
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 477674b..fc9c89f 100644
--- a/src/os/file_plan9.go
+++ b/src/os/file_plan9.go
@@ -9,6 +9,8 @@
"internal/poll"
"io"
"runtime"
+ "sync"
+ "sync/atomic"
"syscall"
"time"
)
@@ -26,8 +28,8 @@
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 @@
// 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 @@
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 f36028b..8ecbffa 100644
--- a/src/os/file_unix.go
+++ b/src/os/file_unix.go
@@ -11,6 +11,7 @@
"internal/syscall/unix"
"io/fs"
"runtime"
+ "sync/atomic"
"syscall"
_ "unsafe" // for go:linkname
)
@@ -58,10 +59,10 @@
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 @@
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 @@
// 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 6ee15eb..d883eb5 100644
--- a/src/os/file_windows.go
+++ b/src/os/file_windows.go
@@ -11,6 +11,7 @@
"internal/syscall/windows"
"runtime"
"sync"
+ "sync/atomic"
"syscall"
"unsafe"
)
@@ -25,8 +26,8 @@
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 @@
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 @@
// 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 66eb8bc..d51a458 100644
--- a/src/os/types.go
+++ b/src/os/types.go
@@ -13,6 +13,8 @@
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
}
