]> Cypherpunks repositories - gostls13.git/commitdiff
os,internal/poll: disassociate handle from IOCP in File.Fd
authorqmuntal <quimmuntal@gmail.com>
Thu, 10 Apr 2025 15:45:47 +0000 (17:45 +0200)
committerQuim Muntal <quimmuntal@gmail.com>
Thu, 24 Apr 2025 13:51:09 +0000 (06:51 -0700)
Go 1.25 will gain support for overlapped IO on handles passed to
os.NewFile thanks to CL 662236. It was previously not possible to add
an overlapped handle to the Go runtime's IO completion port (IOCP),
and now happens on the first call the an IO method.

This means that there is code that relies on the fact that File.Fd
returns a handle that can always be associated with a custom IOCP.
That wouldn't be the case anymore, as a handle can only be associated
with one IOCP at a time and it must be explicitly disassociated.

To fix this breaking change, File.Fd will disassociate the handle
from the Go runtime IOCP before returning it. It is then not necessary
to defer the association until the first IO method is called, which
was recently added in CL 661955 to support this same use case, but
in a more complex and unreliable way.

Updates #19098.

Cq-Include-Trybots: luci.golang.try:gotip-windows-amd64-race,gotip-windows-amd64-longtest,gotip-windows-arm64
Change-Id: Id8a7e04d35057047c61d1733bad5bf45494b2c28
Reviewed-on: https://go-review.googlesource.com/c/go/+/664455
Reviewed-by: Damien Neil <dneil@google.com>
Reviewed-by: Alex Brainman <alex.brainman@gmail.com>
Reviewed-by: Michael Pratt <mpratt@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

src/internal/poll/fd_plan9.go
src/internal/poll/fd_poll_runtime.go
src/internal/poll/fd_unix.go
src/internal/poll/fd_windows.go
src/internal/syscall/windows/syscall_windows.go
src/internal/syscall/windows/types_windows.go
src/os/file.go
src/os/file_windows.go
src/os/os_windows_test.go

index 6db1a9ebb14dc45fd633a18ad739f3106ebb9570..b65485200add69bf9d0ba5bb0ed891d6050e5a7f 100644 (file)
@@ -36,10 +36,6 @@ type FD struct {
        isFile bool
 }
 
-func (fd *FD) initIO() error {
-       return nil
-}
-
 // We need this to close out a file descriptor when it is unlocked,
 // but the real implementation has to live in the net package because
 // it uses os.File's.
index 2dd95a8bbaf6024b6620c76137722037ed8e1832..2aef11243ad34e386e78ff28d5ee801105e6ca0b 100644 (file)
@@ -156,7 +156,6 @@ func setDeadlineImpl(fd *FD, t time.Time, mode int) error {
        }
        defer fd.decref()
 
-       fd.initIO()
        if fd.pd.runtimeCtx == 0 {
                return ErrNoDeadline
        }
index 0888632d809ef5167a9e1f1ee3df2e99615ab669..31e6e21120fde3e8fb7ce8c269ebf342312c758b 100644 (file)
@@ -47,10 +47,6 @@ type FD struct {
        isFile bool
 }
 
-func (fd *FD) initIO() error {
-       return nil
-}
-
 // Init initializes the FD. The Sysfd field should already be set.
 // This can be called multiple times on a single FD.
 // The net argument is a network name from the net package (e.g., "tcp"),
