Skip to content

builtin: reclaim captured closure contexts#18

Open
GGRei wants to merge 2 commits into
masterfrom
fix/27445-closure-context-leak-minimal
Open

builtin: reclaim captured closure contexts#18
GGRei wants to merge 2 commits into
masterfrom
fix/27445-closure-context-leak-minimal

Conversation

@GGRei

@GGRei GGRei commented Jun 17, 2026

Copy link
Copy Markdown
Owner

Summary

This rescues the closure leak fix from vlang#27446 with a smaller, scoped patch.

Captured closure contexts now use collectable memdup instead of memdup_uncollectable, while the closure runtime keeps live contexts reachable through a scanned table. Local captured closures that do not escape are cleaned up when their scope is left, including normal scope exit, return, and labeled/non-labeled break / continue.

The cleanup is conservative: closures are not destroyed when they escape through return values, copies, function arguments, struct fields, globals, defer blocks, spawn, or go.

Changes

  • Add explicit closure context ownership through closure_create_with_data.
  • Keep live closure contexts rooted under Boehm while the closure is alive.
  • Clear root table entries before deleting them.
  • Generate collectable closure contexts for captured anon functions.
  • Distinguish owned value receivers from borrowed pointer receivers for method-value closures.
  • Add local non-escaping closure cleanup in Cgen.
  • Add regression tests for the Capturing closures leak their captured context under -gc boehm (unbounded growth in frame-based apps) vlang/v#27445 memory growth pattern and generated C safety cases.

Tests

./v -o ./v_patch_27445 cmd/v
./v_patch_27445 test vlib/v/gen/c/closure_context_codegen_test.v
./v_patch_27445 -gc boehm test vlib/v/tests/fns/closure_context_boehm_root_test.c.v
./v_patch_27445 -gc none test vlib/v/tests/fns/closure_context_boehm_root_test.c.v
VJOBS=1 ./v_patch_27445 test vlib/v/tests/fns
./v_patch_27445 test vlib/v/gen/c/coutput_test.v
git diff --check

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e3c1b6c869

ℹ️ About Codex in GitHub

Your team has set up Codex to 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 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread vlib/v/gen/c/cgen.v
Comment thread vlib/v/gen/c/cgen.v
Comment thread vlib/v/gen/c/cgen.v Outdated
Comment thread vlib/v/gen/c/cgen.v
@GGRei GGRei force-pushed the fix/27445-closure-context-leak-minimal branch from e3c1b6c to cc9e5b6 Compare June 17, 2026 11:54
@GGRei

GGRei commented Jun 17, 2026

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: cc9e5b6381

ℹ️ About Codex in GitHub

Your team has set up Codex to 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 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread vlib/v/gen/c/cgen.v
@GGRei GGRei force-pushed the fix/27445-closure-context-leak-minimal branch from cc9e5b6 to 1ca0da1 Compare June 17, 2026 12:23
@GGRei

GGRei commented Jun 17, 2026

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1ca0da1cd4

ℹ️ About Codex in GitHub

Your team has set up Codex to 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 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread vlib/v/gen/c/cgen.v Outdated
Comment thread vlib/v/gen/c/cgen.v
Comment thread vlib/v/gen/c/cgen.v
@GGRei GGRei force-pushed the fix/27445-closure-context-leak-minimal branch from 1ca0da1 to f558787 Compare June 17, 2026 14:47
@GGRei

GGRei commented Jun 17, 2026

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f558787b1a

ℹ️ About Codex in GitHub

Your team has set up Codex to 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 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread vlib/v/gen/c/cgen.v
Comment thread vlib/v/gen/c/cgen.v
Comment thread vlib/v/gen/c/cgen.v
@GGRei GGRei force-pushed the fix/27445-closure-context-leak-minimal branch 2 times, most recently from 60f52df to 6c0a119 Compare June 17, 2026 16:55
@GGRei

GGRei commented Jun 17, 2026

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Chef's kiss.

Reviewed commit: 6c0a119ba2

ℹ️ About Codex in GitHub

Your team has set up Codex to 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 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@GGRei GGRei force-pushed the fix/27445-closure-context-leak-minimal branch from 6c0a119 to 70ad17d Compare June 17, 2026 17:34
@GGRei

GGRei commented Jun 17, 2026

Copy link
Copy Markdown
Owner Author

@codex review

@MartenH

MartenH commented Jun 18, 2026

Copy link
Copy Markdown

@GGRei Have you checked out fat closures as an alternative to this?
RFC_closures.md

@GGRei

GGRei commented Jun 18, 2026

Copy link
Copy Markdown
Owner Author

@MartenH Yes, I looked at the fat-closures RFC. I agree it is the cleaner long-term direction: make pure V closures GC-visible and keep trampolines only for C ABI coercions.

I do not think it is a drop-in alternative for this PR, though. This PR intentionally fixes the leak under the existing closure representation. Fat closures would require changing fn value representation/calling convention, Cgen call lowering, C FFI coercions, -gc none/-autofree ownership, bootstrap, and eventually native backend support.

Given where the V1/C backend is in its lifecycle, I would prefer not to block this scoped fix on a much larger closure ABI/codegen redesign. My preference would be land the current fix as an interim unblocker, and treat fat closures as a separate staged RFC/implementation, probably aligned with future V2 ownership/autofree work.

If/when fat closures land, the manual lifetime/reclaim API can be removed or deprecated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants