refactor: consolidate Request lifecycle flags into an atomic int32 state#207
Merged
Conversation
Baseline benchmark (no behavior change) covering NewRequest -> UseRequest -> startServe -> recycle, which the existing create/recycle and high-water benchmarks do not exercise. Landed before the lifecycle-state consolidation so benchstat has before/after samples for the claim/start path.
Replace the three separate lifecycle flags on Request (registered bool,
running atomic.Bool, claimed atomic.Bool) with a single reqState int32 held
in Request.state, making the transitions explicit and race-safe:
reqUnclaimable -> (terminal; created after Jaws.Close)
reqPending -> reqClaimed -> reqRunning -> reqFinished
reqPending/reqClaimed -> reqFinished (never served)
Claiming and starting to serve are now single checked CAS transitions
(casState(reqPending, reqClaimed) and casState(reqClaimed, reqRunning)),
and finishLocked centralizes the finish transition: it captures whether the
Request had been claimed before detaching the session (so Session.delRequest
still grants the claimed-WebSocket grace window) and then stores reqFinished.
Session.delRequest takes wasClaimed explicitly instead of reading a flag, so
the grace decision no longer depends on the order of the finish transition
and the session detach.
TestServe now performs the same checked startServe transition synchronously
before creating its per-run channels, and panics (releasing the broadcast
subscription) if the Request is not servable. This tightens its contract: the
Request must already be claimed via UseRequest. A concurrent Jaws.Close or
retirement can no longer resurrect a Request into reqRunning.
Consolidating three fields into one shrinks each Request by 16 bytes with no
change in allocation count. benchstat (Apple M5 Max, -count=10, three
lifecycle benchmarks), old = separate flags, new = int32 state:
B/op geomean -3.65% (e.g. 401 -> 385, 416 -> 400)
sec/op geomean -2.51%
allocs/op geomean 0.00% (unchanged)
…se-race test
- finishLocked doc: state that both rq.mu and jw.mu are required, and why. claim
runs its reqPending->reqClaimed CAS under rq.mu while startServe runs its
reqClaimed->reqRunning CAS under jw.mu (not rq.mu), so both locks are needed to
keep finishLocked's read-then-store atomic against the other transitions. The old
wording ("rq.mu prevents state changes"; "jw.mu only for map bookkeeping") was
wrong.
- releaseBuffersLocked doc: it performs only buffer mechanics. It no longer cancels
the context, detaches the session, or clears a lifecycle flag — the caller cancels
the context and finishLocked detaches the session and stores reqFinished.
- TestServe_CloseRaceIsSafe: capture the outcome instead of swallowing every panic.
Accept only "jaws: TestServe" setup-failure panics (subscription timeout or the
refused startServe transition), fail on any other panic (resurrection, nil deref,
double-finish assertion) or a serve+panic combination, and assert the Request ends
reqFinished. A measured race split (both branches occur) confirms the assertions
have teeth; the double-finish assertion under -tags debug catches a terminal->
running resurrection regression.
- TestFinishLockedTerminalStatePanicsInDebug: hold jw.mu as well as rq.mu, matching
the documented finishLocked contract and production lock order.
The strengthened Close-race test passed onPanic func(any){}, discarding any panic
recovered from TestServe's process/recycle goroutine; only the synchronous setup
panic was asserted. A panic escaping recycle — notably finishLocked's terminal-state
assertion after a terminal->running resurrection — was therefore swallowed while the
final state still read reqFinished, so the test could pass through the very bug it
guards against.
Capture the onPanic argument (published before doneCh closes, so the post-<-doneCh
read is race-free) and fail if it is non-nil. Verified by fault injection: forcing an
inconsistent terminal state before recycle makes finishLocked's assertion escape and
is now caught (per-element handler panics remain recovered inside process and are out
of scope).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #206 (stable Request identities). Replaces the three separate lifecycle flags on
Request—registered bool,running atomic.Bool,claimed atomic.Bool— with a singlereqState int32held inRequest.state, so the lifecycle transitions are explicit and race-safe.casState(reqPending, reqClaimed)andcasState(reqClaimed, reqRunning). A claim/serve can only succeed from the exact expected state, so a concurrentClose/retirement or a duplicate claim can never resurrect a Request.finishLockedcentralizes the finish transition. It captures whether the Request had been claimed before detaching the session — soSession.delRequeststill grants the claimed-WebSocket grace window — then storesreqFinished.Session.delRequestnow takeswasClaimedexplicitly rather than reading a flag, so the grace decision no longer depends on the order of the finish transition vs. the session detach.maintenance,Close,RequestCounts) useloadState(); identity-gated paths (destKey,wantMessage,appendDirtyTags) useloadState().registered()underrq.mu, exactly as they readregisteredbefore.TestServe contract change (intentional, not a pure refactor)
TestServenow performs the same checkedstartServetransition synchronously, before creating its per-run channels, and panics (after releasing the broadcast subscription) if the Request is not servable — instead of the old fire-and-forgetrunning.Store(true)inside the serve goroutine. This tightens the contract: the Request must already be claimed viaUseRequest, and it may be served only once and only beforeClose.This is a deliberate test-API tightening, not behavior-preserving. An audit of all callers (in-tree and
github.com/linkdata/jaws/jawstest) confirmed none rely on the old loose behavior; the harness always claims viaUseRequestfirst. The upside: a concurrentClose/retirement can no longer race aTestServeinto a half-started state (covered byTestServe_CloseRaceIsSafe, 50 iterations under-race).Performance
Consolidating three fields into one shrinks each
Requestby 16 bytes with no change in allocation count. benchstat (Apple M5 Max,-count=10,-benchtime=200ms), old = separate flags (e23d53f), new = int32 state:The
Request*benchmarks are added by the first commit (e23d53f) and stay in the tree as a regression guard.Tests
request_state_test.go:reqState.String; full-transition table (pending->claimed->running->finished, never-claimed recycle, claimed-not-running retire, double claim/startServe rejected, terminal-stays-finished); post-Close claim returns the cancellation cause (notErrRequestAlreadyClaimed); the debug-onlyfinishLockedterminal-state assertion; duplicateTestServepanics without disturbing the first serve;TestServe/Closerace (50×).Gate
gofmtclean ·go vetclean ·staticcheckclean ·golangci-lint0 issues ·go test -race ./...andgo test -tags debug -race ./...green · coverage 99.7% (only the pre-existingerrunusableuipath below 100%;finishLockedat 100%).