]> Cypherpunks repositories - gostls13.git/commitdiff
os: simplify Process.Release
authorIan Lance Taylor <iant@golang.org>
Mon, 23 Dec 2024 19:27:13 +0000 (11:27 -0800)
committerGopher Robot <gobot@golang.org>
Sat, 8 Feb 2025 01:10:44 +0000 (17:10 -0800)
Consolidate release/deactivation into a single doRelease method.
It needs to check GOOS for backward compatibility, but it's
simpler to keep all the logic in one place.

Change-Id: I242eb084d44d2682f862a8fbf55c410fb8c53358
Reviewed-on: https://go-review.googlesource.com/c/go/+/638580
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
src/os/exec.go
src/os/exec_plan9.go
src/os/exec_unix.go
src/os/exec_windows.go
src/os/pidfd_linux.go

index 212689b0faf0d054f696219c3ab0ff00d402bb8b..6206ae28cd3fea54f081447ebf7550cb0036b388 100644 (file)
@@ -176,40 +176,6 @@ func (p *Process) handleTransientRelease() {
        p.handle.release()
 }
 
-// Drop the Process' persistent reference on the handle, deactivating future
-// Wait/Signal calls with the passed reason.
-//
-// Returns the status prior to this call. If this is not statusOK, then the
-// reference was not dropped or status changed.
-func (p *Process) handlePersistentRelease(reason processStatus) processStatus {
-       if p.handle == nil {
-               panic("handlePersistentRelease called in invalid mode")
-       }
-
-       for {
-               state := p.state.Load()
-               status := processStatus(state)
-               if status != statusOK {
-                       // Both Release and successful Wait will drop the
-                       // Process' persistent reference on the handle. We
-                       // can't allow concurrent calls to drop the reference
-                       // twice, so we use the status as a guard to ensure the
-                       // reference is dropped exactly once.
-                       return status
-               }
-               if !p.state.CompareAndSwap(state, uint32(reason)) {
-                       continue
-               }
-
-               // No need for more cleanup.
-               p.cleanup.Stop()
-
-               p.handle.release()
-
-               return status
-       }
-}
-
 func (p *Process) pidStatus() processStatus {
        if p.handle != nil {
                panic("pidStatus called in invalid mode")
@@ -218,22 +184,6 @@ func (p *Process) pidStatus() processStatus {
        return processStatus(p.state.Load())
 }
 
-func (p *Process) pidDeactivate(reason processStatus) {
-       if p.handle != nil {
-               panic("pidDeactivate called in invalid mode")
-       }
-
-       // Both Release and successful Wait will deactivate the PID. Only one
-       // of those should win, so nothing left to do here if the compare
-       // fails.
-       //
-       // N.B. This means that results can be inconsistent. e.g., with a
-       // racing Release and Wait, Wait may successfully wait on the process,
-       // returning the wait status, while future calls error with "process
-       // released" rather than "process done".
-       p.state.CompareAndSwap(0, uint32(reason))
-}
-
 // ProcAttr holds the attributes that will be applied to a new process
 // started by StartProcess.
 type ProcAttr struct {
@@ -310,23 +260,54 @@ func StartProcess(name string, argv []string, attr *ProcAttr) (*Process, error)
 // rendering it unusable in the future.
 // Release only needs to be called if [Process.Wait] is not.
 func (p *Process) Release() error {
-       // Note to future authors: the Release API is cursed.
-       //
-       // On Unix and Plan 9, Release sets p.Pid = -1. This is the only part of the
-       // Process API that is not thread-safe, but it can't be changed now.
-       //
-       // On Windows, Release does _not_ modify p.Pid.
-       //
-       // On Windows, Wait calls Release after successfully waiting to
-       // proactively clean up resources.
-       //
-       // On Unix and Plan 9, Wait also proactively cleans up resources, but
-       // can not call Release, as Wait does not set p.Pid = -1.
-       //
-       // On Unix and Plan 9, calling Release a second time has no effect.
-       //
-       // On Windows, calling Release a second time returns EINVAL.
-       return p.release()
+       // Unfortunately, for historical reasons, on systems other
+       // than Windows, Release sets the Pid field to -1.
+       // This causes the race detector to report a problem
+       // on concurrent calls to Release, but we can't change it now.
+       if runtime.GOOS != "windows" {
+               p.Pid = -1
+       }
+
+       oldStatus := p.doRelease(statusReleased)
+
+       // For backward compatibility, on Windows only,
+       // we return EINVAL on a second call to Release.
+       if runtime.GOOS == "windows" {
+               if oldStatus == statusReleased {
+                       return syscall.EINVAL
+               }
+       }
+
+       return nil
+}
+
+// doRelease releases a [Process], setting the status to newStatus.
+// If the previous status is not statusOK, this does nothing.
+// It returns the previous status.
+func (p *Process) doRelease(newStatus processStatus) processStatus {
+       for {
+               state := p.state.Load()
+               oldStatus := processStatus(state)
+               if oldStatus != statusOK {
+                       return oldStatus
+               }
+
+               if !p.state.CompareAndSwap(state, uint32(newStatus)) {
+                       continue
+               }
+
+               // We have successfully released the Process.
+               // If it has a handle, release the reference we
+               // created in newHandleProcess.
+               if p.handle != nil {
+                       // No need for more cleanup.
+                       p.cleanup.Stop()
+
+                       p.handle.release()
+               }
+
+               return statusOK
+       }
 }
 
 // Kill causes the [Process] to exit immediately. Kill does not wait until
index 6e32a1ae17db359d6bcf8a6ada5f6b6f54dccb79..357b925b36081331089d0ea1d10e5aaad2869bc2 100644 (file)
@@ -80,7 +80,7 @@ func (p *Process) wait() (ps *ProcessState, err error) {
                return nil, NewSyscallError("wait", err)
        }
 
-       p.pidDeactivate(statusDone)
+       p.doRelease(statusDone)
        ps = &ProcessState{
                pid:    waitmsg.Pid,
                status: &waitmsg,
@@ -88,15 +88,6 @@ func (p *Process) wait() (ps *ProcessState, err error) {
        return ps, nil
 }
 
-func (p *Process) release() error {
-       p.Pid = -1
-
-       // Just mark the PID unusable.
-       p.pidDeactivate(statusReleased)
-
-       return nil
-}
-
 func findProcess(pid int) (p *Process, err error) {
        // NOOP for Plan 9.
        return newPIDProcess(pid), nil
index 3b2552eb510d0bfde4a209232f7ba7fd616b889f..f9be8dc06878ac23d53ba8c386b084aa39d0e4e6 100644 (file)
@@ -49,7 +49,7 @@ func (p *Process) pidWait() (*ProcessState, error) {
        if ready {
                // Mark the process done now, before the call to Wait4,
                // so that Process.pidSignal will not send a signal.
-               p.pidDeactivate(statusDone)
+               p.doRelease(statusDone)
                // Acquire a write lock on sigMu to wait for any
                // active call to the signal method to complete.
                p.sigMu.Lock()
@@ -66,7 +66,7 @@ func (p *Process) pidWait() (*ProcessState, error) {
        if err != nil {
                return nil, NewSyscallError("wait", err)
        }
-       p.pidDeactivate(statusDone)
+       p.doRelease(statusDone)
        return &ProcessState{
                pid:    pid1,
                status: status,
@@ -118,27 +118,6 @@ func convertESRCH(err error) error {
        return err
 }
 
-func (p *Process) release() error {
-       // We clear the Pid field only for API compatibility. On Unix, Release
-       // has always set Pid to -1. Internally, the implementation relies
-       // solely on statusReleased to determine that the Process is released.
-       p.Pid = pidReleased
-
-       if p.handle != nil {
-               // Drop the Process' reference and mark handle unusable for
-               // future calls.
-               //
-               // Ignore the return value: we don't care if this was a no-op
-               // racing with Wait, or a double Release.
-               p.handlePersistentRelease(statusReleased)
-       } else {
-               // Just mark the PID unusable.
-               p.pidDeactivate(statusReleased)
-       }
-
-       return nil
-}
-
 func findProcess(pid int) (p *Process, err error) {
        h, err := pidfdFind(pid)
        if err == ErrProcessDone {
index 0eebe76da32e40bcbf64ccf7c41d8c4a4c80d03b..68c7064d2dd0e5e4ee6e5f5ae0a418eeac36bc8b 100644 (file)
@@ -44,7 +44,11 @@ func (p *Process) wait() (ps *ProcessState, err error) {
        if e != nil {
                return nil, NewSyscallError("GetProcessTimes", e)
        }
-       defer p.Release()
+
+       // For compatibility we use statusReleased here rather
+       // than statusDone.
+       p.doRelease(statusReleased)
+
        return &ProcessState{p.Pid, syscall.WaitStatus{ExitCode: ec}, &u}, nil
 }
 
@@ -73,19 +77,6 @@ func (p *Process) signal(sig Signal) error {
        return syscall.Errno(syscall.EWINDOWS)
 }
 
-func (p *Process) release() error {
-       // Drop the Process' reference and mark handle unusable for
-       // future calls.
-       //
-       // The API on Windows expects EINVAL if Release is called multiple
-       // times.
-       if old := p.handlePersistentRelease(statusReleased); old == statusReleased {
-               return syscall.EINVAL
-       }
-
-       return nil
-}
-
 func (ph *processHandle) closeHandle() {
        syscall.CloseHandle(syscall.Handle(ph.handle))
 }
index fe4a743cf8a8e83b1ac03872e4be96a3d9f01167..7a6f4cfad0b17729d3d065955302ab88c15defdc 100644 (file)
@@ -108,9 +108,11 @@ func (p *Process) pidfdWait() (*ProcessState, error) {
        if err != nil {
                return nil, NewSyscallError("waitid", err)
        }
-       // Release the Process' handle reference, in addition to the reference
-       // we took above.
-       p.handlePersistentRelease(statusDone)
+
+       // Update the Process status to statusDone.
+       // This also releases a reference to the handle.
+       p.doRelease(statusDone)
+
        return &ProcessState{
                pid:    int(info.Pid),
                status: info.WaitStatus(),