| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
…red between threads The HAMT iterator that backs iteration over a contextvars.Context keeps its entire depth-first cursor -- i_level, i_pos and the borrowed node pointers in i_nodes -- inside the iterator object, and advanced it with plain reads and writes. Two threads calling next() on the same iterator interleaved those updates, leaving i_level pointing at an i_nodes slot that was never filled in, so the next step dereferenced a stale or NULL node and crashed. Advance the cursor with the iterator locked, so the whole descent is atomic. The HAMT itself is immutable, so no other locking is required. As with the other free-threaded iterators, concurrent iteration may still skip or repeat items, but it no longer crashes.
|
Seems you've also discovered that the new free-threaded output directories aren't in .gitignore. You won't get merged until all those files you added are removed, and we should fix the .gitignore in its own PR |
Sorry, something went wrong.
|
Sorry about that, and thank you for catching it — the build artifacts were swept in by an automated commit on my side, not deliberately. I've removed them: the branch is force-pushed and this PR is back to the four files it should have been (Python/hamt.c, Include/internal/pycore_hamt.h, the test, and the NEWS entry). And yes — PCbuild/python.props puts free-threaded builds in win32t/amd64t/arm32t/arm64t, but .gitignore only lists the GIL-enabled four, so build.bat --disable-gil leaves its whole output untracked. Fixed separately in #154641, as you suggested. |
Sorry, something went wrong.
|
Overall, this looks good. The added INCREF/DECREFs on key/value are unneeded, I think. Suggested patch below. --- a/Python/hamt.c
+++ b/Python/hamt.c
@@ -2475,8 +2475,8 @@ static PyObject *
hamt_baseiter_tp_iternext(PyObject *op)
{
PyHamtIterator *it = (PyHamtIterator*)op;
- PyObject *key = NULL;
- PyObject *val = NULL;
+ PyObject *key;
+ PyObject *val;
hamt_iter_t res;
/* The depth-first cursor (i_level, i_pos and i_nodes) is stored in the
@@ -2487,15 +2487,16 @@ hamt_baseiter_tp_iternext(PyObject *op)
dereference a stale or NULL node. Serialize the whole descent on the
iterator; the HAMT itself is immutable, so nothing else needs locking.
+ key and val are borrowed references into the tree and stay valid after
+ the lock is released: they are pinned via hi_obj, which this function
+ never drops, and tp_clear (the only other code that drops it) cannot
+ run concurrently because an in-flight tp_iternext call implies the
+ caller owns a reference to the iterator, keeping it reachable.
+
Concurrent iteration can still skip or repeat items, which matches the
behaviour of the other free-threaded iterators. */
Py_BEGIN_CRITICAL_SECTION(op);
res = hamt_iterator_next(&it->hi_iter, &key, &val);
- if (res == I_ITEM) {
- /* hamt_iterator_next() returns borrowed references. */
- Py_INCREF(key);
- Py_INCREF(val);
- }
Py_END_CRITICAL_SECTION();
switch (res) {
@@ -2504,10 +2505,7 @@ hamt_baseiter_tp_iternext(PyObject *op)
return NULL;
case I_ITEM: {
- PyObject *item = (*(it->hi_yield))(key, val);
- Py_DECREF(key);
- Py_DECREF(val);
- return item;
+ return (*(it->hi_yield))(key, val);
}
default: {
|
Sorry, something went wrong.
…rnext() Apply review feedback from nascheme: the temporary references taken around the critical section are unnecessary. * key and val are borrowed into the HAMT, which is immutable and is pinned for the iterator's lifetime by hi_obj, so they stay valid after the critical section is released. hamt_baseiter_tp_clear() is the only code that drops hi_obj and cannot run while a tp_iternext call is in flight, since the caller owns a reference to the iterator. * All three yield functions already take their own references (Py_NewRef() for keys and values, _PyTuple_FromPair() for items), so the returned object never depended on the temporary ones. * Record that lifetime argument in the comment above the lock. This also restores the key and val declarations to the form main already uses.
|
Thanks — you're right, applied verbatim. For the record, three things make the borrowed references safe here:
So the pair only cost two extra atomic refcount updates per step, on objects that are by construction contended across the iterating threads. Your comment is kept as written. With this the diff against main in hamt_baseiter_tp_iternext() is down to the comment plus the critical section — the declarations and the switch are back to being identical to main. |
Sorry, something went wrong.
…h-154535-hamt-iterator-thread-safety # Conflicts: # Lib/test/test_free_threading/test_iteration.py
| Back | FazBrowse Home | New Git URL |
Fixes the SIGSEGV reported in #154535.
The bug
Python/hamt.c's iterator keeps its entire depth-first cursor inside the iterator object — i_level, i_pos[], and i_nodes[] (which holds borrowed node pointers) — and hamt_iterator_next() advanced it with plain reads and writes, with no locking:
A single step is not a single write: descending pushes i_pos[next_level], i_nodes[next_level] and then i_level, while ascending does i_level--. Two threads advancing the same iterator interleave those updates, so i_level ends up pointing at an i_nodes[] slot that was never filled in for that level. Because i_nodes holds borrowed pointers, the very next step dereferences a stale or NULL node in IS_BITMAP_NODE(current) (Py_TYPE) or Py_SIZE(node) — a wild-pointer read, not merely a torn value. No concurrent structural change is needed; cursor desync alone is enough.
This is safe under the GIL, where only one thread runs the advance at a time.
That iterator backs all iteration over a contextvars.Context — iter(ctx), ctx.keys(), ctx.values(), ctx.items() — via hamt_baseiter_tp_iternext().
Per the strategy in #124397, C iterators should get the minimal changes needed so that concurrent use may return duplicate values, skip values, or raise — but must not crash. The HAMT iterator had never been brought under that hardening; it is the contextvars sibling of the already-fixed dict (#154130) and set (#144357) shared-iterator crashes.
The fix
Take a critical section on the iterator in hamt_baseiter_tp_iternext(), spanning the whole hamt_iterator_next() → *_bitmap_next / *_array_next / *_collision_next descent, so no other thread can observe the cursor half-updated. The HAMT itself is immutable, so nothing else needs locking, and the default (GIL-enabled) build is unaffected — Py_BEGIN_CRITICAL_SECTION compiles away there.
The borrowed key/value are strengthened inside the critical section and the yield function is called after releasing it, so no allocation happens under the lock — matching the setiter_iternext() idiom in Objects/setobject.c.
_PyHamt_Eq() also drives hamt_iterator_next(), but over a stack-local PyHamtIteratorState that is private to the call, so it needs no locking. The header comment on PyHamtIteratorState now records which of the two cases requires the lock.
Concurrent iteration over a shared Context iterator may still skip or repeat items. That is the documented expectation for free-threaded C iterators, and the added test asserts only that no item is invented — the same contract used by the existing tuple/list/range tests in the file.
Verification
Free-threaded build of main on Windows x64 (PCbuild\build.bat -p x64 -c Release --disable-gil), PYTHON_GIL=0.
The reproducer from the issue, before the fix — 5/5 runs die with 0xC0000005 (access violation):
After the fix, 5/5 runs complete cleanly.
The new regression test, ContendedContextIterationTest, reproduces it the same way: 5/5 crashes before the fix ("Windows fatal exception: access violation"), 8/8 passes after.
A single pass over a shared iterator turned out to be over too quickly to reliably desync the cursor, so test_restarted_shared_iterator keeps replacing the iterator as it is exhausted to hold the threads on top of each other — that is the variant that reproduces. The class also inherits the existing test_iteration / test_shared_iterator cases.
Also run green: