GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — gh-116738: Make _suggestions module thread-safe by yoney · Pull Request #140321 · python/cpython

Conversation

yoney commented Oct 19, 2025 •

edited by bedevere-app Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

Added a critical section to protect the candidates list in the free-threading build, addressing two possible issues below:

In the free-threading build, PyList_GetItem() could fail and return NULL. Passing it to PyUnicode_Check() results in UB in the loop below.

cpython/Modules/_suggestions.c

Lines 31 to 33 in bedaea0

	for (Py_ssize_t i = 0; i < size; ++i) {
	PyObject *elem = PyList_GetItem(candidates, i);
	if (!PyUnicode_Check(elem)) {

Another possible issue in the free-threading build was the loop below in _Py_CalculateSuggestions().

cpython/Python/suggestions.c

Lines 150 to 153 in bedaea0

	for (Py_ssize_t i = 0; i < dir_size; ++i) {
	PyObject *item = PyList_GET_ITEM(dir, i);
	if (_PyUnicode_Equal(name, item)) {
	continue;

cc: @mpage @colesbury

Note: I considered placing the critical section inside _Py_CalculateSuggestions(). However, due to the loop in _suggestions__generate_suggestions_impl(), the critical section needs to be earlier. I also checked other places where _Py_CalculateSuggestions() is called, and all of them use local list (candidates) variables in C code, so they are safe.

Issue: Audit all built-in modules for thread safety #116738

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          pythongh-116738: Make _suggestions module thread-safe

f9cef6b

bedevere-app Bot mentioned this pull request

Oct 19, 2025

Audit all built-in modules for thread safety #116738

Open


          pythongh-116738: Remove module reference from NEWS

b6937c2

yoney marked this pull request as ready for review

October 19, 2025 04:44

bedevere-app Bot added the awaiting review label

Oct 19, 2025

corona10 reviewed

Oct 19, 2025

View reviewed changes

Comment thread

Modules/_suggestions.c

		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);

corona10 Oct 19, 2025

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

yoney Oct 20, 2025

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

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:

To use PyList_GetItemRef() instead of PyList_GET_ITEM() within the current critical_section?
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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

corona10 Oct 20, 2025 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

yoney Oct 20, 2025

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

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_t i)
	{
	if (!PyList_Check(op)) {
	PyErr_BadInternalCall();
	return NULL;
	}
	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));
	return NULL;
	}
	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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

corona10 Oct 22, 2025

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

corona10 approved these changes

Oct 22, 2025

View reviewed changes

corona10 left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

lgtm!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bedevere-app Bot added awaiting merge and removed awaiting review labels

Oct 22, 2025

corona10 merged commit b3a3843 into python:main

Oct 22, 2025

4 checks passed

Uh oh!

There was an error while loading. Please reload this page.

bedevere-app Bot removed the awaiting merge label

Oct 22, 2025

bedevere-bot commented Oct 22, 2025

Copy link

Copy Markdown

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot ARM64 macOS 3.x (tier-2) has failed when building commit b3a3843.

What do you need to do:

