From caf29da4ccf0ca64b422835dfa48fa6e06f87f24 Mon Sep 17 00:00:00 2001 From: Ian Lance Taylor Date: Sun, 22 Dec 2024 21:00:58 -0800 Subject: [PATCH] os: don't store reference count in Process.state We only need a reference count in processHandle. For #70907 Fixes #71564 Change-Id: I209ded869203dea10f12b070190774fb5f1d3d71 Reviewed-on: https://go-review.googlesource.com/c/go/+/638577 Commit-Queue: Ian Lance Taylor Reviewed-by: Ian Lance Taylor LUCI-TryBot-Result: Go LUCI Reviewed-by: Michael Knyszek Auto-Submit: Ian Lance Taylor --- src/os/exec.go | 94 ++++++++++++-------------------------------------- 1 file changed, 22 insertions(+), 72 deletions(-) diff --git a/src/os/exec.go b/src/os/exec.go index a531cdab08..f3cede2996 100644 --- a/src/os/exec.go +++ b/src/os/exec.go @@ -38,31 +38,10 @@ const ( type Process struct { Pid int - // State contains the atomic process state. + // state contains the atomic process state. // - // If handle is nil, this consists only of the processStatus fields, + // This consists of the processStatus fields, // which indicate if the process is done/released. - // - // In handle is not nil, the lower bits also contain a reference - // count for the handle field. - // - // The Process itself initially holds 1 persistent reference. Any - // operation that uses the handle with a system call temporarily holds - // an additional transient reference. This prevents the handle from - // being closed prematurely, which could result in the OS allocating a - // different handle with the same value, leading to Process' methods - // operating on the wrong process. - // - // Release and Wait both drop the Process' persistent reference, but - // other concurrent references may delay actually closing the handle - // because they hold a transient reference. - // - // Regardless, we want new method calls to immediately treat the handle - // as unavailable after Release or Wait to avoid extending this delay. - // This is achieved by setting either processStatus flag when the - // Process' persistent reference is dropped. The only difference in the - // flags is the reason the handle is unavailable, which affects the - // errors returned by concurrent calls. state atomic.Uint64 // Used only when handle is nil @@ -151,7 +130,6 @@ func newHandleProcess(pid int, handle uintptr) *Process { Pid: pid, handle: ph, } - p.state.Store(1) // 1 persistent reference runtime.SetFinalizer(p, (*Process).Release) return p } @@ -170,53 +148,31 @@ func (p *Process) handleTransientAcquire() (uintptr, processStatus) { panic("handleTransientAcquire called in invalid mode") } - for { - refs := p.state.Load() - if refs&processStatusMask != 0 { - return 0, processStatus(refs & processStatusMask) - } - new := refs + 1 - if !p.state.CompareAndSwap(refs, new) { - continue - } - h, ok := p.handle.acquire() - if !ok { - panic("inconsistent reference counts") - } + state := p.state.Load() + if state&processStatusMask != 0 { + return 0, processStatus(state & processStatusMask) + } + h, ok := p.handle.acquire() + if ok { return h, statusOK } + + // This case means that the handle has been closed. + // We always set the status to non-zero before closing the handle. + // If we get here the status must have been set non-zero after + // we just checked it above. + state = p.state.Load() + if state&processStatusMask == 0 { + panic("inconsistent process status") + } + return 0, processStatus(state & processStatusMask) } func (p *Process) handleTransientRelease() { if p.handle == nil { panic("handleTransientRelease called in invalid mode") } - - for { - state := p.state.Load() - refs := state &^ processStatusMask - status := processStatus(state & processStatusMask) - if refs == 0 { - // This should never happen because - // handleTransientRelease is always paired with - // handleTransientAcquire. - panic("release of handle with refcount 0") - } - if refs == 1 && status == statusOK { - // Process holds a persistent reference and always sets - // a status when releasing that reference - // (handlePersistentRelease). Thus something has gone - // wrong if this is the last release but a status has - // not always been set. - panic("final release of handle without processStatus") - } - new := state - 1 - if !p.state.CompareAndSwap(state, new) { - continue - } - p.handle.release() - return - } + p.handle.release() } // Drop the Process' persistent reference on the handle, deactivating future @@ -230,8 +186,8 @@ func (p *Process) handlePersistentRelease(reason processStatus) processStatus { } for { - refs := p.state.Load() - status := processStatus(refs & processStatusMask) + state := p.state.Load() + status := processStatus(state & processStatusMask) if status != statusOK { // Both Release and successful Wait will drop the // Process' persistent reference on the handle. We @@ -240,13 +196,7 @@ func (p *Process) handlePersistentRelease(reason processStatus) processStatus { // reference is dropped exactly once. return status } - if refs == 0 { - // This should never happen because dropping the - // persistent reference always sets a status. - panic("release of handle with refcount 0") - } - new := (refs - 1) | uint64(reason) - if !p.state.CompareAndSwap(refs, new) { + if !p.state.CompareAndSwap(state, uint64(reason)) { continue } p.handle.release() -- 2.51.0