index 469d078fa39aee784324a3e98cc4516e45998cfc..e846c2cd52509c238c04fe151f74ddcb16408da9 100644 (file)
@@ -10,6 +10,7 @@ import (
        "internal/syscall/windows"
        "io"
        "sync"
+       "sync/atomic"
        "syscall"
        "unicode/utf16"
        "unicode/utf8"
@@ -98,6 +99,12 @@ func (o *operation) setEvent() {
        o.o.HEvent = h | 1
 }
 
+func (o *operation) close() {
+       if o.o.HEvent != 0 {
+               syscall.CloseHandle(o.o.HEvent)
+       }
+}
+
 func (o *operation) overlapped() *syscall.Overlapped {
        if o.fd.isBlocking {
                // Don't return the overlapped object if the file handle
@@ -169,7 +176,7 @@ func waitIO(o *operation) error {
                panic("can't wait on blocking operations")
        }
        fd := o.fd
-       if !fd.pd.pollable() {
+       if !fd.pollable() {
                // The overlapped handle is not added to the runtime poller,
                // the only way to wait for the IO to complete is block until
                // the overlapped event is signaled.
@@ -190,7 +197,7 @@ func waitIO(o *operation) error {
 // cancelIO cancels the IO operation o and waits for it to complete.
 func cancelIO(o *operation) {
        fd := o.fd
-       if !fd.pd.pollable() {
+       if !fd.pollable() {
                return
        }
        // Cancel our request.
@@ -209,14 +216,13 @@ func cancelIO(o *operation) {
 // to avoid reusing the values from a previous call.
 func execIO(o *operation, submit func(o *operation) error) (int, error) {
        fd := o.fd
-       fd.initIO()
        // Notify runtime netpoll about starting IO.
        err := fd.pd.prepare(int(o.mode), fd.isFile)
        if err != nil {
                return 0, err
        }
        // Start IO.
-       if !fd.isBlocking && o.o.HEvent == 0 && !fd.pd.pollable() {
+       if !fd.isBlocking && o.o.HEvent == 0 && !fd.pollable() {
                // If the handle is opened for overlapped IO but we can't
                // use the runtime poller, then we need to use an
                // event to wait for the IO to complete.
@@ -244,10 +250,11 @@ func execIO(o *operation, submit func(o *operation) error) (int, error) {
                        err = windows.WSAGetOverlappedResult(fd.Sysfd, &o.o, &o.qty, false, &o.flags)
                }
        }
-       // ERROR_OPERATION_ABORTED may have been caused by us. In that case,
-       // map it to our own error. Don't do more than that, each submitted
-       // function may have its own meaning for each error.
-       if err == syscall.ERROR_OPERATION_ABORTED {
+       switch err {
+       case syscall.ERROR_OPERATION_ABORTED:
+               // ERROR_OPERATION_ABORTED may have been caused by us. In that case,
+               // map it to our own error. Don't do more than that, each submitted
+               // function may have its own meaning for each error.
                if waitErr != nil {
                        // IO canceled by the poller while waiting for completion.
                        err = waitErr
@@ -257,6 +264,12 @@ func execIO(o *operation, submit func(o *operation) error) (int, error) {
                        // we assume it is interrupted by Close.
                        err = errClosing(fd.isFile)
                }
+       case windows.ERROR_IO_INCOMPLETE:
+               // waitIO couldn't wait for the IO to complete.
+               if waitErr != nil {
+                       // The wait error will be more informative.
+                       err = waitErr
+               }
        }
        return int(o.qty), err
 }
@@ -314,9 +327,7 @@ type FD struct {
        // Whether FILE_FLAG_OVERLAPPED was not set when opening the file.
        isBlocking bool
 
-       // Initialization parameters.
-       initIOOnce sync.Once
-       initIOErr  error // only used in the net package
+       disassociated atomic.Bool
 }
 
 // setOffset sets the offset fields of the overlapped object
@@ -343,6 +354,12 @@ func (fd *FD) addOffset(off int) {
        fd.setOffset(fd.offset + int64(off))
 }
 
+// pollable should be used instead of fd.pd.pollable(),
+// as it is aware of the disassociated state.
+func (fd *FD) pollable() bool {
+       return fd.pd.pollable() && !fd.disassociated.Load()
+}
+
 // fileKind describes the kind of file.
 type fileKind byte
 
@@ -353,35 +370,6 @@ const (
        kindPipe
 )
 
-func (fd *FD) initIO() error {
-       if fd.isBlocking {
-               return nil
-       }
-       fd.initIOOnce.Do(func() {
-               if fd.closing() {
-                       // Closing, nothing to do.
-                       return
-               }
-               // The runtime poller will ignore I/O completion
-               // notifications not initiated by this package,
-               // so it is safe to add handles owned by the caller.
-               fd.initIOErr = fd.pd.init(fd)
-               if fd.initIOErr != nil {
-                       return
-               }
-               fd.rop.runtimeCtx = fd.pd.runtimeCtx
-               fd.wop.runtimeCtx = fd.pd.runtimeCtx
-               if fd.kind != kindNet || socketCanUseSetFileCompletionNotificationModes {
-                       // Non-socket handles can use SetFileCompletionNotificationModes without problems.
-                       err := syscall.SetFileCompletionNotificationModes(fd.Sysfd,
-                               syscall.FILE_SKIP_SET_EVENT_ON_HANDLE|syscall.FILE_SKIP_COMPLETION_PORT_ON_SUCCESS,
-                       )
-                       fd.skipSyncNotif = err == nil
-               }
-       })
-       return fd.initIOErr
-}
-
 // Init initializes the FD. The Sysfd field should already be set.
 // This can be called multiple times on a single FD.
 // The net argument is a network name from the net package (e.g., "tcp"),
@@ -411,20 +399,46 @@ func (fd *FD) Init(net string, pollable bool) error {
        fd.rop.fd = fd
        fd.wop.fd = fd
 
-       // A file handle (and its duplicated handles) can only be associated
-       // with one IOCP. A new association will fail if the handle is already
-       // associated. Defer the association until the first I/O operation so that
-       // overlapped handles passed in os.NewFile have a chance to be used
-       // with an external IOCP. This is common case, for example, when calling
-       // os.NewFile on a handle just to pass it to a exec.Command standard
-       // input/output/error. If the association fails, the I/O operations
-       // will be performed synchronously.
-       if fd.kind == kindNet {
-               // The net package is the only consumer that requires overlapped
-               // handles and that cares about handle IOCP association errors.
-               // We can should do the IOCP association here.
-               return fd.initIO()
+       // It is safe to add overlapped handles that also perform I/O
+       // outside of the runtime poller. The runtime poller will ignore
+       // I/O completion notifications not initiated by us.
+       err := fd.pd.init(fd)
+       if err != nil {
+               return err
+       }
+       fd.rop.runtimeCtx = fd.pd.runtimeCtx
+       fd.wop.runtimeCtx = fd.pd.runtimeCtx
+       if fd.kind != kindNet || socketCanUseSetFileCompletionNotificationModes {
+               // Non-socket handles can use SetFileCompletionNotificationModes without problems.
+               err := syscall.SetFileCompletionNotificationModes(fd.Sysfd,
+                       syscall.FILE_SKIP_SET_EVENT_ON_HANDLE|syscall.FILE_SKIP_COMPLETION_PORT_ON_SUCCESS,
+               )
+               fd.skipSyncNotif = err == nil
+       }
+       return nil
+}
+
+// DisassociateIOCP disassociates the file handle from the IOCP.
+// The disassociate operation will not succeed if there is any
+// in-progress IO operation on the file handle.
+func (fd *FD) DisassociateIOCP() error {
+       if err := fd.incref(); err != nil {
+               return err
+       }
+       defer fd.decref()
+
+       if fd.isBlocking || !fd.pollable() {
+               // Nothing to disassociate.
+               return nil
        }
+
+       info := windows.FILE_COMPLETION_INFORMATION{}
+       if err := windows.NtSetInformationFile(fd.Sysfd, &windows.IO_STATUS_BLOCK{}, unsafe.Pointer(&info), uint32(unsafe.Sizeof(info)), windows.FileReplaceCompletionInformation); err != nil {
+               return err
+       }
+       fd.disassociated.Store(true)
+       // Don't call fd.pd.close(), it would be too racy.
+       // There is no harm on leaving fd.pd open until Close is called.
        return nil
 }
 
@@ -432,6 +446,8 @@ func (fd *FD) destroy() error {
        if fd.Sysfd == syscall.InvalidHandle {
                return syscall.EINVAL
        }
+       fd.rop.close()
+       fd.wop.close()
        // Poller may want to unregister fd in readiness notification mechanism,
        // so this must be executed before fd.CloseFunc.
        fd.pd.close()
@@ -454,12 +470,7 @@ func (fd *FD) Close() error {
        if !fd.fdmu.increfAndClose() {
                return errClosing(fd.isFile)
        }
-       // There is a potential race between a concurrent call to fd.initIO,
-       // which calls fd.pd.init, and the call to fd.pd.evict below.
-       // This is solved by calling fd.initIO ourselves, which will
-       // block until the concurrent fd.initIO has completed. Note
-       // that fd.initIO is no-op if first called from here.
-       fd.initIO()
+
        if fd.kind == kindPipe {
                syscall.CancelIoEx(fd.Sysfd, nil)
        }
index b6859a54322d2f60df54e8da73b380b8f2b1761e..b6692166cc460fbae723b159abe96b0e4932b440 100644 (file)
@@ -32,6 +32,7 @@ func UTF16PtrToString(p *uint16) string {
 }
 
 const (
+       ERROR_INVALID_HANDLE         syscall.Errno = 6
        ERROR_BAD_LENGTH             syscall.Errno = 24
        ERROR_SHARING_VIOLATION      syscall.Errno = 32
        ERROR_LOCK_VIOLATION         syscall.Errno = 33
@@ -39,6 +40,7 @@ const (
        ERROR_CALL_NOT_IMPLEMENTED   syscall.Errno = 120
        ERROR_INVALID_NAME           syscall.Errno = 123
        ERROR_LOCK_FAILED            syscall.Errno = 167
+       ERROR_IO_INCOMPLETE          syscall.Errno = 996
        ERROR_NO_TOKEN               syscall.Errno = 1008
        ERROR_NO_UNICODE_TRANSLATION syscall.Errno = 1113
        ERROR_CANT_ACCESS_FILE       syscall.Errno = 1920
index 6c81754e1a9a887b5a3a65c81ddf90b7d134aada..9f8f61f6d918b5687ea1b544db78e9f7b78d1fd6 100644 (file)
@@ -248,3 +248,11 @@ type FILE_LINK_INFORMATION struct {
        FileNameLength  uint32
        FileName        [syscall.MAX_PATH]uint16
 }
+
+const FileReplaceCompletionInformation = 61
+
+// https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/ntifs/ns-ntifs-_file_completion_information
+type FILE_COMPLETION_INFORMATION struct {
+       Port syscall.Handle
+       Key  uintptr
+}
index c6e9167d2c99fa4f311673b809dbea2c1b3b6bbc..7a7f2c06bec4e3f25b66892ea9453b6ae47b44cb 100644 (file)
@@ -714,8 +714,12 @@ func (f *File) SyscallConn() (syscall.RawConn, error) {
 // Do not close the returned descriptor; that could cause a later
 // close of f to close an unrelated descriptor.
 //
-// On Unix systems this will cause the [File.SetDeadline]
-// methods to stop working.
+// Fd's behavior differs on some platforms:
+//
+//   - On Unix and Windows, [File.SetDeadline] methods will stop working.
+//   - On Windows, the file descriptor will be disassociated from the
+//     Go runtime I/O completion port if there are no concurrent I/O
+//     operations on the file.
 //
 // For most uses prefer the f.SyscallConn method.
 func (f *File) Fd() uintptr {
index 7b1db188b55ecfa07893781fdded41ac8c6240de..c97307371c88ad1c59d3cb32c26d339bffd9fa6a 100644 (file)
@@ -37,6 +37,11 @@ func (file *File) fd() uintptr {
        if file == nil {
                return uintptr(syscall.InvalidHandle)
        }
+       // Try to disassociate the file from the runtime poller.
+       // File.Fd doesn't return an error, so we don't have a way to
+       // report it. We just ignore it. It's up to the caller to call
+       // it when there are no concurrent IO operations.
+       _ = file.pfd.DisassociateIOCP()
        return uintptr(file.pfd.Sysfd)
 }
 
index d78080ccc4198d46e89a12b3d386af10c664b038..89a61f02295aa4725db591d2b88296c84cb7e24f 100644 (file)
@@ -1985,19 +1985,7 @@ func TestPipeCanceled(t *testing.T) {
 }
 
 func iocpAssociateFile(f *os.File, iocp syscall.Handle) error {
-       sc, err := f.SyscallConn()
-       if err != nil {
-               return err
-       }
-       var syserr error
-       err = sc.Control(func(fd uintptr) {
-               if _, err = windows.CreateIoCompletionPort(syscall.Handle(fd), iocp, 0, 0); err != nil {
-                       syserr = err
-               }
-       })
-       if err == nil {
-               err = syserr
-       }
+       _, err := windows.CreateIoCompletionPort(syscall.Handle(f.Fd()), iocp, 0, 0)
        return err
 }
 
@@ -2075,6 +2063,54 @@ func TestFileAssociatedWithExternalIOCP(t *testing.T) {
        }
 }
 
+func TestFileWriteFdRace(t *testing.T) {
+       t.Parallel()
+
+       f := newFileOverlapped(t, filepath.Join(t.TempDir(), "a"), true)
+
+       var wg sync.WaitGroup
+       wg.Add(2)
+
+       go func() {
+               defer wg.Done()
+               n, err := f.Write([]byte("hi"))
+               if err != nil {
+                       // We look at error strings as the
+                       // expected errors are OS-specific.
+                       switch {
+                       case errors.Is(err, windows.ERROR_INVALID_HANDLE):
+                               // Ignore an expected error.
+                       default:
+                               // Unexpected error.
+                               t.Error(err)
+                       }
+                       return
+               }
+               if n != 2 {
+                       t.Errorf("wrote %d bytes, expected 2", n)
+                       return
+               }
+       }()
+       go func() {
+               defer wg.Done()
+               f.Fd()
+       }()
+       wg.Wait()
+
+       iocp, err := windows.CreateIoCompletionPort(syscall.InvalidHandle, 0, 0, 0)
+       if err != nil {
+               t.Fatal(err)
+       }
+       defer syscall.CloseHandle(iocp)
+       if err := iocpAssociateFile(f, iocp); err != nil {
+               t.Fatal(err)
+       }
+
+       if _, err := f.Write([]byte("hi")); err != nil {
+               t.Error(err)
+       }
+}
+
 func TestSplitPath(t *testing.T) {
        t.Parallel()
        for _, tt := range []struct{ path, wantDir, wantBase string }{