Repository navigation
Conversation
Two threads could both see ``decl_imported`` unset, both load the declarations, and the second one overwrote the module state, leaking the first set of references. This also happens on GIL builds, since the import and the attribute lookups can release the GIL. On free-threaded builds the flag was additionally read and written without ordering, so a reader could see it set before the fields. Keep loading without a lock, but publish the result in a critical section: the first thread stores its references, a later one drops its own. Read the flag with acquire and set it with release ordering on free-threaded builds. Fixes #391. Assisted-by: Claude Opus 5.5
| raise AttributeError(name) | ||
| try: | ||
| barrier.wait() | ||
| except threading.BrokenBarrierError: |
There was a problem hiding this comment.
— Comment created by Claude
If the two threads never meet at the barrier, this except swallows the BrokenBarrierError and the test still passes. That happens when the second thread reaches the barrier more than 5 s late (for example on a loaded CI runner or under coverage), or when a later change serializes the loads. In that case the loads run one after the other, so the test passes even on unfixed code. It no longer catches regressions, and nothing reports that.
Suggestion: count successful barrier.wait() returns and fail the child if fewer than 2 threads got through. If serialized loads should be allowed, print a skip that the parent turns into self.skipTest. Then a run that never raced is reported, not counted as a pass.
| _zic_module_state* rec = _zic_state(module); | ||
|
|
||
| if (!rec->decl_imported) { | ||
| if (!_zic_load_flag(&rec->decl_imported)) { |
There was a problem hiding this comment.
— Comment created by Claude
The new acquire/release ordering assumes that once decl_imported is set, the four fields are valid. _zic_state_clear (the m_clear slot) does not keep that true: it Py_CLEARs builtin_impl_specs, empty, fallback and implements_class but leaves decl_imported at 1.
If anything calls into the module after m_clear, this fast path skips the reload and the callers use NULL fields. For example, a finalizer at interpreter shutdown or GC of a module cycle could call providedBy / implementedBy. Then PyObject_CallFunctionObjArgs(NULL, ...) in implementedByFallback or PyObject_TypeCheck(spec, NULL) crashes. This is not new, but it is the same invariant this PR now depends on. Resetting the flag in _zic_state_clear (rec->decl_imported = 0;) would make the two consistent.
| * fields stored before it (issue #391). | ||
| */ | ||
| #ifdef Py_GIL_DISABLED | ||
| #define _zic_load_flag(p) _Py_atomic_load_int_acquire(p) |
There was a problem hiding this comment.
— Comment created by Claude
_Py_atomic_load_int_acquire / _Py_atomic_store_int_release are private, underscore-prefixed CPython helpers. They are not covered by the C API stability guarantees and could be renamed or removed in a future release. That would break free-threaded builds only, and CI covers only 3.14t and 3.15t. On 3.13t they were not checked either (the PR tested 3.14t only).
Consider a short comment that states this dependency, or a capability guard with a C11 <stdatomic.h> fallback, so that a header change gives a clear error and not a confusing compile failure on a single build variant.
| } | ||
| Py_END_CRITICAL_SECTION(); | ||
|
|
||
| if (published) { |
There was a problem hiding this comment.
— Comment created by Claude
Optional simplification: the published variable and the second if block can go. Use the idiom already used in _assure_cache_field: inside the critical section, after moving ownership into rec, set the four locals to NULL. After Py_END_CRITICAL_SECTION(), call Py_XDECREF on all four unconditionally. The loser still drops its references outside the lock, and the ownership transfer is visible where it happens.
Fixes #391.
_zic_state_load_declarationscheckeddecl_importedand set it after loading, with nothing in between to stop a second thread. Two problems:NULLfields.The fix keeps the change local to that function:
Py_BEGIN_CRITICAL_SECTION(module)with no Python calls inside: the first thread stores its references and sets the flag, a later one drops its own._Py_atomic_*); elsewhere these stay plain accesses. The fast path once loaded stays lock-free.Test: a new reproducer in
test_coptimizations_declarations_import.pymakes two threads meet at athreading.Barrierinside the load (via a module__getattr__forImplements), so both are guaranteed to load concurrently, then checks that each declarations object gained exactly one reference. It is deterministic and runs on GIL builds too.Checked locally:
Py_DECREFs makes it fail again.-Wall -Wextra; pre-commit passes.This and #397 both add an entry to
CHANGES.rst; whichever is merged second gets a trivial conflict there.Assisted-by: Claude Opus 5.5