Repository navigation
cython.critical_section usability issues #7324
Description
Describe your issue
Using critical sections in Cython has some unexpected usability issues. To be clear, these issues mostly stem from how critical section work by themselves, not by how Cython has implemented them. However, because Cython sometimes hides calls to the C API, thread safety bugs where a critical section get released under one's nose become easier to miss and harder to debug. In general, we've been operating under the assumption that any call to the C API can release a critical section mainly due to two reasons:
- Any call to the C API can lead to unexpected code running which might or might not release the critical section.
- C APIs that currently do not release a critical section could be changed in the future and release it.
More specific suggestions:
- There's the issue that @ngoldbaum raised in [BUG] yielding while holding a critical section crashes 3.14t #7317 (probably more of a "bug" than a usability issue) that should be fixed by Ban yield inside critical sections #7321.
- Using
cython.critical_sectionas a decorator like it's documented in the critical section docs will always lead to a call to the C API to construct the Python object to return, which could lead to the critical section unexpectedly getting released. I think it's probably a good idea to disallow this as well. - When using
cython.critical_sectionas a context manager, would it be a good idea to warn against code that would call into the C API? We can always think of ways to disable such warnings, but, in the general case, these would be helpful.
cc @ngoldbaum @da-woods @rgommers for thoughts
Activity
da-woods commentedon Nov 13, 2025
I think what critical sections are useful for in Cython is accessing
cdefattributes ofcdef classes(and to a lesser extent,cdefmodule globals). For everything else you're probably right that Cython's a little too opaque to be able to reason effectively about where the critical section might get broken which makes them not particularly useful (especially to people who don't have the knowledge to really inspect the generated C code).I tend to disagree with this specific point - In most cases I think you'd generally be relaxed about the final conversion to the Python object - you'd typically have read and processed the data correctly in a thread-safe way and the conversion would usually be on some local temporary. Additionally, it's also pretty unlikely that the conversion function would try to acquire a critical section on the
selfargument thus causing a deadlock. (Although this last point is very much an implementation detail of critical sections, and something where things are generally less thread-safe in a GIL world).In #6766 I proposed warning on two related things that I think are big footguns - critical sections on methods of a Python (non-cdef) class are basically useless and Python attribute access (via
PyObject_GetAttr) almost always breaks the critical section.My current feeling is that adding warnings on all Python operations would be too much. But I could be wrong and am happy to hear other opinions.
I wonder if we should be promoting pymutex as the first choice rather than critical sections in the documentation?
ngoldbaum commentedon Nov 13, 2025
This is tough because without conditional compilation, any new locks will impact the GIL-enabled build. One really nice aspect of critical sections is that they're no-ops on the GIL-enabled build.
Also ping @colesbury as well.
da-woods commentedon Nov 13, 2025
My view is that's usually right - if you want it to be thread-safe in free-threading then it should probably be thread-safe in the GIL build.
ngoldbaum commentedon Nov 13, 2025
My view is more often people want the same amount of thread safety they used to have on the GIL-enabled build. That is, they don't want to deal with new kinds of bugs. Critical sections are often exactly what's needed to get that.
Adding new locks introduces new possibilities for deadlocks and new possibilities for multithreaded scaling bottlenecks.
da-woods commentedon Nov 13, 2025
Would a
pymutex_in_freethreading_only* be useful? That'd be fairly easy to add as a thing on its own without needing any improvements in user-facing conditional compilation. It wouldn't have the deadlock avoidance of critical sections but also would be easier to reason about.(* maybe with a better name...)
ngoldbaum commentedon Nov 14, 2025
I think that might help a bit, especially since
DEFandIFare discouraged.That said, I think warnings guiding people toward using critical sections safely long with some community know-how around testing code using them using explicitly multithreaded tests.