GeezerVison is showing github.com — Make `_warnings.c` thread-safe in free-threaded build · Issue #116664 · python/cpython
Make _warnings.c thread-safe in free-threaded build #116664DescriptionFeature or enhancement
The warnings implementation is split between Python (warnings.py) and C (_warnings.c). There are a few bits of code in the C module that are not thread-safe without the GIL:
The Py_SETREF calls are not thread-safe if concurrent writers try to overwrite the same field (e.g., st->once_registry). We can probably use critical sections to make it thread-safe.
get_once_registry:
|
Py_SETREF(st->once_registry, registry); |
get_default_action:
|
Py_SETREF(st->default_action, default_action); |
get_filter:
|
Py_SETREF(st->filters, warnings_filters); |
Some uses of borrowed references are likely not thread-safe
_PyDict_GetItemWithError (replace with PyDict_GetItemRef?)
Linked PRs
Activityerlend-aasland commented on Mar 12, 2024 I'd like to take a stab at this. Reacted by Sam Gross and Donghee Na erlend-aasland commented on Mar 13, 2024 FYI: My CPython time is very fragmented these days, so I'll move forward with a handful of smaller PRs instead of one PR that fixes everything. erlend-aasland commented on Mar 18, 2024 For the module state setref operations: AFAICS all of these happen in warn_explicit()1 helpers, so perhaps we can get away with running the warn_explicit() calls as critical sections.
Footnotes
-
the C function, not the Python API ↩
erlend-aasland commented on Mar 29, 2024 BTW, don't we need to protect _warnings._filters_mutated?
|
warnings_filters_mutated_impl(PyObject *module) |
|
/*[clinic end generated code: output=8ce517abd12b88f4 input=35ecbf08ee2491b2]*/ |
|
{ |
|
PyInterpreterState *interp = get_current_interp(); |
|
if (interp == NULL) { |
|
return NULL; |
|
} |
|
WarningsState *st = warnings_get_state(interp); |
|
if (st == NULL) { |
|
return NULL; |
|
} |
|
st->filters_version++; |
|
Py_RETURN_NONE; |
|
} |
Possibly also the already_warned(...) calls, since that function reads st->filters_version. erlend-aasland commented on Mar 29, 2024 (Reopening; #116959 introduced compilation warnings on non-free-treaded builds.) colesbury commented on Mar 29, 2024 ContributorAuthor More actions _warnings._filters_mutated: Yes, I think so. Although I think the whole way it's used in catch_warnings is not really thread-safe even with the GIL.
already_warned(...): I think the calling code already does the locking? It would be good to add an assertion that it's locked at least.
Reacted by Erlend E. Aasland erlend-aasland commented on Mar 29, 2024
_warnings._filters_mutated: Yes, I think so. Although I think the whole way it's used in catch_warnings is not really thread-safe even with the GIL.
👍 BTW, I'm a little bit sceptical about using the @critical_section Argument Clinic directive here, since that uses the module object, and not the mutex in the module state, to lock the critical section. Perhaps it does not matter.
already_warned(...): I think the calling code already does the locking? It would be good to add an assertion that it's locked at least.
Yep, an assert would be good. added a commit that references this issue on Mar 29, 2024gh-116664: Fix unused var warnings in _warnings.c in non-free-threade…
MetadataMetadataDevelopmentNo branches or pull requestsIssue actions
|