KEMBAR78
gh-134745: Change PyThread_allocate_lock() implementation to PyMutex by vstinner · Pull Request #134747 · python/cpython · GitHub
Skip to content

gh-134745: Change PyThread_allocate_lock() implementation to PyMutex #134747

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 7 commits into from
May 30, 2025
Merged

gh-134745: Change PyThread_allocate_lock() implementation to PyMutex #134747

merged 7 commits into from
May 30, 2025

Conversation

@vstinner
Copy link
Member

@vstinner vstinner commented May 26, 2025 •
edited by colesbury
Loading

PyMutex is a fast and portable lock currently only used internally by Python. This change modifies PyThread_allocate_lock to reuse PyMutex.

@bedevere-app bedevere-app bot mentioned this pull request May 26, 2025
Closed
@vstinner
Copy link
Member Author

This draft PR currently only modify Python/thread_pthread.h. I left Python/thread_nt.h unchanged.

_PyThread_at_fork_reinit() is still implemented by allocating a new lock and leaking the old lock.

@vstinner
Copy link
Member Author

@colesbury: What do you think of this idea? Does it make sense? Do you see any possible issue?

If the approach makes sense, I will update my PR to modify also the Windows implementation.

@colesbury
Copy link
Contributor

I think the idea is fine, but the PR title and description are confusing. threading.Lock / _thread.lock / _thread._thread.allocate_lock() already use PyMutex (all three are the same thing):

cpython/Modules/_threadmodule.c

Lines 757 to 760 in f6f4e8a

typedef struct {
PyObject_HEAD
PyMutex lock;
} lockobject;

This PR changes the implementation of the C API PyThread_allocate_lock(). We are generally using PyMutex directly instead of PyThread_allocate_lock() in new code, and we've also changed a lot of existing uses too. Using PyMutex directly is simpler because it avoids an allocation that may fail.

I think it's okay to change the implementation of PyThread_allocate_lock(). It looks like it's going to simplify the code, but there's some risk of unknown behavioral changes.

@vstinner vstinner changed the title gh-134745: Change threading.Lock implementation to PyMutex gh-134745: Change PyThread_allocate_lock() implementation to PyMutex May 27, 2025
colesbury
colesbury reviewed May 27, 2025
View reviewed changes
vstinner added 2 commits May 28, 2025 12:28
@vstinner
Address Sam's review
b754673
@vstinner
Add _PY_FAIL_IF_INTERRUPTED flag
f734745 
Don't call signal handlers if interrupted, only return PY_LOCK_INTR.
@vstinner vstinner marked this pull request as ready for review May 28, 2025 10:54
@vstinner vstinner mentioned this pull request May 29, 2025
Closed
colesbury
colesbury approved these changes May 29, 2025
View reviewed changes
Copy link
Contributor

@colesbury colesbury left a comment

Choose a reason for hiding this comment

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

This LGTM, but I think you should include a NEWS entry. One behavioral change is that previously intr_flag was ignored on Windows, but now it actually makes the acquisitions interruptible.

vstinner and others added 2 commits May 30, 2025 11:29
@vstinner @colesbury
Update Python/thread.c
b1fbf6c 
Co-authored-by: Sam Gross <colesbury@gmail.com>
@vstinner
Add NEWS entry
475cfb3
@vstinner vstinner enabled auto-merge (squash) May 30, 2025 09:42
@vstinner vstinner disabled auto-merge May 30, 2025 09:44
@vstinner vstinner enabled auto-merge (squash) May 30, 2025 09:45
@vstinner vstinner merged commit ebf6d13 into python:main May 30, 2025
39 checks passed
@vstinner vstinner deleted the pymutex branch May 30, 2025 10:15
@vstinner
Copy link
Member Author

Merged, thanks for the review @colesbury.

@encukou encukou mentioned this pull request Jun 3, 2025
Closed
Pranjal095 pushed a commit to Pranjal095/cpython that referenced this pull request Jul 12, 2025
@vstinner @colesbury @Pranjal095
pythongh-134745: Change PyThread_allocate_lock() implementation to Py…
3d5b6a8 
…Mutex (python#134747)

Co-authored-by: Sam Gross <colesbury@gmail.com>
@kumaraditya303 kumaraditya303 mentioned this pull request Jul 25, 2025
Closed
taegyunkim pushed a commit to taegyunkim/cpython that referenced this pull request Aug 4, 2025
@vstinner @colesbury @taegyunkim
pythongh-134745: Change PyThread_allocate_lock() implementation to Py…
ceb9b09 
…Mutex (python#134747)

Co-authored-by: Sam Gross <colesbury@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@colesbury colesbury colesbury approved these changes

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@vstinner @colesbury