Uh oh!
There was an error while loading. Please reload this page.
gh-116738: Use PyMutex for bz2 module - #140555
Conversation
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Thanks and sorry for the back-and-forth. A small tip: when I (and I mean me explicitly) suggest something (and don't explicitly request changes), you don't need to directly follow my advice if my suggestion was phrased as a question (that question could be addressed to a wider audience).
yoney
commented
Oct 25, 2025
@picnixz Thanks for the review! I’m glad we discovered Since the changes were quite small, I just created the extra commit, knowing we might need to revert if needed. This is just part of a good review process! |
Uh oh!
There was an error while loading. Please reload this page.
emmatyping
left a comment
There was a problem hiding this comment.
Thanks! I think this looks good
emmatyping
commented
Oct 26, 2025
Oh, perhaps we should add a test on free-threading to ensure we aren't getting data races and the locking is working? Feel free to take some inspiration from https://github.com/python/cpython/blob/main/Lib/test/test_zstd.py#L2668 |
yoney
commented
Oct 27, 2025
@emmatyping Thanks for pointing to the |
Uh oh!
There was an error while loading. Please reload this page.
The methods are already wrapped with a lock, which makes them thread-safe in free-threaded build. This replaces `PyThread_acquire_lock` with `PyMutex` and removes some macros and allocation handling code. Also add a test for free-threading to ensure we aren't getting data races and that the locking is working.
I was initially looking into the
bz2module for free-threading. The methods are already wrapped with a lock, which I believe is primarily used to release the GIL. This locking makes the methods thread-safe in free-threaded build. I replacedPyThread_acquire_lockwithPyMutex, which releases the GIL when the thread is parked. This change removes some macros and allocation handling code.cc: @mpage@colesbury