MNT: Add linter for thread-unsafe C API uses #26159
Description
As noted in PEP 703 C API functions that return borrowed references are problematic in the nogil build.
We need to audit our usages and replace any that upgrade a borrowed reference to an owned reference with alternate C API functions that return new references.
Problematic Functions:
-
PyList_GetItem(MNT: migrate PyList_GetItem usages to PyList_GetItemRef #26246) -
PyList_GET_ITEM(all existing usages are in argument parsing) -
PyDict_GetItem -
PyDict_GetItemWithError -
PyDict_Next -
PyDict_GetItemString(ENH: fix thread-unsafe C API usages #27145 for the PyDict items). -
_PyDict_GetItemStringWithError(MNT: replace _PyDict_GetItemStringWithError with PyDict_GetItemStringRef #26282) - Some sort of linter to force us to audit new usages of problematic functions.
Activity
MNT: replace _PyDict_GetItemStringWithError with PyDict_GetItemString…
arunppsg commentedon Jun 16, 2024
@ngoldbaum Can I work on replacing usage of
PyList_GET_ITEM? I intend to replace it withPyList_GetItemRefas it returns a strong reference. Is there anything else I should look for?ngoldbaum commentedon Jun 16, 2024
So, careful, we don’t want to wholesale replace API usages. Sometimes we intentionally create borrowed references.
Any usages where in the same region of code we first create a borrowed reference and then soon after upgrade it to an owned reference by calling
Py_INCREFcan be replaced byGetItemRef. Replacing those usages (and deleting the subsequentINCREF) shouldn’t introduce new reference cou ting bugs unless we’re only doing anINCREFconditionally.Basically, be careful, because reference counting bugs can be hard to track down and spot, and we don’t want to introduce any new ones in a refactor.
ENH: fix thread-unsafe C API usages (#27145)
ENH: fix thread-unsafe C API usages (numpy#27145)
[-]MNT: Replace C API usages that return borrowed references[/-][+]MNT: Add linter for thread-unsafe C API uses[/+]ngoldbaum commentedon Oct 24, 2024
I think the remaining issue on this task is to add a linter to NumPy's CI to check PRs for new uses of possibly problematic C API items. We should double-check and explicitly whitelist all the remaining ones, then make any new uses get whitelisted as they come in.
I added the sprintable label to this since this should be doable for anyone who understands how to work with github actions and parse git diffs.
ENH: fix thread-unsafe C API usages (numpy#27145)
lvllvl commentedon Apr 3, 2025
Hi @ngoldbaum, do you mind checking out my PR when you get a chance? I was trying to add a workflow linter for the problematic functions.
Here's a link to the PR: #28634