GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||
GeezerVison is showing github.com — gh-126316: Make grp.getgrall() thread-safe: add a mutex by vstinner · Pull Request #127055 · python/cpython Conversation
|
|
@ZeroIntensity @colesbury: Would you mind to review my change? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Thanks for the insight, Sam. How scalable is this approach (as in, will we have a static |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
I don't think we'll have |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Correct: I wrote #127093 to remove it. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Merged, thanks for reviews @colesbury and @ZeroIntensity. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
GH-127104 is a backport of this pull request to the 3.13 branch. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
…) (#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.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.