Don't panic.
Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
Go to the page of the buildbot that failed (https://buildbot.python.org/#/builders/725/builds/12157) and take a look at the build logs.
Check if the failure is related to this commit (b3a3843) or if it is a false positive.
If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/#/builders/725/builds/12157

Failed tests:

test_urllib2net

Failed subtests:

test_ftp - test.test_urllib2net.OtherNetworkTests.test_ftp

Summary of the results of the build (if available):

Click to see traceback logs

Traceback (most recent call last):
  File "/Users/buildbot/buildarea/3.x.pablogsal-macos-m1.macos-with-brew/build/Lib/test/test_urllib2net.py", line 239, in _test_urls
    f = urlopen(url, req, support.INTERNET_TIMEOUT)
  File "/Users/buildbot/buildarea/3.x.pablogsal-macos-m1.macos-with-brew/build/Lib/test/test_urllib2net.py", line 28, in wrapped
    return _retry_thrice(func, exc, *args, **kwargs)
  File "/Users/buildbot/buildarea/3.x.pablogsal-macos-m1.macos-with-brew/build/Lib/test/test_urllib2net.py", line 24, in _retry_thrice
    raise last_exc
  File "/Users/buildbot/buildarea/3.x.pablogsal-macos-m1.macos-with-brew/build/Lib/test/test_urllib2net.py", line 20, in _retry_thrice
    return func(*args, **kwargs)
  File "/Users/buildbot/buildarea/3.x.pablogsal-macos-m1.macos-with-brew/build/Lib/urllib/request.py", line 487, in open
    response = self._open(req, data)
  File "/Users/buildbot/buildarea/3.x.pablogsal-macos-m1.macos-with-brew/build/Lib/urllib/request.py", line 504, in _open
    result = self._call_chain(self.handle_open, protocol, protocol +
                              '_open', req)
  File "/Users/buildbot/buildarea/3.x.pablogsal-macos-m1.macos-with-brew/build/Lib/urllib/request.py", line 464, in _call_chain
    result = func(*args)
  File "/Users/buildbot/buildarea/3.x.pablogsal-macos-m1.macos-with-brew/build/Lib/urllib/request.py", line 1556, in ftp_open
    raise URLError(f"ftp error: {exp}") from exp
urllib.error.URLError: <urlopen error ftp error: timed out>


Traceback (most recent call last):
  File "/Users/buildbot/buildarea/3.x.pablogsal-macos-m1.macos-with-brew/build/Lib/urllib/request.py", line 1546, in ftp_open
    fp, retrlen = fw.retrfile(file, type)
                  ~~~~~~~~~~~^^^^^^^^^^^^
  File "/Users/buildbot/buildarea/3.x.pablogsal-macos-m1.macos-with-brew/build/Lib/urllib/request.py", line 1806, in retrfile
    conn, retrlen = self.ftp.ntransfercmd(cmd)
                    ~~~~~~~~~~~~~~~~~~~~~^^^^^
  File "/Users/buildbot/buildarea/3.x.pablogsal-macos-m1.macos-with-brew/build/Lib/ftplib.py", line 354, in ntransfercmd
    conn = socket.create_connection((host, port), self.timeout,
                                    source_address=self.source_address)
  File "/Users/buildbot/buildarea/3.x.pablogsal-macos-m1.macos-with-brew/build/Lib/socket.py", line 879, in create_connection
    raise exceptions[0]
  File "/Users/buildbot/buildarea/3.x.pablogsal-macos-m1.macos-with-brew/build/Lib/socket.py", line 864, in create_connection
    sock.connect(sa)
    ~~~~~~~~~~~~^^^^
TimeoutError: [Errno 60] Operation timed out


Traceback (most recent call last):
  File "/Users/buildbot/buildarea/3.x.pablogsal-macos-m1.macos-with-brew/build/Lib/urllib/request.py", line 1546, in ftp_open
    fp, retrlen = fw.retrfile(file, type)
                  ~~~~~~~~~~~^^^^^^^^^^^^
  File "/Users/buildbot/buildarea/3.x.pablogsal-macos-m1.macos-with-brew/build/Lib/urllib/request.py", line 1806, in retrfile
    conn, retrlen = self.ftp.ntransfercmd(cmd)
                    ~~~~~~~~~~~~~~~~~~~~~^^^^^
  File "/Users/buildbot/buildarea/3.x.pablogsal-macos-m1.macos-with-brew/build/Lib/ftplib.py", line 354, in ntransfercmd
    conn = socket.create_connection((host, port), self.timeout,
                                    source_address=self.source_address)
  File "/Users/buildbot/buildarea/3.x.pablogsal-macos-m1.macos-with-brew/build/Lib/socket.py", line 879, in create_connection
    raise exceptions[0]
  File "/Users/buildbot/buildarea/3.x.pablogsal-macos-m1.macos-with-brew/build/Lib/socket.py", line 864, in create_connection
    sock.connect(sa)
    ~~~~~~~~~~~~^^^^
TimeoutError: timed out


Traceback (most recent call last):
  File "/Users/buildbot/buildarea/3.x.pablogsal-macos-m1.macos-with-brew/build/Lib/test/test_urllib2net.py", line 239, in _test_urls
    f = urlopen(url, req, support.INTERNET_TIMEOUT)
  File "/Users/buildbot/buildarea/3.x.pablogsal-macos-m1.macos-with-brew/build/Lib/test/test_urllib2net.py", line 28, in wrapped
    return _retry_thrice(func, exc, *args, **kwargs)
  File "/Users/buildbot/buildarea/3.x.pablogsal-macos-m1.macos-with-brew/build/Lib/test/test_urllib2net.py", line 24, in _retry_thrice
    raise last_exc
  File "/Users/buildbot/buildarea/3.x.pablogsal-macos-m1.macos-with-brew/build/Lib/test/test_urllib2net.py", line 20, in _retry_thrice
    return func(*args, **kwargs)
  File "/Users/buildbot/buildarea/3.x.pablogsal-macos-m1.macos-with-brew/build/Lib/urllib/request.py", line 487, in open
    response = self._open(req, data)
  File "/Users/buildbot/buildarea/3.x.pablogsal-macos-m1.macos-with-brew/build/Lib/urllib/request.py", line 504, in _open
    result = self._call_chain(self.handle_open, protocol, protocol +
                              '_open', req)
  File "/Users/buildbot/buildarea/3.x.pablogsal-macos-m1.macos-with-brew/build/Lib/urllib/request.py", line 464, in _call_chain
    result = func(*args)
  File "/Users/buildbot/buildarea/3.x.pablogsal-macos-m1.macos-with-brew/build/Lib/urllib/request.py", line 1556, in ftp_open
    raise URLError(f"ftp error: {exp}") from exp
urllib.error.URLError: <urlopen error ftp error: [Errno 60] Operation timed out>

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

yoney deleted the ft_suggestions branch

October 28, 2025 15:51

kumaraditya303 added the topic-free-threading label

Nov 20, 2025

StanFromIreland pushed a commit to StanFromIreland/cpython that referenced this pull request

Dec 6, 2025


          pythongh-116738: Make _suggestions module thread-safe (pythongh-140321)

e78b5c9

Reviewers

corona10

corona10 approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

topic-free-threading

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

yoney commented Oct 19, 2025 • edited by bedevere-app Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

corona10 Oct 19, 2025

Choose a reason for hiding this comment

Uh oh!

yoney Oct 20, 2025

Choose a reason for hiding this comment

Uh oh!

corona10 Oct 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yoney Oct 20, 2025

Choose a reason for hiding this comment

Uh oh!

corona10 Oct 22, 2025

Choose a reason for hiding this comment

Uh oh!

corona10 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bedevere-bot commented Oct 22, 2025

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

yoney commented Oct 19, 2025 •

edited by bedevere-app Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

corona10 Oct 20, 2025 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.