Skip to content

gh-116738: Make _suggestions module thread-safe#140321

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
corona10 merged 2 commits into from
Oct 22, 2025
Merged

gh-116738: Make _suggestions module thread-safe #140321

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f9cef6b
gh-116738: Make _suggestions module thread-safe
yoney Oct 19, 2025
b6937c2
gh-116738: Remove module reference from NEWS
yoney Oct 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions Lib/test/test_free_threading/test_suggestions.py
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
import unittest

from test.support import import_helper, threading_helper
from test.support.threading_helper import run_concurrently

suggestions = import_helper.import_module("_suggestions")

NTHREADS = 10


@threading_helper.requires_working_threading()
class SuggestionsTests(unittest.TestCase):
def test_generate_suggestions(self):
candidates = [str(i) for i in range(100)]

def worker():
_ = suggestions._generate_suggestions(candidates, "42")
candidates.clear()

run_concurrently(worker_func=worker, nthreads=NTHREADS)


if __name__ == "__main__":
unittest.main()
2 changes: 2 additions & 0 deletions Misc/NEWS.d/next/Core_and_Builtins/2025-10-18-19-52-20.gh-issue-116738.NLJW0L.rst
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
Make _suggestions module thread-safe on the :term:`free threaded <free
threading>` build.
5 changes: 3 additions & 2 deletions Modules/_suggestions.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -8,6 +8,7 @@ module _suggestions
/*[clinic end generated code: output=da39a3ee5e6b4b0d input=e58d81fafad5637b]*/

/*[clinic input]
@critical_section candidates
_suggestions._generate_suggestions
candidates: object
item: unicode
Expand All@@ -18,7 +19,7 @@ Returns the candidate in candidates that's closest to item
static PyObject *
_suggestions__generate_suggestions_impl(PyObject *module,
PyObject *candidates, PyObject *item)
/*[clinic end generated code: output=79be7b653ae5e7ca input=ba2a8dddc654e33a]*/
/*[clinic end generated code: output=79be7b653ae5e7ca input=92861a6c9bd8f667]*/
{
// Check if dir is a list
if (!PyList_CheckExact(candidates)){
Expand All@@ -29,7 +30,7 @@ _suggestions__generate_suggestions_impl(PyObject *module,
// Check if all elements in the list are Unicode
Py_ssize_t size = PyList_Size(candidates);
for (Py_ssize_t i = 0; i < size; ++i){
PyObject *elem = PyList_GetItem(candidates, i);
PyObject *elem = PyList_GET_ITEM(candidates, i);
Copy link
Member

@corona10corona10Oct 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about using PyList_GetItemRef with this change.

Copy link
ContributorAuthor

@yoneyyoneyOct 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@corona10, Thank you so much for your comment!

Just to make sure I understand your suggestion correctly, did you mean:

  1. To use PyList_GetItemRef() instead of PyList_GET_ITEM() within the current critical_section?
  2. Or to remove the critical_section and use PyList_GetItemRef() in both loops I mentioned in the description?

Regarding 1: I replaced PyList_GetItem() with PyList_GET_ITEM() because, in the with-GIL build, it’s already assumed that PyList_GetItem() cannot fail, so the returned value isn't checked. Within the critical_section, the assumption is that the candidates list cannot be mutated, and PyList_GET_ITEM() avoids extra atomic operations.

Regarding 2: I think this could be implemented with PyList_GetItemRef() and no critical_section, as long as errors returned from PyList_GetItemRef() are properly handled. However, in my opinion, using a critical_section makes the code a bit easier to read and reason about, since the candidates list cannot be mutated in this context. This means we don’t need to consider update cases between blocks or iterations.

What do you think, would it be better to use PyList_GetItemRef() here?

Copy link
Member

@corona10corona10Oct 20, 2025•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yoney
Hmm, even if you use PyList_GET_ITEM, it won’t check whether the returned value is NULL, so it won’t actually fix the UB you mentioned. What do you think?

cpython/Include/cpython/listobject.h

Line 40 in f11ec6e

#definePyList_GET_ITEM(op, index) (_PyList_CAST(op)->ob_item[(index)])

Copy link
ContributorAuthor

@yoneyyoneyOct 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@corona10
Sorry, my previous comment wasn't clear before. I was referring to cases where PyList_GetItem() could fail due to PyList_Check() or valid_index(), particularly in the free-threading build. I was trying to explain why we need a critical_section.

cpython/Objects/listobject.c

Lines 384 to 397 in f11ec6e

PyObject*
PyList_GetItem(PyObject*op, Py_ssize_ti)
{
if (!PyList_Check(op)){
PyErr_BadInternalCall();
returnNULL;
}
if (!valid_index(i, Py_SIZE(op))){
_Py_DECLARE_STR(list_err, "list index out of range");
PyErr_SetObject(PyExc_IndexError, &_Py_STR(list_err));
returnNULL;
}
return ((PyListObject*)op) ->ob_item[i];
}

These properties are checked in _suggestions__generate_suggestions_impl() earlier, before calling PyList_GetItem(), and the GIL or the critical_section "protects" them. That's why I replaced PyList_GetItem() with PyList_GET_ITEM(). However, the actual element of the list could still be NULL. I had assumed this wasn’t possible, but should we check if the element is NULL?

Copy link
Member

@corona10corona10Oct 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now I get it, looks good to me. Thank you for explain

if (!PyUnicode_Check(elem)){
PyErr_SetString(PyExc_TypeError, "all elements in 'candidates' must be strings");
return NULL;
Expand Down
5 changes: 4 additions & 1 deletion Modules/clinic/_suggestions.c.h
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading