os: simplify Process.Release

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 <[email protected]>
Auto-Submit: Ian Lance Taylor <[email protected]>
Reviewed-by: Michael Knyszek <[email protected]>
Reviewed-by: Cherry Mui <[email protected]>

Author: ianlancetaylor

src/os/exec.go

@@ -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

src/os/exec_plan9.go

@@ -80,23 +80,14 @@ 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,
 	}
 	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

src/os/exec_unix.go

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

src/os/exec_windows.go

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

src/os/pidfd_linux.go

@@ -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(),