fix(runtime): avoid reentrant weak cleanup deadlocks - #2575
Open
cpunion wants to merge 3 commits into
Open
Conversation
This was referenced Sep 12, 2026
Collaborator
Author
|
@fennoai Please review the weak-cleanup reentrancy fix, especially the one-time atomic pending-list publication, registration-time batch reclamation, and address-reuse identity/live checks. The same public-API pressure test fails 3/3 on the old runtime with matching native self-deadlock stacks and passes 53 times with default GC markers plus 20 times with one marker locally. Only the affected Qiniu job is currently being validated for this PR and dependent #2567; full matrices will follow after both targeted jobs pass. |
Contributor
|
FennoAI is processing your request, please wait a moment... |
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review: weak-cleanup reentrancy fix
The core design is sound and directly addresses the captured self-deadlock. Moving the map mutation and lock acquisition out of the GC-finalizer path — the cleanup now only does an atomic live store plus a lock-free CAS push onto the intrusive dead list (retireWeakHandle), with reclamation deferred to registration under the existing weakState.mu (drainWeakHandles) — correctly removes the recursive weakState.mu acquisition inside a finalizer that the native stacks showed deadlocking. The read path (llgoMakeStrongFromWeak) stays a single atomic load with no lock/map/alloc, so there is no hot-path regression.
The two focus areas hold up under review:
- Address-reuse identity check (
drainWeakHandles,weakState.m[h.key] == h) correctly prevents a delayed old cleanup from deleting a fresh handle after the encoded address is reused — matching the mirrored logic inmfinal.go. - Live check at registration (
h != nil && live != 0) correctly rejects a handle that was marked dead but not yet published/drained, so a stale entry is never re-vended; its later push then findsm[key] != hand skips the delete. This interaction is the subtle case the PR targets and it is correct.
Findings below are hardening/accuracy items, not blockers. I did not run the suite; local verification numbers are as reported by the author.
cpunion
marked this pull request as ready for review
September 12, 2026 16:15
Contributor
There was a problem hiding this comment.
Review: weak-pointer cleanup reentrancy fix
The core fix is sound. Moving the map mutation out of the GC finalizer and onto a lock-free Treiber "dead" stack (atomic CAS push, no lock/no alloc) genuinely eliminates the mutex-reentrancy/allocate-in-finalizer deadlock, and it mirrors the proven freeCleanupSlot/popCleanupSlot pattern already in z_gc.go. The two reuse guards are exactly right: the live != 0 check at registration avoids handing back a handle a finalizer already marked dead, and the weakState.m[h.key] == h identity check prevents a stale handle from deleting a newer one that reused the same encoded address. Test coverage is thoughtful — TestPointerIdentity pins handle-identity semantics, and the stress test isolates the deadlock in a child process with an out-of-process hard deadline.
Two non-blocking observations are left inline. No correctness-breaking defect was found in the lock-free logic.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
LLGo baseline benchmarks
Program measurements
Core language and compiler benchmarks
Timer runtime benchmarks
Compared with |
LLGo WebAssembly build benchmarks
WebAssembly output sizes
LLGo WebAssembly build measurements
Compared with |
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.
Problem
Fix a real weak-cleanup self-deadlock captured in the Qiniu Linux amd64 / LLVM 22 / Go 1.27 shard-0 investigation for #2567. In the failing diagnostic job,
crypto/ecdsawas the only unfinished package. Native stacks taken two minutes apart show the same thread holdingweakState.muinside a weak cleanup, allocating duringmapaccess1_fast64, recursively enteringGC_invoke_finalizers, and then blocking in another weak cleanup on that same non-recursive mutex. The parent LLGo process is waiting for the test child, not looping in compilation. This weak callback code predates #2567 and is unchanged between its previously passing and subsequently timed-out revisions. The earlier timeout attempts have no surviving native stacks, so this PR does not claim each had the same cause.Change
Weak cleanup now only atomically marks its handle dead and publishes it to an intrusive pending list. Registration detaches one batch and removes matching map entries while holding the existing registry lock. Each producer initializes its private node's ordinary
nextlink before the sequentially consistent head CAS publishes it, stops accessing the node after publication, and the head Swap transfers the detached batch to the sole drainer; the head atomics therefore order the ordinary link accesses without redundant per-link atomics. A live-state check handles the interval between invalidation and publication; an identity check prevents a delayed old cleanup from deleting a new handle after address reuse. The pending list keeps handles reachable, but retains only the existing encoded referent identity. Weak-pointer reads are unchanged, and no background goroutine or collector policy change is introduced. Clearing each detached node'snextprevents a user-held dead weak pointer from retaining the rest of the batch. Reclamation is registration-gated, with the explicit limits below.Add regular weak-identity coverage and the opt-in
test/_stress/runtime/weakregression. It uses only publicweak.Makeandruntime.GCAPIs, keeps referents alive until a whole batch is registered, releases them from an exiting goroutine, and checks bounded collection progress. A helper-process deadline turns the old runtime deadlock into a finite test failure. There are no production test hooks, retries, sleeps, or reduced-pressure success conditions.Standard-library reuse and reclamation limits
LLGo already compiles GOROOT's public
weak.Pointer,Make, andValueimplementation. This file provides the two collector-dependent runtime hooks. In Go 1.27's runtime, a registration node is attached to the referent's GC span; sweep removes that node, zeros the heap-allocated handle, and returns the node to the runtime allocator. A user-held weak pointer still retains the dead handle to preserve identity; the handle itself becomes collectible when no roots retain it. Those runtime routines depend on Go's span metadata, marking/sweeping synchronization, write barriers, and scheduler, so they cannot be directly reused with LLGo's BDWGC heap.This PR removes dead registry entries only during the next non-nil
weak.Make. If registration stops, the last dead batch and its map entries remain retained; neitherPointer.Valuenorruntime.GCdirectly drains them. They retain handles, not the referents. The batch has no fixed size bound, so this is a real metadata-retention tradeoff, not a guarantee of bounded memory relative to currently live weak pointers. Continued registrations detach pending batches instead of accumulating all historical dead handles. Deleting entries also does not shrink the map's backing capacity or guarantee an immediate RSS reduction.Each non-nil registration adds one atomic head exchange, including existing-handle lookups. A captured batch of D handles requires O(D) work under the registry mutex: taking one batch prevents cleanup arrivals from indefinitely extending that batch, but does not cap a single registration's latency. Each handle gains one pointer; the reduced closure allocation described below must not be interpreted as a whole-process memory saving.
Pointer.Valuekeeps its existing allocation-free, lock-free path.Complete reclamation after automatic GC with no subsequent registration needs a separately scheduled safe consumer or deeper collector integration. Adding a drain only to explicit
runtime.GCwould not cover automatic collection and would still require reentrancy analysis. A consumer must use an allocation-safe notification path; an ordinary channel is not automatically suitable because LLGo's channel receive can allocate while holding its own lock. Native LLGo goroutines currently use OS threads, so a dedicated worker also adds thread/stack and lifecycle costs. This PR does not add that mechanism or claim to solve idle metadata retention. The source comments document the ownership handoff, retained-identity distinction, and reclamation limits.Local verification
b07cd12ad, 3 independent executionsGC_MARKERS=1-race -count=10llgo test -count=3 ./test/std/weak,go test -race -count=3 ./test/std/weak, and the selected wasm/atomic source-patch type-check tests also pass. Native disassembly confirms that the valid-handle cleanup path contains only loads/stores and atomic operations, with no lock, map, or allocator calls. Although a handle gains one pointer, the cleanup closure no longer captures a separate key: this arm64 build reduces direct registration allocation requests from four allocations totaling 48 bytes to three totaling 40 bytes, excluding map/cleanup internals and allocator size-class rounding. This is code-generation evidence, not a whole-process memory benchmark.The standard-Go
-raceruns validate the public test harness against Go's runtime; they are not race-detector coverage of LLGo's runtime implementation.Targeted CI verification
The complete previously failing Qiniu Ubuntu 24.04 large / LLVM 22 / Go 1.27 shard-0 job passed for this fix in run 34703427423, including all later integration checks. The original test, symbol, and build-mode phases completed in 136, 12, and 615 seconds; the job completed in 23m37s without ptrace sampling. The same target compiler and public stress source were then compiled against unmodified main runtime
b07cd12ad: the helper hit its 60-second deadline, and the native stack validator confirmed on one thread two weak cleanup callbacks, twoGC_invoke_finalizersframes, allocatingmapaccess1_fast64, and theweakStatemutex self-lock. Recompiled against the fixed runtime, the same 16-round/8,192-object stress test passed three times in 0.14–0.16 seconds per run.The ordinary PR matrix for
50409da6ealso completed with 64 successful checks, no failure, and only the expected tag-gated release job skipped. Its normal Qiniu shard-0 job independently passed in 22m26s, and Codecov reports all modified coverable lines covered. The subsequenta7863ee05commit adds source comments only; the full-matrix results cited here are for50409da6e, not a new CI run on the documentation commit.#2567 was rebased onto this fix without changing its five panic-location patches. Its complete targeted Qiniu job independently passed in run 34702086188, including a 136-second first test phase, 12-second symbol check, 602-second build-mode checks, all later integration checks, and three 0.16-second weak-stress runs, again without ptrace sampling. The temporary diagnostic workflow in draft #2573 and its unrelated MakeFunc/continuous-GC starvation reproducer are not included in either production PR.