Root freshly-boxed call args immediately, not after the whole list#14
Merged
Conversation
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.
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
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
box_python_value'sbox_*calls return ajl_value_t*with nothing onthe Julia side keeping it alive.
JuliaVal.__call__(positional args andkwargs),
JuliaArray.__setitem__/jl_getindex, andJuliaDict.__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 inconvert.py) roots each freshly-boxedvalue via the existing
jl2py_ref_create/jl2py_ref_releasemachinery(already used by
JuliaValGC— no new C code) the instant it's boxed,released once the call completes. Existing
JuliaValargs are left as-issince 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 reusedthe 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_int64returns an already-rooted, permanent pointer for smallvalues, 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
master, unchanged)tests/test_arg_rooting.py)auto_convert_and_root) to make the race deterministic, covering thepositional-args, kwargs, and dict-
__setitem__pathsunfixed
master, and confirmed via a standalone ctypes-level reprothat unfixed code silently corrupts values under forced GC pressure