`test_super_traverse_early_return_does_not_abort` is flaky on free-threaded builds (~30% of runs)
### Bug Description
`test_super_traverse_early_return_does_not_abort` ([`tests/test_gc.rs:829`](https://github.com/PyO3/pyo3/blob/main/tests/test_gc.rs#L829), added in #6206) fails intermittently on the free-threaded CI job:
```
thread 'test_super_traverse_early_return_does_not_abort' panicked at tests/test_gc.rs:850:9:
child __traverse__ ran despite the super-type traverse returning non-zero
```
It reproduces on `main` at 1ed13a0fd, without any local modification:
| build | failure rate |
| --- | --- |
| free-threaded 3.15.0b4, `--features=abi3t,full,multiple-pymethods --no-default-features` | **12 / 40 runs** |
| GIL-enabled 3.14.6, `--features=full,multiple-pymethods` | 0 / 40 runs |
This is not a rare flake.
### Steps to Reproduce
```bash
uv python install 3.15t
export PYO3_PYTHON=$(uv python find 3.15t)
cargo test --no-fail-fast --features=abi3t,full,multiple-pymethods --no-default-features --test test_gc
# re-run a few times, or narrow to the minimal pair:
# <test binary> --exact test_super_traverse_early_return_does_not_abort dict_cycle_collected_without_traverse
```
### Backtrace
The panic itself, from a local run on `main` (`RUST_BACKTRACE=1`) — frame for frame the same as CI:
```
thread 'test_super_traverse_early_return_does_not_abort' (1469631) panicked at tests/test_gc.rs:850:9:
child __traverse__ ran despite the super-type traverse returning non-zero
stack backtrace:
0: __rustc::rust_begin_unwind
at /rustc/ac68faa20c58cbccd01ee7208bf3b6e93a7d7f96/library/std/src/panicking.rs:689:5
1: core::panicking::panic_fmt
at /rustc/ac68faa20c58cbccd01ee7208bf3b6e93a7d7f96/library/core/src/panicking.rs:80:14
2: test_gc::test_super_traverse_early_return_does_not_abort::{{closure}}
at ./tests/test_gc.rs:850:9
3: pyo3::marker::Python::attach
at ./src/marker.rs:419:9
4: test_gc::test_super_traverse_early_return_does_not_abort
at ./tests/test_gc.rs:830:5
5: test_gc::test_super_traverse_early_return_does_not_abort::{{closure}}
at ./tests/test_gc.rs:829:53
```
That backtrace only shows the assertion site, not the cause — the traversal that set the flag happened earlier, on a different thread. Instrumenting `TraverseChild::__traverse__` to dump a backtrace whenever it runs (and re-running with `--nocapture`, since the output otherwise belongs to the other test and gets swallowed) captures the actual chain:
```
>>> TraverseChild::__traverse__ ran on thread Some("test_cycle_clear")
0: test_gc::TraverseChild::__traverse__
at ./tests/test_gc.rs:827:13
1: pyo3::impl_::pymethods::traverse_impl::{{closure}}
at ./src/impl_/pymethods.rs:441:49
...
7: pyo3::impl_::pymethods::traverse_impl
at ./src/impl_/pymethods.rs:441:11
8: pyo3::impl_::pymethods::_call_traverse
at ./src/impl_/pymethods.rs:372:27
9: test_gc::TraverseChild::__pymethod_traverse__
at ./tests/test_gc.rs:819:1
10: update_refs
11: mi_heap_visit_blocks
12: gc_visit_heaps
13: gc_collect_main
14: gc_collect
...
17: PyEval_EvalCode
18: <pyo3::instance::Bound<PyCode> as PyCodeMethods>::run
at ./src/types/code.rs:133:13
19: pyo3::marker::Python::run
at ./src/marker.rs:643:14
20: test_gc::DropCheck::assert_drops_with_gc::{{closure}}
at ./tests/test_gc.rs:107:20
```
Read bottom-up: `assert_drops_with_gc` runs `gc.collect()` on the **`test_cycle_clear`** thread
(frames 20-14), the free-threaded collector's `update_refs` phase visits every tracked object
(frames 13-10), reaches our object and runs `traverse_impl` (frames 9-7). `update_refs` visits with
a `visitproc` that always returns 0, so the super-traverse does not return non-zero, nothing returns
early, and the child's `__traverse__` body runs (frames 1-0) and sets the global flag.
Two such interfering traversals were recorded in the failing run.
### Your operating system and version
Linux (CachyOS, kernel 7.1.3)
### Your Python version (`python --version`)
3.15.0b4 free-threading build (fails) / 3.14.6 GIL-enabled (passes)
### Your Rust version (`rustc --version`)
rustc 1.96.0
### Your PyO3 version
0.29
### How did you install python? Did you use a virtualenv?
uv
### Additional Info
## Root Cause
The test asserts on a **process-global** `AtomicBool` that **any** traversal of the object sets:
```rust
static CHILD_TRAVERSED: AtomicBool = AtomicBool::new(false);
impl TraverseChild {
fn __traverse__(&self, _visit: PyVisit<'_>) -> Result<(), PyTraverseError> {
CHILD_TRAVERSED.store(true, Ordering::SeqCst);
Ok(())
}
}
```
`traverse_impl` ([`src/impl_/pymethods.rs:397`](https://github.com/PyO3/pyo3/blob/main/src/impl_/pymethods.rs#L397)) only skips the child's `__traverse__` when the super-type traverse returns non-zero:
```rust
let super_retval = unsafe { call_super_traverse(slf, visit, arg, current_traverse) };
if super_retval != 0 {
return super_retval;
}
// ... eventually calls impl_(), i.e. TraverseChild::__traverse__
```
Only `gc.get_referrers()`'s `referrersvisit` returns non-zero. **Every ordinary collection** visits
with `visit_decref`, which always returns 0, so a plain `gc.collect()` runs the child's
`__traverse__` to completion and sets the flag.
The test therefore asserts "no garbage collection touched this object between the `store(false)` and
the assertion" — which nothing in the test guarantees.
### Proof of the mechanism
Inserting one collection into the window makes it fail deterministically, on a **GIL** build, running
the test **alone**:
```rust
CHILD_TRAVERSED.store(false, Ordering::SeqCst);
py.import("gc").unwrap().call_method0("collect").unwrap(); // added
```
```
EXPERIMENT: after a plain gc.collect(), CHILD_TRAVERSED = true
panicked at tests/test_gc.rs:854:9: child __traverse__ ran despite ...
```
### Why free-threaded specifically
The collections come from *other tests in the same binary*. `assert_drops_with_gc`
([`tests/test_gc.rs:99`](https://github.com/PyO3/pyo3/blob/main/tests/test_gc.rs#L99)) runs up to 100
collections in a loop, and on the free-threaded build sleeps 5 ms per iteration — which stretches
those tests out to as much as 500 ms and widens the overlap:
```rust
for _ in 0..100 {
if self.0.is_completed() { return; }
Python::attach(|py| { py.run(c"import gc; gc.collect()", None, None).unwrap(); });
#[cfg(Py_GIL_DISABLED)]
std::thread::sleep(std::time::Duration::from_millis(5));
}
```
Nine tests in `test_gc.rs` call it. On a GIL build they cannot run Python concurrently with the
window, because `Python::attach` holds the GIL across the whole closure; on the free-threaded build
they run genuinely in parallel.
Isolating it confirms this exactly (free-threaded, 3.15.0b4):
| selection | failure rate |
| --- | --- |
| `test_super_traverse_early_return_does_not_abort` alone | 0 / 20 |
| that test **+** `dict_cycle_collected_without_traverse` | **15 / 20** |
Two tests are sufficient to reproduce it.
## Suggested Fix
Don't involve the garbage collector at all. `test_gc.rs` already has the pattern for this:
`unsendable_are_not_traversed_on_foreign_thread` ([`tests/test_gc.rs:569`](https://github.com/PyO3/pyo3/blob/main/tests/test_gc.rs#L569)) also asserts a *negative* about traversal, and does it by fetching `tp_traverse` with the file's `get_type_traverse` helper and calling it directly with its own `visitproc`. The file also already has `visit_error`, a `visitproc` returning non-zero — exactly the halt-early behaviour `gc.get_referrers()` was standing in
for.
Doing the same here makes the test deterministic, and lets it additionally assert the return value — which the current version never checks:
```rust
#[pyclass(extends = TraverseBase)]
struct TraverseChild {
// Set by `__traverse__` so the test can assert whether the child's own traverse body ran.
traversed: AtomicBool,
}
```
```rust
// The traverse is invoked directly rather than through `gc.get_referrers()`: a real collection
// visits with a `visitproc` which returns zero, so it runs the child traverse in full, and any
// concurrently running test may trigger one.
let traverse = unsafe { get_type_traverse(child.get_type().as_type_ptr()).unwrap() };
assert_ne!(
unsafe { traverse(child.as_ptr(), visit_error, std::ptr::null_mut()) },
0,
"the non-zero from the super-type traverse should be propagated"
);
assert!(
!child.borrow().traversed.load(Ordering::SeqCst),
"child __traverse__ ran despite the super-type traverse returning non-zero"
);
```
The global `static CHILD_TRAVERSED` goes away entirely. A per-instance `AtomicBool` field is what the other traverse tests in this file already use (`gc_during_borrow`, `traverse_cannot_be_hijacked`), and the global was the only one of its kind in the file.
Verified on free-threaded 3.15.0b4:
| | before | after |
| --- | --- | --- |
| full `test_gc` suite | 12 failures / 40 runs | **0 failures / 60 runs** |
| the two-test pair | 15 failures / 20 runs | **0 failures / 40 runs** |
It does not weaken the test — it strengthens it. Mutating `traverse_impl` to drop its early return (`if false && super_retval != 0`) still fails, and now fails on the return-value assertion, which is one step closer to the actual defect:
```
panicked at tests/test_gc.rs:848:9:
assertion `left != right` failed: the non-zero from the super-type traverse should be propagated
```
The original abort regression is still covered: an early return from `traverse_impl` with the `PanicTrap` still armed aborts the process on this path just the same, whether the non-zero comes from `referrersvisit` or from `visit_error`.
### Alternative considered
Making the flag thread-local (`thread_local! { static CHILD_TRAVERSED: Cell<bool> }`) also takes the
failures to 0/60, because the interfering collections run on other tests' threads. But it keeps the
test coupled to GC timing and leaves a narrow same-thread window open (an *automatic* collection
triggered by the allocations inside `py.import("gc")` / `call_method1`). Calling `tp_traverse`
directly removes the dependency instead of filtering it, so that is the better fix.
1 条评论