gh-117511: Make PyMutex public in the non-limited API by colesbury · Pull Request #117731 · python/cpython

colesbury · 2024-04-10T20:47:17Z

Issue: Make some PyMutex functions public #117511

📚 Direct link 📚: https://cpython-previews--117731.org.readthedocs.build/en/117731/c-api/init.html#synchronization-primitives
📚 Documentation preview 📚: https://cpython-previews--117731.org.readthedocs.build/

Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>

Update code comments to further clarify that the `_bits` field is not part of the public C API.

colesbury · 2024-06-17T19:44:28Z

@encukou @vstinner - I've updated the docs based on capi-workgroup/decisions#22 (comment).

I'm not sure of the best way to make the functions also exported as regular functions, particularly with the same name. It's easy to export them with different names and _PyMutex_LockSlow and _PyMutex_UnlockSlow are effectively that.

vstinner · 2024-06-17T19:51:45Z

I'm not sure of the best way to make the functions also exported as regular functions, particularly with the same name.

You can look at my PR which does exactly that just for Py_TYPE(): https://github.com/python/cpython/pull/120601/files

In short, add an underscore prefix to the static inline function, then use a macro to "rename" the static inline function.

vstinner · 2024-06-18T06:40:24Z

+
+#ifndef Py_LIMITED_API
+# define Py_CPYTHON_LOCK_H
+# include "cpython/lock.h"


I don't see the value of such header file. Just include cpython/lock.h in Python.h, and check Py_LIMITED_API in cpython/lock.h.

@ericsnowcurrently expressed a preference for this style when pyatomic.h was added: #109344 (comment)

IMO, it's helpful to follow a consistent pattern when it comes to the Include/cpython header files. That means in some cases we end up with very minimal header files like this in Include/.

If you mention consistency, there are already many header files in Include/cpython/ which have no companion Include/ header file:
$ grep cpython/ Include/Python.h #include "cpython/pydebug.h" #include "cpython/longintrepr.h" #include "cpython/odictobject.h" #include "cpython/funcobject.h" #include "cpython/classobject.h" #include "cpython/code.h" #include "cpython/cellobject.h" #include "cpython/initconfig.h" #include "cpython/genobject.h" #include "cpython/picklebufobject.h" #include "cpython/pytime.h" #include "cpython/context.h" #include "cpython/pyctype.h" #include "cpython/pyfpe.h" #include "cpython/tracemalloc.h" #include "cpython/optimizer.h"

vstinner · 2024-06-18T06:42:44Z

 op->ob_tid = _Py_ThreadId();
 op->_padding = 0;
- op->ob_mutex = (struct _PyMutex){ 0 };
+ op->ob_mutex = (PyMutex){ 0 };


If there is no public PyMutex_STATIC_INIT, can you maybe add a private one in pycore_lock.h?

Let's consider that in a separate PR:
It's not directly related to making the API public
It would make sense to consider that for multiple types (e.g., PyEvent, _PyOnceFlag), not just PyMutex

vstinner · 2024-06-18T06:43:57Z

+#undef PyMutex_Lock
 void
-_PyMutex_LockSlow(PyMutex *m)
+PyMutex_Lock(PyMutex *m)


You might move it to the end to still use the static inline macro in the following lines.

vstinner · 2024-06-19T07:39:37Z

+
+#ifndef Py_LIMITED_API
+# define Py_CPYTHON_LOCK_H
+# include "cpython/lock.h"


If you mention consistency, there are already many header files in Include/cpython/ which have no companion Include/ header file:
$ grep cpython/ Include/Python.h #include "cpython/pydebug.h" #include "cpython/longintrepr.h" #include "cpython/odictobject.h" #include "cpython/funcobject.h" #include "cpython/classobject.h" #include "cpython/code.h" #include "cpython/cellobject.h" #include "cpython/initconfig.h" #include "cpython/genobject.h" #include "cpython/picklebufobject.h" #include "cpython/pytime.h" #include "cpython/context.h" #include "cpython/pyctype.h" #include "cpython/pyfpe.h" #include "cpython/tracemalloc.h" #include "cpython/optimizer.h"

vstinner

LGTM. Thanks for the different update and the doc.

@colesbury got an exception from @Yhg1s (3.13 release manager) to add this API to Python 3.13, god.

miss-islington-app · 2024-06-20T15:29:11Z

Thanks @colesbury for the PR 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

miss-islington-app · 2024-06-20T15:29:16Z

Sorry, @colesbury, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 3af7263037de1d0ef63b070fc7bfc2cf042eaebe 3.13

…17731) (cherry picked from commit 3af7263)

…ythonGH-117731) (cherry picked from commit 3af7263) Co-authored-by: Sam Gross <colesbury@gmail.com>

bedevere-app · 2024-06-20T15:35:51Z

GH-120800 is a backport of this pull request to the 3.13 branch.

…) (#120800) (cherry picked from commit 3af7263)

vstinner · 2024-06-20T16:17:11Z

Yeah, congrats!

erlend-aasland · 2024-06-27T22:01:33Z

@colesbury got an exception from @Yhg1s (3.13 release manager) to add this API to Python 3.13, god.

That is very good news! Glad to see this land.

…17731)

