Expose critical section APIs that accept a PyMutex rather than an object

[ngoldbaum]

c.f. python/cpython#133296

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

[vstinner]

Can you describe the proposed API?

[ngoldbaum]

I updated the description with a concrete proposal.

[vstinner]

We also need to make these API functions public

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]

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_Begin and PyCriticalSection2_Begin are 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_SECTION and Py_BEGIN_CRITICAL_SECTION2 are documented, in terms of an explicit expansion of the macro in the C API docs in terms of the lower-level constructs.

[encukou]

You could document the new public macros in the same way as Py_BEGIN_CRITICAL_SECTION and Py_BEGIN_CRITICAL_SECTION2 are documented, in terms of an explicit expansion of the macro in the C API docs in terms of the lower-level constructs.

This is the way. From the WIP guidelines:

Macros can be used in the following cases: [...] Shortcuts for functionality that can be accomplished trivially, but perhaps tediously, without macros (e.g. Py_VISIT, Py_BEGIN_ALLOW_THREADS, Py_RETURN_RICHCOMPARE). In this case, the macro-less equivalent should be clear from documentation: consider adding the macro's expansion to the docs. Non-C wrappers are expected re-implement these macros.

Py_BEGIN_ALLOW_THREADS is 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 PyCriticalSection struct and the END macros/functions? I suppose not; a different (or future) implementation can put a "is_mutex" flag in the struct.

[vstinner]

@ngoldbaum:

For one: the pyo3 ffi can't wrap macros so if people implementing native extensions in Rust wanted to use this API

Oh ok, it's a good reason to make the two functions public.

@encukou:

Naming-wise, I'd prefer Py_BEGIN_CRITICAL_SECTION_MUTEX. "Mut" sounds too much like "mutable".

I agree. I modified the API in the first comment. I also completed PyCriticalSection_BeginMutex() and PyCriticalSection2_BeginMutex() API to add return type and parameters (with their types).

@ngoldbaum:

These can just be _PyCriticalSection_BeginMutex, _PyCriticalSection2_BeginMutex with the underscores removed and moved to cpython/critical_section.h as well.

_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]

API to add return type and parameters (with their types).

Thanks! Also thanks for the name suggestion.

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

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]

I clarified that these should paired with the existing END macros/functions.

IMO, this is ready for a vote now.

• @encukou
• @erlend-aasland
• @mdboom
• @serhiy-storchaka
• @vstinner
• @zooba

[zooba]

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]

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]

Ping @serhiy-storchaka.

[serhiy-storchaka]

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]

But I have one question. Isn't it equivalent to direct use of PyMutex_Lock/PyMutex_Unlock, except the lack of fast path?

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.