Skip to content

Comments

gh-145142: Make str.maketrans safe under free-threading#145157

Open
VanshAgarwal24036 wants to merge 7 commits intopython:mainfrom
VanshAgarwal24036:gh-145142-dict-next-critical-section
Open

gh-145142: Make str.maketrans safe under free-threading#145157
VanshAgarwal24036 wants to merge 7 commits intopython:mainfrom
VanshAgarwal24036:gh-145142-dict-next-critical-section

Conversation

@VanshAgarwal24036
Copy link
Contributor

@VanshAgarwal24036 VanshAgarwal24036 commented Feb 23, 2026

Replace unsafe PyDict_Next iteration with snapshot iteration using PyDict_Items to avoid crashes when the input dict is mutated concurrently under free-threading.

Adds a regression test.

Copy link
Contributor

@colesbury colesbury left a comment

Choose a reason for hiding this comment

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

A few comments below

}
PyObject *items = PyDict_Items(x);
if(items == NULL) goto err;
Py_ssize_t n = PyList_GET_SIZE(items);
Copy link
Contributor

Choose a reason for hiding this comment

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

Use a critical section here instead of copying the items to a list. You'll need to fix up the goto err. Something like:

{
    ...
    Py_BEGIN_CRITICAL_SECTION(x);
    while (PyDict_Next(x, &i, &key, &value)) {
        ...
    }
    goto done;
err:
    Py_CLEAR(new);
done:
    Py_END_CRITICAL_SECTION();
    return new;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • It fails the Windows _freeze_module build (syntax errors from macro expansion).
  • A version with early returns builds but causes test_str failures under free-threading on macOS/Linux

Is there a preferred pattern for using Py_BEGIN_CRITICAL_SECTION here, or would you prefer using the dict’s internal locking helpers instead?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants