GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-116738: Use PyMutex for bz2 module by yoney · Pull Request #140555 · python/cpython

Conversation

@yoney

@yoney yoney commented Oct 24, 2025 •
edited by bedevere-app Bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

I was initially looking into the bz2 module 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 replaced PyThread_acquire_lock with PyMutex, which releases the GIL when the thread is parked. This change removes some macros and allocation handling code.

cc: @mpage @colesbury

  • Issue: Audit all built-in modules for thread safety #116738

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@yoney
pythongh-116738: Use PyMutex for bz2 module
c46e975
@bedevere-app bedevere-app Bot mentioned this pull request Oct 24, 2025
Audit all built-in modules for thread safety #116738
Open
@yoney
yoney marked this pull request as ready for review October 24, 2025 16:35
@bedevere-app bedevere-app Bot added the awaiting review label Oct 24, 2025
@kumaraditya303 kumaraditya303 added skip news topic-free-threading labels Oct 24, 2025
kumaraditya303
kumaraditya303 approved these changes Oct 24, 2025
View reviewed changes
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting review labels Oct 24, 2025
picnixz
picnixz reviewed Oct 24, 2025
View reviewed changes
Comment thread Modules/_bz2module.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
yoney added 2 commits October 24, 2025 11:39
@yoney
pythongh-116738: Keep the macros
5362112
@yoney
Revert "pythongh-116738: Keep the macros"
26760f1 
This reverts commit 5362112.
picnixz
picnixz approved these changes Oct 25, 2025
View reviewed changes

@picnixz picnixz left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@yoney

yoney commented Oct 25, 2025

Copy link
Copy Markdown
Contributor Author

@picnixz Thanks for the review! I’m glad we discovered zstd, thanks to @emmatyping.

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!
emmatyping reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
emmatyping
emmatyping reviewed Oct 25, 2025
View reviewed changes
Comment thread Modules/_bz2module.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@yoney
Assert that mutex is not locked during deallocation
43617f8
emmatyping
emmatyping approved these changes Oct 26, 2025
View reviewed changes

@emmatyping emmatyping left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Thanks! I think this looks good

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@emmatyping

emmatyping commented Oct 26, 2025

Copy link
Copy Markdown
Member

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 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
vstinner
vstinner approved these changes Oct 26, 2025
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@yoney
Add bz2 free threading test
45f313f
@yoney

yoney commented Oct 27, 2025

Copy link
Copy Markdown
Contributor Author

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

@emmatyping Thanks for pointing to the zstd tests! I had a local test for compressor, but after seeing the decompressor example in zstd, I also created a decompression test and added it to the PR. Thank you!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
colesbury
colesbury approved these changes Oct 27, 2025
View reviewed changes
@colesbury
colesbury merged commit 9479a62 into python:main Oct 27, 2025
4 checks passed

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot removed the awaiting merge label Oct 27, 2025
@yoney
yoney deleted the ft_bz2 branch October 28, 2025 15:51
@yoney yoney mentioned this pull request Oct 28, 2025
gh-116738: Use PyMutex for lzma module #140711
Merged
StanFromIreland pushed a commit to StanFromIreland/cpython that referenced this pull request Dec 6, 2025
@yoney @StanFromIreland
pythongh-116738: Use PyMutex for bz2 module (pythongh-140555)
1a3563f 
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.
This was referenced Dec 8, 2025
gh-116738: Make zlib module thread-safe #142432
Merged
gh-116738: Make _bz2 module thread-safe #142756
Merged
@IvyXu420 IvyXu420 mentioned this pull request Jul 27, 2026
gh-154707: Replace PyThread_type_lock with PyMutex in bufferedio.c #154708
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner approved these changes
@colesbury colesbury colesbury approved these changes
@emmatyping emmatyping emmatyping approved these changes
@picnixz picnixz picnixz approved these changes
@kumaraditya303 kumaraditya303 kumaraditya303 approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news topic-free-threading

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@yoney @emmatyping @vstinner @colesbury @picnixz @kumaraditya303
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.