gh-126316: Make grp.getgrall() thread-safe: add a mutex#127055
gh-126316: Make grp.getgrall() thread-safe: add a mutex#127055vstinner merged 4 commits intopython:mainfrom
Conversation
grpmodule.c is no longer built with the limited C API, since PyMutex is excluded from the limited C API.
|
@ZeroIntensity @colesbury: Would you mind to review my change? |
ZeroIntensity
left a comment
ZeroIntensity
left a comment
There was a problem hiding this comment.
Hmm, this is an interesting way to do it. Local static variables are notorious for being non-thread-safe, so I'm not sure this kind of approach will work. If it does, I don't think we're fixing any issues with re-entrancy.
I would just go with @critical_section on each function, and then switch the module's Py_mod_multiple_interpreters to Py_MOD_MULTIPLE_INTERPRETERS_SUPPORTED instead of Py_MOD_PER_INTERPRETER_GIL_SUPPORTED to denote shared-GIL for subinterpreters. That should fix races for both cases.
colesbury
left a comment
colesbury
left a comment
There was a problem hiding this comment.
This approach looks good to me. I think you are right that we don't have to worry about deadlock here because the calls inside the lock won't call arbitrary Python code.
There's a failing CI related to generated files. I think the pycore_lock.h include in pycore_modsupport.h can be removed now. Also, maybe the arg clinic files need to be regenerated?
Local static variables are notorious for being non-thread-safe
A static PyMutex is fine because PyMutex is thread-safe. (It doesn't matter if it's a local or global static variable.)
The general problems with static variables (including static local variables) is that the data structures are not thread-safe or that they are lazily initialized pointers, which is not the case here.
|
Thanks for the insight, Sam. How scalable is this approach (as in, will we have a static |
|
I don't think we'll have |
Correct: I wrote #127093 to remove it. |
|
Merged, thanks for reviews @colesbury and @ZeroIntensity. |
|
GH-127104 is a backport of this pull request to the 3.13 branch. |
…) (#127104) * gh-126316: Make grp.getgrall() thread-safe: add a mutex (#127055) grpmodule.c is no longer built with the limited C API, since PyMutex is excluded from the limited C API. (cherry picked from commit 3c2bd66) * Revert ABI changes Don't use Argument Clinic for grp.getgrgid() to avoid changing the ABI (change PyInterpreterState structure by adding an "id" identifier).
…#127055) grpmodule.c is no longer built with the limited C API, since PyMutex is excluded from the limited C API.
3 participants
grpmodule.c is no longer built with the limited C API, since PyMutex is excluded from the limited C API.
grpis not thread safe #126316