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 <[email protected]>
Reviewed-by: Alex Brainman <[email protected]>
Reviewed-by: Michael Pratt <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>

Author: qmuntal

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.

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
 	}

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"),

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,27 +399,55 @@ 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
 }
 
 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)
 	}

src/internal/syscall/windows/syscall_windows.go

@@ -32,13 +32,15 @@ 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
 	ERROR_NOT_SUPPORTED          syscall.Errno = 50
 	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

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
+}

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 {

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)
 }
 

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 }{