Repository navigation
runtime: avoid user cleanup reentrancy in BDWGC callbacks - #2587
Merged
xushiwei merged 2 commits intoSep 17, 2026
Merged
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review: run user cleanups outside BDWGC finalizer callbacks
The core change is sound. Moving cancelable cleanup callbacks off the BDWGC collecting goroutine (via the intrusive cleanupQueue + worker) correctly fixes the deadlock where a finalizer runs synchronously while an application lock is held. I verified the important paths:
- The fix covers the API the test exercises.
runtime.AddCleanup→AddCancelableCleanupPtr→ the queued path, soTestCleanupDoesNotBlockCollectorgenuinely validates the new behavior. - The remaining synchronous
id==0path is intentional and safe. Its only caller isweak_llgo.go'sretireWeakHandle, which by design neither allocates nor takes a lock (see its comment atweak_llgo.go:50-52) — so leaving it inline in the finalizer is correct. - Slot reuse + generation and the lock-free free-list are correct. The
generation<<32 | index+1id defeats staleStopcalls, and slot pops are serialized undercleanupSlots.mu, so no ABA on the free-list head.StopCleanupPtrcorrectly cancels a queued-but-unclaimed entry via thecleanupActive→cleanupStoppedCAS racing the worker'scleanupActive→cleanupRunningCAS. - Comments accurately describe the new behavior.
The findings below are design/robustness considerations, not correctness blockers.
Test coverage. The new test covers the deadlock-avoidance scenario well, but the PR adds substantial machinery (queue, worker fan-out, slot reuse, cancel-after-queue) that is otherwise untested. Consider adding: (1) a Stop-after-object-unreachable-but-before-worker-runs test — the exact "cancel a queued cleanup" case the StopCleanupPtr doc now claims to support; (2) a many-cleanups stress test to exercise popCleanupSlot/freeCleanupSlot reuse and concurrent execution.
| e.next = nil | ||
| // Execute outside the queue lock so allocations made by a callback | ||
| // can enqueue more cleanups without reentering user code. Hosted | ||
| // goroutines each own an OS thread: reuse this worker rather than |
Contributor
There was a problem hiding this comment.
Unbounded goroutine fan-out under GC pressure. runCleanups starts one go runCleanup(e) per drained entry with no ceiling. A GC cycle that finalizes many objects carrying Cleanup handles will spawn one goroutine per cleanup at once — precisely when scheduler/memory pressure is highest. The comment's goal ("a blocked cleanup must not prevent other cleanups from running") can be met with a bounded worker pool or a semaphore, which would cap concurrent goroutines and avoid paying a goroutine spawn+teardown for each typically-short callback. Worth considering as a follow-up if high cleanup churn is expected.
| cleanupQueue.mu.Lock() | ||
| e.next = cleanupQueue.head | ||
| cleanupQueue.head = e | ||
| cleanupQueue.ready.Signal() |
Contributor
There was a problem hiding this comment.
Minor: ready.Signal() fires on every enqueue, including bursts where the worker is already draining, so a burst of finalizers issues one cond-signal syscall per entry on the collecting goroutine's path. Signaling only on the empty→non-empty transition (i.e. when cleanupQueue.head == nil before the push) would coalesce these. Not a correctness issue — the current form is safe.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
LLGo WebAssembly build benchmarks
WebAssembly output sizes
LLGo WebAssembly build measurements
Compared with |
LLGo baseline benchmarks
Program measurements
Core language and compiler benchmarks
Timer runtime benchmarks
Compared with |
cpunion
approved these changes
Sep 16, 2026
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
3 participants
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
Root cause and design
client-go/rest can deadlock when a transport-cache cleanup holds the cache mutex and allocates. BDWGC recursively invokes another cleanup, which attempts to acquire the same mutex. The timer thread can also block in cleanup, preventing the test timeout from firing. This remains after the internal weak cleanup fix in #2575.
The intrusive queue is initialized before user cleanup registration and does not allocate while holding its native mutex. The single worker detaches a batch, releases the queue lock, then runs callbacks directly. Callback allocations can enqueue more work but cannot synchronously reenter another user cleanup. A long-running or blocked callback delays subsequent callbacks; the runtime does not create additional threads to compensate.
This change targets hosted BDWGC builds; wasm, baremetal and nogc are unchanged.
Validation
Current single-worker revision, local macOS ARM64:
Linux AMD64 CI validation is pending. This PR does not claim to resolve the separate client-go/transport GC retention assertions or semver test portability issue.