gh-112075: Fix dict thread safety issues by Kronuz · Pull Request #119288 · python/cpython · GitHub
Skip to content

gh-112075: Fix dict thread safety issues - #119288

Merged
DinoV merged 2 commits into
python:mainfrom
Kronuz:dict_thread_safety
May 24, 2024
Merged

gh-112075: Fix dict thread safety issues#119288
DinoV merged 2 commits into
python:mainfrom
Kronuz:dict_thread_safety

Conversation

@Kronuz

@Kronuz Kronuz commented May 21, 2024

Copy link
Copy Markdown
Contributor

Summary:

For dicts, there are a myriad of variables that are atomics. i.e.:

mp->ma_used
mp->ma_keys
mp->ma_values
entry_ptr->me_value
entry_ptr->me_key
entry_ptr->me_hash
di->di_pos
di->len
values->values[ix]
values->capacity
values->size
keys->dk_nentries
keys->dk_usable
op->ob_ref_shared

All of these can be accessed in write or read mode. When these are being accessed in write mode, regardless of the status of the dictionary lock, the write should be an atomic store. All reads must be atomic loads if the lock is not held, or can be non-atomic reads otherwise.

This diff goes through all of the ma_used use cases and makes sure this holds true.


Test Plan:

./configure --disable-gil --with-thread-sanitizer --with-pydebug
make -j
env TSAN_OPTIONS=suppressions=Tools/tsan/suppressions_free_threading.txt ./python -m test --tsan -j4

All tests passing, no related TSAN issues.


@Kronuz
Kronuz force-pushed the dict_thread_safety branch from 22279c0 to dc2c785 Compare May 21, 2024 15:37
@Kronuz
Kronuz marked this pull request as ready for review May 21, 2024 17:37
Comment thread Objects/dictobject.c Outdated
@Kronuz
Kronuz force-pushed the dict_thread_safety branch from dc2c785 to b98504f Compare May 24, 2024 02:03
@Kronuz
Kronuz force-pushed the dict_thread_safety branch from b98504f to 2eeb71a Compare May 24, 2024 02:06

@DinoV DinoV left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM!

@DinoV
DinoV merged commit f0ed186 into python:main May 24, 2024
@colesbury

Copy link
Copy Markdown
Contributor

@colesbury colesbury added the needs backport to 3.13 bugs and security fixes label Jul 9, 2024
@miss-islington-app

Copy link
Copy Markdown

Thanks @Kronuz for the PR, and @DinoV for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 9, 2024
Fix dict thread safety issues
(cherry picked from commit f0ed186)

Co-authored-by: Germán Méndez Bravo <kronuz@fb.com>
@bedevere-app

bedevere-app Bot commented Jul 9, 2024

Copy link
Copy Markdown

GH-121545 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app Bot removed the needs backport to 3.13 bugs and security fixes label Jul 9, 2024
colesbury pushed a commit that referenced this pull request Jul 9, 2024
(cherry picked from commit f0ed186)

Co-authored-by: Germán Méndez Bravo <kronuz@fb.com>
@bedevere-bot

Copy link
Copy Markdown

estyxx pushed a commit to estyxx/cpython that referenced this pull request Jul 17, 2024
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.

4 participants