builtin: collect captured closure contexts under -gc boehm (fix unbounded stored-closure leak)#27446
builtin: collect captured closure contexts under -gc boehm (fix unbounded stored-closure leak)#27446MartenH wants to merge 18 commits into
Conversation
…nded stored-closure leak) Capturing closures (`fn [x] (...)`) leaked their captured context whenever the closure was stored (assigned, returned, kept in a collection, used as a struct-field event handler). gen/c/fn.v allocated the context with memdup_uncollectable, and the only reclaimer (closure_try_destroy) was emitted only for temporary closures -- and it called free(), a no-op under -gc boehm, so it never freed the context anyway. Long-running immediate-mode GUIs that rebuild handler closures every frame grew without bound. Fix: allocate the context collectably (memdup) and keep it reachable through a GC-scanned table (g_closure_live) while the closure is live; reclaim by clearing the trampoline slot and dropping the table entry -- no explicit free, so it is idempotent and cannot double-free. Adds an opt-in frame-epoch reclamation API (begin_frame_build/end_frame_build/reclaim_frames/live_count) for frame-based apps; programs that never call it behave as before. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f9e5dee56
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…es through it Review fix (vlang#27446): the leak fix changed gen/c/fn.v to allocate the captured-closure context with memdup instead of memdup_uncollectable, but markused still rooted only memdup_uncollectable under anon_fn. Under -skip-unused a program whose only memdup reference is a captured closure would emit builtin__memdup while markused stripped its definition -> C/link failure. - markused.v: root `memdup` (not `memdup_uncollectable`) for anon_fn. - gen/c/cgen.v: the method-value closure path (`obj.method` bound as a value) also allocated its receiver context with memdup_uncollectable and leaked the same way; route it through memdup too. memdup_uncollectable is now emitted by no codegen path, so the single `memdup` root suffices and the leak fix covers both closure-creation sites. Verified under -skip-unused: a capturing closure and a method-value closure both link and run; generated C contains no memdup_uncollectable; closure correctness tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adds vlib/v/tests/fns/closure_reclaim_test.c.v for vlang#27446: - captured context survives a full GC cycle while the closure is live (no premature free — the core safety property of collectable ctx + live table); - try_destroy is idempotent (double-destroy / nil are safe no-ops); - closures created outside a begin/end_frame_build window (sentinel frame) are never auto-reclaimed by reclaim_frames and keep working; - reclaim_frames bounds live_count under per-frame closure churn (the leak regression: steady-state count after 100 vs 1000 frames must not grow). Count-based checks are guarded with `$if gcboehm ?` (the bookkeeping is boehm-only); correctness checks run under every gc mode. Verified green under -gc boehm, the default, and -gc none. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adds vlib/builtin/closure/closure_reclaim_test.c.v for vlang#27446 as a module-internal test (module closure, no import — like the module's own files), since builtin.closure is a builtin part, not a normal importable module: - captured context survives a full GC cycle while the closure is live (no premature free — the core safety property of collectable ctx + live table); - try_destroy is idempotent (double-destroy / nil are safe no-ops); - closures created outside a begin/end_frame_build window (sentinel frame) are never auto-reclaimed by reclaim_frames and keep working; - reclaim_frames bounds live_count under per-frame closure churn (the leak regression: steady-state count after 100 vs 1000 frames must not grow). Count-based checks are guarded with `$if gcboehm ?` (the bookkeeping is boehm-only); correctness checks run under every gc mode. Verified vfmt-clean and green under -gc boehm, the default, and -gc none. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
MartenH
force-pushed
the
fix-closure-context-leak
branch
from
7756263 to
5d94c93
Compare
Adds vlib/builtin/closure/closure_reclaim_test.c.v for vlang#27446 as a module-internal test (module closure, no import — like the module's own files), since builtin.closure is a builtin part, not a normal importable module: - captured context survives a full GC cycle while the closure is live (no premature free — the core safety property of collectable ctx + live table); - try_destroy is idempotent (double-destroy / nil are safe no-ops); - closures created outside a begin/end_frame_build window (sentinel frame) are never auto-reclaimed by reclaim_frames and keep working; - reclaim_frames bounds live_count under per-frame closure churn (the leak regression: steady-state count after 100 vs 1000 frames must not grow). Count-based checks are guarded with `$if gcboehm ?` (the bookkeeping is boehm-only); correctness checks run under every gc mode. Verified vfmt-clean and green under -gc boehm, the default, and -gc none. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
MartenH
force-pushed
the
fix-closure-context-leak
branch
from
5d94c93 to
641b093
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 641b093da0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
map.delete zeroes only the key; the ClosureLiveInfo value (whose `ctx` field is GC-scanned) can linger in the backing DenseArray until a later compaction — which for small or sparse maps may never happen — so the captured context stays rooted and is never reclaimed after try_destroy/reclaim_frames, even though live_count drops. Null the value in place before deleting the key so the freed slot holds no live pointer. Addresses the Codex P2 review on closure.c.v:499.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8e709d6aab
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
g_closure_frame and g_closure_in_build were process-global, so a capturing closure created on a worker thread while another thread was inside begin_frame_build/end_frame_build got stamped with that frame and became eligible for reclaim_frames — even though it was never part of the discarded frame tree, risking a dangling function pointer. (Immediate-mode apps commonly build frames on the UI thread while worker threads register long-lived callbacks.) Mark both globals @[thread_local] so the frame-build window belongs to the thread that opened it; closures created on any other thread get the closure_frame_never sentinel and are never auto-reclaimed. Addresses the Codex P2 review on closure.c.v:449.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 508b6499b3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Since g_closure_frame/g_closure_in_build are thread-local but g_closure_live is process-global, reclaim_frames on one thread would otherwise compare its own frame counter against closures stamped by every other build thread and could free another thread's still-live closures. Record an owner id (assigned per frame-building thread under the closure mutex) on each frame-stamped closure and only reclaim closures owned by the calling thread. Addresses the Codex P2 review on closure.c.v (reclaim owner-thread).
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ab4d3e155
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ab4d3e155
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
… false leaks `-gc boehm_leak` defines `gcboehm` too, so closure_release_slot's `$if gcboehm ?` branch runs in leak-detection mode. There `free` is a real `GC_FREE` and find-leak reports any object that goes unreachable without an explicit free, so dropping the g_closure_live root without freeing made every destroyed/frame-reclaimed context show up as a false leak under gc_check_leaks(). Add a `$if gcboehm_leak ?` explicit free of the (now unrooted, dead) context after clearing the map entry; plain boehm stays collectable with no explicit free. Safe: the slot's `already_freed` guard makes release once-per-slot, and each context is a distinct per-closure memdup. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 639abbac96
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
… contract A closure value is the bare address of its trampoline slot, so once a slot is freed and reused the new closure's handle aliases the old one bit-for-bit. The repeated-destroy guard only holds while the slot is still free; a stale try_destroy() after reuse would release the new closure. Detecting that needs the handle to carry a per-slot generation, which the single-pointer closure ABI cannot do. Document the contract accordingly (treat a destroyed handle as a freed pointer) rather than implying full stale-handle idempotency. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: edb36382ad
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
The @[thread_local] attribute on a __global was silently ignored: the C generator only emitted _Thread_local for the hardcoded g_memory_block, and the parser never recorded the attribute at all. So builtin.closure's g_closure_frame/g_closure_in_build/g_closure_owner were process-global despite the attribute, defeating the per-thread frame-build hardening (a worker-thread closure built during the UI thread's frame window could be wrongly reclaimed by reclaim_frames). - ast.GlobalField gains is_thread_local - the parser sets it from the `thread_local` attr (alongside markused/weak/...) - consts_and_globals.v emits `_Thread_local ` for any global carrying it Adds a deterministic thread-local test (channel barrier: every thread writes before any reads, so a non-TLS global would fail the per-thread assertion). Verified the emitted C now carries _Thread_local for @[thread_local] globals; passes under gcc and tcc. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6d51a1901a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
A @[thread_local] global gets a per-thread copy, but only a *constant* initializer
is replicated into each thread's copy; a non-constant initializer runs once in
_vinit() on the main thread, so other threads would observe a zero-initialized
value (e.g. nil function pointers). Error on such inits instead of silently
handing out zeroed thread-locals. Constant inits (literals, numeric casts of
literals like u32(0), fixed-array, unsafe{nil}, @[cinit]) are still accepted —
the closure frame-build globals use exactly those.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The new checker rule (a @[thread_local] global must be const-initialized) flags
`__global context = Context{}`, whose default allocator is built at runtime in
_vinit() — which is exactly the latent bug: under real TLS that initializer runs
only on the main thread, so other threads would see a zeroed context (nil
allocator fn pointers). Drop the runtime initializer (zero-init per thread) and
add ctx(), which installs default_allocator the first time the context is used on
a thread. alloc/free/realloc/push_allocator route through it. Verified: compiles
and alloc/free work.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b7af2e3195
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c5296622f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| is_const_init := e.is_literal() || (e is ast.ArrayInit && e.is_fixed) | ||
| || (e is ast.UnsafeExpr && (e.expr is ast.Nil || e.expr.is_literal())) | ||
| || node.attrs.contains('cinit') |
There was a problem hiding this comment.
With this check, an explicit @[thread_local] initializer is accepted only when the parsed expression is a literal/fixed array/unsafe literal. The TLS globals added in vlib/builtin/closure/closure.c.v use u32(0) and u64(0), which are ast.CastExprs even though cgen already treats casts of constants as C constants, so rebuilding with the new checker rejects the compiler's own closure globals as non-constant. Please include constant casts here or reuse the existing C-constant initializer predicate.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c13ee759f5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4ab4b8dfef
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ee286debfa
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| tls_kw := if (field.name == 'g_memory_block' && g.pref.prealloc) | ||
| || field.is_thread_local { | ||
| 'V_THREAD_LOCAL ' |
There was a problem hiding this comment.
When field.is_thread_local is set here, the initializer still follows the normal global path below: if should_init is false (for example under -usecache or non-built modules), even a constant initializer falls through to an _vinit() assignment. _vinit() only initializes the current thread's TLS instance, so a non-zero @[thread_local] __global x = 7 can read as zero from spawned threads in those builds. The TLS case needs to force a direct C initializer whenever the checker accepted it as constant.
Useful? React with 👍 / 👎.
@GGRei I can test it shortly with GUI and get back to you |
|
Wait, I have one last issue with the BSD family in CGen, and then you’ll be able to test. Thanks! |
|
It's ok now. Fixed. :) |
|
@GGRei Ran it locally here and got and from claude I got. The reason is architectural: GGRei's scope-exit reclaim only frees closures that don't escape their scope. But gui's leak is precisely the escaping ones — event-handler closures created in the view function and stored in the layout/renderer tree, rebuilt every frame. Scope reclaim correctly leaves those alone (they're still referenced at scope exit), so they accumulate. Your frame-reclaim approach (#27446) is gui-specific exactly for this: it reclaims by frame-epoch regardless of escape, relying on the "handlers rebuilt each frame" contract. Net: it's a more correct general fix (frees only provably-dead scope-local closures) but it doesn't target the gui's leak class (per-frame handler closures that escape into the layout tree). Your #27446 frame-epoch approach was purpose-built for exactly that, which is why it plateaus and GGRei's doesn't. |
|
@MartenH Thanks, that result makes sense and matches the intended limitation of 70ad17d. 70ad17d is deliberately conservative: it fixes the compiler/runtime foundation by making captured closure contexts collectable, keeping them rooted while live, and destroying non-escaping temporary closures at safe scope/branch/return exits. It does not destroy closures that escape their local scope, because doing that generically would be a use-after-free risk. So if cantester is leaking handlers created every frame and stored in the GUI layout/renderer tree, then I agree this PR alone will not fully fix that leak. Those handlers escape by design, so CGen must keep them alive unless there is an explicit owner/lifetime policy. The remaining fix should be at the GUI ownership layer: either vlang/gui should release/destroy callbacks it owns when replacing the frame/tree, avoid recreating them, or use a frame-epoch reclaim mechanism under a clear GUI-specific contract. Your frame-reclaim result is useful evidence for that direction. In short: 70ad17d is the safe runtime/CGen foundation, not the complete cantester/vlang-gui fix. |
|
@MartenH If that sounds good to you, feel free to take my commit/patch into your PR and ask for review/merge. Would you also be willing to handle the follow-up fix on the vlang/gui side? |
|
@GGRei I have actually no idea how to solve it in GUI with just your patch and since you clearly have an idea I suggest you do it, it would be very welcome and I would be happy to test. Regarding the PR, it's your patch so create the PR |
|
@MartenH The PR is open, and the follow-up strategy is described inside. |
2 participants
Fixes #27445.
Problem
Capturing closures (
fn [x] (...)) leak their captured context whenever the closure is stored, under the default-gc boehm.gen/c/fn.vallocates the context withmemdup_uncollectable, and the only reclaimer —closure_try_destroy— is emitted only for temporary closures (and usedfree(), a no-op in boehm mode, so it didn't free the context anyway). Minimal repro / measurements: see the linked issue (closure →live0→2 GB; identical array churn without a closure → flat).Fix
Make the context collectable and keep it reachable through a GC-scanned table while the closure is live; reclaim it by clearing the trampoline slot and dropping the table entry (no explicit free).
gen/c/fn.v:memdup_uncollectable→memdup.vlib/builtin/closure/closure.c.v:g_closure_live map[voidptr]ClosureLiveInfo({ctx, frame}). Thectxin the GC-scanned map value keeps the collectable context alive while the closure lives (the trampoline slot that also points at it is in an mmap page the GC doesn't scan).closure_createinserts;closure_try_destroyremoves + returns the slot to the free list — noGC_FREE, so it's idempotent and can never double-free; the GC reclaims the context.@[markused]) API:try_destroy(c),begin_frame_build(),end_frame_build(),reclaim_frames(keep u32),live_count(). Closures created outside abegin/end_frame_buildwindow get a sentinel frame and are never auto-reclaimed (so app-setup/event-handler closures are untouched).Why not simpler
closure_try_destroy+ realGC_FREE, called by the app → double-free with shared transient closures (no idempotent explicit free).GC_add_roots→ premature free (GC_MAX_ROOT_SETScap drops later page roots). The table+epoch design avoids both.Compatibility / risk
v selfrebuilds cleanly.Validation
live2 GB → 0 with reclamation.map/filter, captured-string closures invoked in a loop, and double/tripletry_destroyall pass.Window.update()): a livedata_grid(180 s / 3400 frames) goes from unboundedlive(→ 364 MB) to bounded (46–126 MB), RSS plateaus.Open to reshaping the API (e.g. a single
set_frame(u32)instead ofbegin/end_frame_build) if preferred.🤖 Generated with Claude Code