From 8a8f506516e1210c9ca3a352d76bd1d570c407fd Mon Sep 17 00:00:00 2001 From: qmuntal Date: Thu, 10 Apr 2025 17:45:47 +0200 Subject: [PATCH] os,internal/poll: disassociate handle from IOCP in File.Fd 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 Reviewed-by: Alex Brainman Reviewed-by: Michael Pratt LUCI-TryBot-Result: Go LUCI --- src/internal/poll/fd_plan9.go | 4 - src/internal/poll/fd_poll_runtime.go | 1 - src/internal/poll/fd_unix.go | 4 - src/internal/poll/fd_windows.go | 129 ++++++++++-------- .../syscall/windows/syscall_windows.go | 2 + src/internal/syscall/windows/types_windows.go | 8 ++ src/os/file.go | 8 +- src/os/file_windows.go | 5 + src/os/os_windows_test.go | 62 +++++++-- 9 files changed, 140 insertions(+), 83 deletions(-) diff --git a/src/internal/poll/fd_plan9.go b/src/internal/poll/fd_plan9.go index 6db1a9ebb1..b65485200a 100644 --- a/src/internal/poll/fd_plan9.go +++ b/src/internal/poll/fd_plan9.go @@ -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. diff --git a/src/internal/poll/fd_poll_runtime.go b/src/internal/poll/fd_poll_runtime.go index 2dd95a8bba..2aef11243a 100644 --- a/src/internal/poll/fd_poll_runtime.go +++ b/src/internal/poll/fd_poll_runtime.go @@ -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 } diff --git a/src/internal/poll/fd_unix.go b/src/internal/poll/fd_unix.go index 0888632d80..31e6e21120 100644 --- a/src/internal/poll/fd_unix.go +++ b/src/internal/poll/fd_unix.go @@ -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"), diff --git a/src/internal/poll/fd_windows.go b/src/internal/poll/fd_windows.go index 469d078fa3..e846c2cd52 100644 --- a/src/internal/poll/fd_windows.go +++ b/src/internal/poll/fd_windows.go @@ -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) } diff --git a/src/internal/syscall/windows/syscall_windows.go b/src/internal/syscall/windows/syscall_windows.go index b6859a5432..b6692166cc 100644 --- a/src/internal/syscall/windows/syscall_windows.go +++ b/src/internal/syscall/windows/syscall_windows.go @@ -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 diff --git a/src/internal/syscall/windows/types_windows.go b/src/internal/syscall/windows/types_windows.go index 6c81754e1a..9f8f61f6d9 100644 --- a/src/internal/syscall/windows/types_windows.go +++ b/src/internal/syscall/windows/types_windows.go @@ -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 +} diff --git a/src/os/file.go b/src/os/file.go index c6e9167d2c..7a7f2c06be 100644 --- a/src/os/file.go +++ b/src/os/file.go @@ -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 { diff --git a/src/os/file_windows.go b/src/os/file_windows.go index 7b1db188b5..c97307371c 100644 --- a/src/os/file_windows.go +++ b/src/os/file_windows.go @@ -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) } diff --git a/src/os/os_windows_test.go b/src/os/os_windows_test.go index d78080ccc4..89a61f0229 100644 --- a/src/os/os_windows_test.go +++ b/src/os/os_windows_test.go @@ -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 }{