Skip to content

Root freshly-boxed call args immediately, not after the whole list#14

Merged
jack-champagne merged 1 commit into
masterfrom
root-args-immediately
Jul 2, 2026
Merged

Root freshly-boxed call args immediately, not after the whole list#14
jack-champagne merged 1 commit into
masterfrom
root-args-immediately

Conversation

@jack-champagne

Copy link
Copy Markdown
Member

Summary

box_python_value's box_* calls return a jl_value_t* with nothing on
the Julia side keeping it alive. JuliaVal.__call__ (positional args and
kwargs), JuliaArray.__setitem__/jl_getindex, and JuliaDict.__setitem__
all box multiple values via separate ctypes calls and only root them once
the full list is handed to the consuming Julia call — leaving each
already-boxed element GC-invisible while later elements are still being
boxed.

If a GC cycle runs in that window, an earlier-boxed value can be collected
and its memory reused by a later allocation, corrupting it silently.

auto_convert_and_root (new in convert.py) roots each freshly-boxed
value via the existing jl2py_ref_create/jl2py_ref_release machinery
(already used by JuliaValGC — no new C code) the instant it's boxed,
released once the call completes. Existing JuliaVal args are left as-is
since they're already kept alive by the caller's own Python reference.

Reproduced directly before writing the fix: boxing 5 floats one at a
time with a forced full GC (GC.gc(true)) between each box call reused
the same freed memory for the next box, corrupting every element after
the first. Small ints are unaffected — Julia's boxed-int cache means
jl_box_int64 returns an already-rooted, permanent pointer for small
values, so this only bites floats/strings/complex/large ints (e.g. any
call with 2+ float args, which is common in pypiccolo's numeric code).

Test plan

  • Existing suite: 80 passed, 3 skipped (baseline on master, unchanged)
  • With fix: 83 passed, 3 skipped (+3 new tests in tests/test_arg_rooting.py)
  • New tests force a GC cycle between each argument's boxing (monkeypatching
    auto_convert_and_root) to make the race deterministic, covering the
    positional-args, kwargs, and dict-__setitem__ paths
  • Confirmed the new tests fail (can't even import the API) against
    unfixed master, and confirmed via a standalone ctypes-level repro
    that unfixed code silently corrupts values under forced GC pressure

box_python_value's box_* calls return a jl_value_t* with nothing on
the Julia side keeping it alive. JuliaVal.__call__ (positional args
and kwargs), JuliaArray.__setitem__/jl_getindex, and
JuliaDict.__setitem__ all boxed multiple values via separate ctypes
calls and only rooted them once the full list was handed to the
consuming Julia call -- leaving each already-boxed element
GC-invisible while later elements were still being boxed.

auto_convert_and_root roots each value via jl2py_ref_create the
instant it's boxed, released after the call. Reproduced the
corruption directly (forced GC between box calls reused freed memory
for the next box, corrupting the previous value) and confirmed the
fix closes it; added tests/test_arg_rooting.py covering the
positional, kwargs, and dict-setitem call paths.

Note: small ints are unaffected -- Julia's boxed-int cache means
jl_box_int64 returns an already-rooted, permanent pointer for small
values, so this only bites floats/strings/complex/large ints.
@jack-champagne jack-champagne merged commit 1f42392 into master Jul 2, 2026
1 check passed
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.

1 participant