Skip to content

Conversation

@ngoldbaum
Copy link
Contributor

@ngoldbaum ngoldbaum commented Feb 27, 2025

Fixes #4904 (specifically the race I found here, the others have been reported upstream).

I haven't been able to see any observable impact of the data races that TSAN finds. That said, with these changes I no longer see race reports from ThreadSanitizer. Note that you need to use Python 3.14 to be able to use thread sanitizer usefully without creating a ton of suppressions, free-threaded CPython 3.13 has a number of races with fixes in 3.14 that probably won't be backported.

As soon as we can add 3.14 in CI I'll look at setting up free-threaded TSAN CI using the cpython_sanity docker image @nascheme has been working on.

I added some comments to explain the choices I made. I'm no expert in C++ atomics though so I'd appreciate some more careful eyes on this code.

I tested this by running this command in the 3.14t docker container after cloning pyo3, checking out the PR branch from #4811 and installing rustup to install a nightly toolchain and the rust-src component:

TSAN_OPTIONS="suppressions=$PWD/suppressions.txt:halt_on_error=1" LD_LIBRARY_PATH="$(pyenv prefix)/lib" LDFLAGS="-L$(pyenv prefix)/lib" RUSTFLAGS="-Zsanitizer=thread" cargo test --lib --test test_gc -Zbuild-std --target x86_64-unknown-linux-gnuTSAN_OPTIONS="suppressions=$PWD/suppressions.txt:halt_on_error=1" LD_LIBRARY_PATH="$(pyenv prefix)/lib" LDFLAGS="-L$(pyenv prefix)/lib" RUSTFLAGS="-Zsanitizer=thread" cargo test --lib --test test_gc -Zbuild-std --target x86_64-unknown-linux-gnu

I created a suppressions file for a race in CPython that's been reported upstream:

root@6221150df46c:/work/pyo3# cat suppressions.txt 
race:long_from_non_binary_base

Ping @davidhewitt.

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing, the stronger guarantees here don't look like they can hurt.

@alex
Copy link
Contributor

alex commented Feb 27, 2025

Being more conservative can't hurt, and being TSAN clean feels valuable, so this LGTM.

@ngoldbaum ngoldbaum enabled auto-merge February 27, 2025 21:46
@ngoldbaum
Copy link
Contributor Author

Thanks for the quick validation! Added to the merge queue...

@ngoldbaum ngoldbaum added this pull request to the merge queue Feb 27, 2025
Merged via the queue into PyO3:main with commit 1e2aae3 Feb 27, 2025
48 checks passed
@ngoldbaum ngoldbaum deleted the fix-thread-safety branch February 27, 2025 23:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Data races found by TSAN on free-threaded build

3 participants