Protect the container walks in dumps() against concurrent mutation (#234) - #235

Thank you! Everything seems good here.

On the CI: yes, I think it should be added, especially because I do not have (yet?) use cases requiring the FT interpreter.

(Note for myself) On the snapshot cost: it would be interesting, even only to declare it in the documentation, to measure the impact, ideally adding it to the benchmarks tables.

Last, could you please help me understand the third point above, "No claim about re-entrancy", as I'm having difficulties grasping the meaning of the final part, "it did not reproduce, so I am not asserting a pre-existing bug there"?

Thanks — and taking the third point first, because the honest answer is that the sentence
was both unclear and wrong, and chasing your question turned up a bug in the released
version.

What I meant by "re-entrancy"

Not a second thread. One thread, hurting itself: dumps() calls back into Python in the
middle of a container walk — default= for a value it cannot serialize, PyObject_Str(key)
under MM_COERCE_KEYS_TO_STRINGS, __lt__ under MM_SORT_KEYS — and any of those callables
can mutate the very container being walked. That is the same shape of damage as #234 but
needs no free-threading at all, so if it reproduced it would be a bug on the GIL build and
older than this branch.

The clause you quoted meant: I tried one such case, it did not crash, and I did not want the
PR description to claim more than that. What made it a bad sentence is that "it" was a single
shape — a default= mutating a dict — while I let it read as though it covered the walks
generally. It did not cover lists. So I went and tested it properly.

The list case reproduces, on the released version, with the GIL

import rapidjson

class X: pass

lst = [X() for _ in range(256)]

def default(o):
    del lst[128:]          # shrink the list dumps() is walking, once
    return None

rapidjson.dumps(lst, default=default)

python-rapidjson 1.23 from PyPI, stock CPython, sys._is_gil_enabled() is True, one
thread, no PYTHON_GIL override:

Controls on 3.14.6, 8 runs each, same script, one line different:

SIGSEGV, and the stack has the callback under the list walk:

frame #0  _PyWeakref_GetWeakrefCount    EXC_BAD_ACCESS (code=1, address=0x10031)
frame #1  subtype_dealloc
...
frame #6  PyObject_CallFunctionObjArgs          <- the default= call
frame #7  dumps_internal<...>
frame #8  dumps_internal<...>
frame #9  do_encode
frame #10 dumps

The cause is the pair of lines this PR already changes:

Py_ssize_t size = PyList_GET_SIZE(object);
for (Py_ssize_t i = 0; i < size; i++) {
    ...
    PyObject* item = PyList_GET_ITEM(object, i);
    bool r = RECURSE(item);

size is captured before the loop and RECURSE can re-enter Python, so after the callback
shrinks the list the walk keeps indexing to the old length with an unchecked macro over
borrowed references. Exactly the same reasoning as the concurrent case in #234 — the only
difference is who does the shrinking.

I have not pinned the precise interleaving beyond that; the measured facts are the tables
and the stack.

What that means for this PR

The list hunk here already fixes it: same reproducer against this branch is 0/10 in both
GIL and free-threaded modes, because re-reading PyList_GET_SIZE(object) each iteration
stops at the new length and PyList_GetItemRef is bounds-checked.

The awkward part is targeting. This PR is against free-threaded, but master carries the
identical loop (rapidjson.cpp around line 2510 there) and the crash above is on a stock GIL
build, so that hunk fixes something on master independently of any of this. You may want it
there on its own. One snag if you do: PyList_GetItemRef is 3.13+, and master supports
older, so a backport needs either a version guard or PyList_GetItem plus an explicit
Py_INCREF. Happy to prepare that as a separate small PR against master if you would like
it — or to open it as its own issue first, if you would rather have it tracked separately from
the free-threading work. Your call which.

I searched the tracker (open and closed, issues and PRs) for re-entrancy, segfault, crash,
mutation and PyList_GET_ITEM before writing this and found nothing describing it; #48 is
object_hook on the parse side and is closed.

The dict half stayed negative

For completeness, since this is what the original sentence was actually about. Five
single-threaded shapes — default= mutating the dict, the same with MM_SORT_KEYS, a key
whose __str__ mutates it under key coercion, a key whose __lt__ mutates it under sorting,
and a default= that deletes half the remaining keys — all 0 faults, on both the unpatched
and patched builds, under both PYTHON_GIL=1 and PYTHON_GIL=0. I also checked the output
rather than only the exit status for one of them (duplicate keys emitted, truncated object,
invalid JSON): clean, all 64 keys, every run.

So I still have no dict re-entrancy bug to show you. I would not read that as proof there
isn't one — I could not construct the case, which is a fact about my inputs — but it is more
than the one probe behind the original sentence. And it is moot for this PR anyway: the two
dict walks now iterate a snapshot with strong references, which is immune to mutation from a
callback for the same reason it is immune to mutation from another thread.

To make the harness worth anything I ran a known-positive first: the #234 concurrent
reproducer crashes 6/6 on the unpatched build and 0/6 on the patched one. Without that, the
zeros above would be a statement about my test rig rather than about the code.

Your other two points

CI — glad to add it. Two independent pieces, and I'd rather you tell me which you want
than guess again: a free-threaded-tests job (actions/setup-python takes 3.14t directly,
alongside the existing deadsnakes debug job) and free-threaded wheels in build_wheels
(CIBW_ENABLE: cpython-freethreading). I can put either or both in this PR, or in a separate
one so this stays a source-only change.

Snapshot cost — agreed that it should be measured before it is documented. I'll benchmark
the PyDict_Items snapshot against the current streaming walk across the existing
benchmarks/ cases and post the numbers here. If the cost is real, the #ifdef Py_GIL_DISABLED split I mentioned becomes the obvious answer rather than a hypothetical, and
you can decide with a table in front of you instead of my guess.

(As on the PR itself: developed with AI assistance; the measurements are mine, run on this
machine, and every table above is reproducible from the script shown.)

One correction to the branch, found while preparing the master backport I mentioned above.

The item == NULL path I added to the list walk is reachable — that is the whole point of it,
another thread can shrink the list between the length check and the fetch — but I broke out of
the loop without clearing the error. PyList_GetItemRef() sets IndexError when the index is
out of range, so dumps() was returning a string with a live exception, and the caller sees a
spurious IndexError: list index out of range.

Measured on a free-threaded build, one thread calling dumps() in a loop while another
repeatedly shrinks and regrows the list:

Pushed as a separate commit so the reviewed one stays legible. Test suite unchanged: 914
passed, 26 skipped, 2 xfailed. The #234 reproducer and the re-entrancy cases from my previous
comment are all still 0 faults on the rebuilt module.

One judgement call in it, and I'd rather you make it than me. Stopping quietly means a
concurrent shrink yields a truncated array — the items that were there — which is the same
shape of answer the dict walks give, since a snapshot is also a view of a list that no longer
exists. The alternative is to keep the error and return false, so dumps() raises rather
than silently returning short. I went with truncation for consistency with the dict half, but
raising is defensible and it is a one-line change if you prefer it.

Sorry for reviewing-then-amending. The probe that caught it did not exist when you looked at
this; I wrote it because the master backport made me re-read that branch, and it is in the
same harness as the tables above.

Thank you.
On the judgement call: I will need to think about it some more, because I'm flip/flopping between the two approaches. On one side, I tend to see the case as something wrong the user is doing, and silently hiding that may be counterproductive. On the other side, the user can always pass an explicit copy of the structure to the dump, when he has multi-threaded code operating on it...
Maybe we should have a look at what other libraries do in this case?

On "what other libraries do" — there is a fairly direct precedent, and it cuts against the detect-and-raise side.

ultrajson#689, "thread safety fixes for free-threading", was opened by Kumar Aditya (a CPython core developer) in October 2025 and closed unmerged on 2026-05-28, after roughly seven months of discussion. That is essentially the approach you are weighing: notice the concurrent mutation and surface it rather than let the walk read freed memory. It did not land, and the reasoning there is worth reading directly rather than through my summary.

The other data point is what CPython did to its own instances. cpython#149816 collects a batch of free-threading races, and the fixes from it — cpython#149909 for example — do not detect misuse and raise. They make the access itself atomic, replacing a borrow-then-incref with PyList_GetItemRef. Faced with the same choice inside the interpreter, upstream took the "make the read safe" route rather than the "report the user's mistake" route.

That does not settle it for a third-party serializer, and I would rather not push. Your framing — that a caller mutating a structure mid-dump is doing something wrong — is a real position, and ujson's documentation takes it too. The one asymmetry I would flag is this: a raise is a behaviour change your users can see and argue with, whereas the current state is a use-after-free they cannot see and cannot defend against. Those are not symmetric in cost even where they are symmetric in principle.

Happy to leave it there. The CI half is merged and that was the part I had a real stake in; whatever you decide on the judgement call, I will not read a slower answer as a rejection.