Repository navigation
Expose critical section APIs that accept a PyMutex rather than an object #67
Description
Right now extension authors can use the workaround of creating a dummy object to use exclusively with critical sections, but IMO it should be possible to use the critical section API with a PyMutex directly. This makes it easier for objects that don't subtype PyObject to use critical sections or to use critical sections in situations where a PyMutex lock might lead to deadlocks.
Proposed API
I think the internal APIs make sense to make public.
Specifically:
Py_BEGIN_CRITICAL_SECTION_MUTEX(mutex),Py_BEGIN_CRITICAL_SECTION2_MUTEX(mutex1, mutex2)
These are identical to the corresponding Py_BEGIN_CRITICAL_SECTION and Py_BEGIN_CRITICAL_SECTION2 macros (e.g. they include braces), but they accept a PyMutex instead of an object. We also need to make these API functions public:
void PyCriticalSection_BeginMutex(PyCriticalSection *c, PyMutex *mutex)void PyCriticalSection2_BeginMutex(PyCriticalSection2 *c, PyMutex *mutex1, PyMutex *mutex2)
These can be real C API functions that call the static inline functions _PyCriticalSection_BeginMutex, _PyCriticalSection2_BeginMutex.
These can be used in cases where using the macros is awkward, but should be avoided for the same reason the regular object versions should be avoided, since the macros force you to deal with managing the critical sections.
The new macros should be paired with the existing END macros (Py_END_CRITICAL_SECTION, Py_END_CRITICAL_SECTION2).
Activity
vstinner commentedon Jun 2, 2025
Can you describe the proposed API?
ngoldbaum commentedon Jun 2, 2025
I updated the description with a concrete proposal.
vstinner commentedon Jun 2, 2025
I would prefer to leave them with an underscore prefix (to keep them prívate). Or is there an use case to call them directly?
ngoldbaum commentedon Jun 2, 2025
For one: the pyo3 ffi can't wrap macros so if people implementing native extensions in Rust wanted to use this API then either those users or pyo3-ffi would need to create a dummy PyObject and use
PyCriticalSection_Begin, which is already public.Also
PyCriticalSection_BeginandPyCriticalSection2_Beginare already public so it seems odd to not make these variants public as well for the same FFI reasons.You could document the new public macros in the same way as
Py_BEGIN_CRITICAL_SECTIONandPy_BEGIN_CRITICAL_SECTION2are documented, in terms of an explicit expansion of the macro in the C API docs in terms of the lower-level constructs.encukou commentedon Jun 2, 2025
This is the way. From the WIP guidelines:
Py_BEGIN_ALLOW_THREADSis the direct precedent here.Naming-wise, I'd prefer
Py_BEGIN_CRITICAL_SECTION_MUTEX. "Mut" sounds too much like "mutable".Is there any issue with reusing the
PyCriticalSectionstruct and the END macros/functions? I suppose not; a different (or future) implementation can put a "is_mutex" flag in the struct.vstinner commentedon Jun 3, 2025
@ngoldbaum:
Oh ok, it's a good reason to make the two functions public.
@encukou:
I agree. I modified the API in the first comment. I also completed
PyCriticalSection_BeginMutex()andPyCriticalSection2_BeginMutex()API to add return type and parameters (with their types).@ngoldbaum:
_PyCriticalSection_BeginMutex() and _PyCriticalSection2_BeginMutex() are currently implemented as static inline functions. I would prefer to implement them as regular ("opaque") functions: move the implementation from the header file to a C file (
Python/critical_section.c).ngoldbaum commentedon Jun 3, 2025
Thanks! Also thanks for the name suggestion.
Agreed - C API functions are better for PyO3 too. I edited the proposal description. Feel free to edit more if you think things need more clarification.
encukou commentedon Jun 4, 2025
I clarified that these should paired with the existing END macros/functions.
IMO, this is ready for a vote now.
zooba commentedon Jun 4, 2025
I'll be honest I don't quite get the point of a critical section that doesn't actually exclude other threads from running the same code, but I don't have energy to argue about it right now.
ngoldbaum commentedon Jun 10, 2025
I've never done this before so bear with me, but I'll try to implement this proposal in a CPython PR this week. I'll follow python/cpython#119353 and compare with the current state of things in those headers.
vstinner commentedon Jun 25, 2025
Ping @serhiy-storchaka.
serhiy-storchaka commentedon Jul 21, 2025
Sorry for delay. I have no principal objections.
But I have one question. Isn't it equivalent to direct use of
PyMutex_Lock/PyMutex_Unlock, except the lack of fast path?ngoldbaum commentedon Jul 21, 2025
Not quite, it's still possible for the critical section associated with the mutex to get suspended. That's why the docs in my CPython PR emphasize that it only makes sense to use in situations where deadlocks are possible and for the same reason you'd use a regular critical section on a python object instead of a mutex. Also with the same (weak) guarantees provided by critical sections as a whole.