pythongh-117511: Make PyMutex public in the non-limited API
3c743f8

bedevere-appBot mentioned this pull request Apr 10, 2024
Make some PyMutex functions public #117511
Closed

colesbury added 3 commits April 10, 2024 16:48

Add blurb
d340d28

Add missing newline
e59e0f7

Replace struct _PyMutex with PyMutex
1cef880

colesbury mentioned this pull request Apr 10, 2024
Make PyMutex public in the non-limited API capi-workgroup/decisions#22
Closed

erlend-aasland reviewed Apr 11, 2024
View reviewed changes

Comment threadDoc/c-api/init.rst Outdated
Comment threadDoc/c-api/init.rst Outdated
Comment threadDoc/c-api/init.rst Outdated
Comment threadMisc/NEWS.d/next/C API/2024-04-10-16-48-04.gh-issue-117511.RZtBRK.rst Outdated

colesburyand others added 7 commits April 11, 2024 11:53

Apply suggestions from code review
28c90bc
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>

Update Doc/c-api/init.rst
e321817
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>

Use underscore prefix for PyMutex field.
6533089
Update code comments to further clarify that the `_bits` field is not part of the public C API.

Fix comment
215020a

PyMutex should not be copied or moved
c0f75f9

Merge branch 'main' intopythongh-117511-pymutex
4ebc5e5

Update documentation
19a3492

colesbury marked this pull request as ready for review June 17, 2024 19:38

colesbury requested a review from a team as a code owner June 17, 2024 19:38

bedevere-appBot added the awaiting core review label Jun 17, 2024

colesbury requested review from encukou and vstinner June 17, 2024 19:38

Export PyMutex_Lock and PyMutex_Unlock as regular functions
7230134

vstinner reviewed Jun 17, 2024
View reviewed changes

Comment threadInclude/cpython/lock.h

vstinner reviewed Jun 18, 2024
View reviewed changes

Move PyMutex_Lock/Unlock definitions to end of file
6b2d377

colesbury added the needs backport to 3.13 bugs and security fixes label Jun 18, 2024

vstinner reviewed Jun 19, 2024
View reviewed changes

Comment threadDoc/c-api/init.rst Outdated

Update Doc/c-api/init.rst
476e493

vstinner reviewed Jun 19, 2024
View reviewed changes

vstinner approved these changes Jun 20, 2024
View reviewed changes

bedevere-appBot added awaiting merge and removed awaiting core review labels Jun 20, 2024

colesbury merged commit 3af7263 into python:mainJun 20, 2024

bedevere-appBot removed the awaiting merge label Jun 20, 2024

miss-islington-appBot assigned colesbury Jun 20, 2024

colesbury deleted the gh-117511-pymutex branch June 20, 2024 15:29

colesbury added a commit to colesbury/cpython that referenced this pull request Jun 20, 2024
pythongh-117511: Make PyMutex public in the non-limited API (python#1…
b75dff5
…17731) (cherry picked from commit 3af7263)

colesbury added a commit to colesbury/cpython that referenced this pull request Jun 20, 2024
[3.13]pythongh-117511: Make PyMutex public in the non-limited API (p…
74c5b83
…ythonGH-117731) (cherry picked from commit 3af7263) Co-authored-by: Sam Gross <colesbury@gmail.com>

colesbury added a commit to colesbury/cpython that referenced this pull request Jun 20, 2024
[3.13]pythongh-117511: Make PyMutex public in the non-limited API (p…
84a639b
…ythonGH-117731) (cherry picked from commit 3af7263) Co-authored-by: Sam Gross <colesbury@gmail.com>

bedevere-appBot removed the needs backport to 3.13 bugs and security fixes label Jun 20, 2024

colesbury added a commit that referenced this pull request Jun 20, 2024
[3.13]gh-117511: Make PyMutex public in the non-limited API (GH-117731…
3cb6c4c
…) (#120800) (cherry picked from commit 3af7263)

mrahtz pushed a commit to mrahtz/cpython that referenced this pull request Jun 30, 2024
pythongh-117511: Make PyMutex public in the non-limited API (python#1…
7c7eec3
…17731)

noahbkim pushed a commit to hudson-trading/cpython that referenced this pull request Jul 11, 2024
pythongh-117511: Make PyMutex public in the non-limited API (python#1…
81adac2
…17731)

estyxx pushed a commit to estyxx/cpython that referenced this pull request Jul 17, 2024
pythongh-117511: Make PyMutex public in the non-limited API (python#1…
f85752b
…17731)

colesbury removed their assignment Jul 19, 2024

Rogdham mentioned this pull request May 28, 2025
Add PyMutex, PyMutex_Lock, PyMutex_Unlock python/pythoncapi-compat#142
Closed

Uh oh!

Uh oh!

Conversation

colesbury commented Apr 10, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

colesbury commented Jun 17, 2024

Uh oh!

vstinner commented Jun 17, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

miss-islington-appBot commented Jun 20, 2024

Uh oh!

miss-islington-appBot commented Jun 20, 2024

Uh oh!

bedevere-appBot commented Jun 20, 2024

Uh oh!

vstinner commented Jun 20, 2024

Uh oh!

erlend-aasland commented Jun 27, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

colesbury commented Apr 10, 2024•
edited
Loading