GV

GeezerVison

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

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-112075: Add critical sections for most dict APIs by DinoV · Pull Request #114508 · python/cpython

Conversation

@DinoV

@DinoV DinoV commented Jan 23, 2024 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

Starts adding thread safety to dict objects.

Adds critical sections around most APIs. Doesn't add any thread safety to the get-path as that'll be covered later.

There's generally 3 variations in here:

  1. Use @critical_section for APIs which are exposed via argument clinic and don't directly correlate with a public C API which needs to acquire the lock
  2. Use a _lock_held suffix for keeping changes to complicated functions simple and just wrapping them with a critical section
  3. Acquire and release the lock in an existing function where it won't be overly disruptive to the existing logic

Most things are made thread safe, I think the one glaring exception is that fetches of a single item are not thread safe. I'm hoping we can get the QSBR support in sooner rather than later and get that working. This does make iteration thread safe but it requires a lock which is not the long term plan so eventually that should become lock-free.

This specifically doesn't address the other issues for dict thread safety called out in the issue, it's just addressing the critical sections.

  • Issue: Make dict objects thread-safe in --disable-gil builds #112075

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@DinoV DinoV added topic-free-threading skip news labels Jan 23, 2024
@DinoV DinoV changed the title Nogil dict locks gh-112075: Add critical sections for most dict APIs Jan 23, 2024
@bedevere-app bedevere-app Bot mentioned this pull request Jan 23, 2024
Make dict objects thread-safe in --disable-gil builds #112075
Closed
@DinoV
DinoV force-pushed the nogil_dict_locks branch 6 times, most recently from 937afe2 to 1b5b406 Compare January 24, 2024 18:59
@DinoV
DinoV marked this pull request as ready for review January 24, 2024 20:26
@DinoV
DinoV requested review from markshannon and methane as code owners January 24, 2024 20:26
@bedevere-app bedevere-app Bot added the awaiting core review label Jan 24, 2024
@colesbury
colesbury self-requested a review January 31, 2024 20:15
colesbury
colesbury reviewed Jan 31, 2024
View reviewed changes
Comment thread Objects/dictobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/dictobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/dictobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/dictobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/dictobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/dictobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/dictobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/dictobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/dictobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@DinoV
DinoV force-pushed the nogil_dict_locks branch from 1b5b406 to 8695859 Compare February 2, 2024 18:51
@DinoV
DinoV requested a review from rhettinger as a code owner February 2, 2024 18:51
@DinoV
DinoV force-pushed the nogil_dict_locks branch 2 times, most recently from 65dede8 to 4118b1d Compare February 2, 2024 23:07
@rhettinger
rhettinger removed their request for review February 3, 2024 15:36
colesbury
colesbury reviewed Feb 5, 2024
View reviewed changes

@colesbury colesbury left a comment

Copy link
Copy Markdown
Contributor

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.

This generally looks good. I included a number of formatting suggestions. Take them or leave them as you wish.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Objects/dictobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/dictobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/dictobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/dictobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/dictobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/dictobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/dictobject.c
dictiter_iternextkey(PyObject *self)
{
dictiterobject *di = (dictiterobject *)self;
PyDictObject *d = di->di_dict;

@colesbury colesbury Feb 5, 2024

Copy link
Copy Markdown
Contributor

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.

We'll need a bit more work to make concurrent next(it) thread-safe on the same iterator. That's not quite as urgent, but let's not lose track of it.

Sorry, something went wrong.

Uh oh!

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

@DinoV DinoV Feb 5, 2024 •
edited
Loading

Uh oh!

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

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.

I actually have that ready to go I believe in what will become the PR for lcok-free thread-safe iteration, just need to get other things landed first :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
colesbury reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread Include/internal/pycore_critical_section.h Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/dictobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/dictobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@DinoV
DinoV force-pushed the nogil_dict_locks branch 2 times, most recently from c326efa to d25c9a0 Compare February 6, 2024 19:21
colesbury
colesbury approved these changes Feb 6, 2024
View reviewed changes

@colesbury colesbury left a comment

Copy link
Copy Markdown
Contributor

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
Comment thread Include/internal/pycore_critical_section.h Outdated
#ifdef Py_DEBUG

#define _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(op) \
if (Py_REFCNT(op) != 1) { \

@colesbury colesbury Feb 6, 2024

Copy link
Copy Markdown
Contributor

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.

Maybe adjust the whitespace before \

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
DinoV reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting core review labels Feb 6, 2024
DinoV added 3 commits February 6, 2024 13:39
@DinoV
Add macro to assert critical section is held
455461b
@DinoV
Critical sections
a0d1e27
@DinoV
Most dict operations should use locks
584f4cb
DinoV added 2 commits February 6, 2024 13:39
@DinoV
Lock someplaces outside of dicts & review feedback
aa80085
@DinoV
Fix formatting
ab5d224
@DinoV
DinoV force-pushed the nogil_dict_locks branch from d25c9a0 to ab5d224 Compare February 6, 2024 21:39
@DinoV
DinoV merged commit 92abb01 into python:main Feb 6, 2024
@bedevere-app bedevere-app Bot removed the awaiting merge label Feb 6, 2024
@bedevere-bot

bedevere-bot commented Feb 6, 2024

Copy link
Copy Markdown

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

Hi! The buildbot x86-64 macOS 3.x has failed when building commit 92abb01.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/366/builds/6553) and take a look at the build logs.
  4. Check if the failure is related to this commit (92abb01) or if it is a false positive.
  5. 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/all/#builders/366/builds/6553

Failed tests:

  • test.test_multiprocessing_forkserver.test_processes

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

==

Click to see traceback logs 
remote: Enumerating objects: 14, done.        
remote: Counting objects:   7% (1/13)        
remote: Counting objects:  15% (2/13)        
remote: Counting objects:  23% (3/13)        
remote: Counting objects:  30% (4/13)        
remote: Counting objects:  38% (5/13)        
remote: Counting objects:  46% (6/13)        
remote: Counting objects:  53% (7/13)        
remote: Counting objects:  61% (8/13)        
remote: Counting objects:  69% (9/13)        
remote: Counting objects:  76% (10/13)        
remote: Counting objects:  84% (11/13)        
remote: Counting objects:  92% (12/13)        
remote: Counting objects: 100% (13/13)        
remote: Counting objects: 100% (13/13), done.        
remote: Compressing objects:  25% (1/4)        
remote: Compressing objects:  50% (2/4)        
remote: Compressing objects:  75% (3/4)        
remote: Compressing objects: 100% (4/4)        
remote: Compressing objects: 100% (4/4), done.        
remote: Total 14 (delta 9), reused 10 (delta 9), pack-reused 1        
From https://github.com/python/cpython
 * branch                  main       -> FETCH_HEAD
Note: switching to '92abb0124037e5bc938fa870461a26f64c56095b'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by switching back to a branch.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -c with the switch command. Example:

  git switch -c <new-branch-name>

Or undo this operation with:

  git switch -

Turn off this advice by setting config variable advice.detachedHead to false

HEAD is now at 92abb01240 gh-112075: Add critical sections for most dict APIs (#114508)
Switched to and reset branch 'main'

configure: WARNING: pkg-config is missing. Some dependencies may not be detected correctly.

/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: libpython3.13d.a(dynamic_annotations.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: libpython3.13d.a(gc_free_threading.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: libpython3.13d.a(jit.o) has no symbols
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups
ld: warning: -undefined dynamic_lookup may not work with chained fixups

make: *** [buildbottest] Error 2
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot

bedevere-bot commented Feb 6, 2024

Copy link
Copy Markdown

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

Hi! The buildbot AMD64 Ubuntu NoGIL 3.x has failed when building commit 92abb01.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/1225/builds/1373) and take a look at the build logs.
  4. Check if the failure is related to this commit (92abb01) or if it is a false positive.
  5. 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/all/#builders/1225/builds/1373

Failed tests:

  • test_tools
  • test_math

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

==

Click to see traceback logs 
remote: Enumerating objects: 14, done.        
remote: Counting objects:   7% (1/13)        
remote: Counting objects:  15% (2/13)        
remote: Counting objects:  23% (3/13)        
remote: Counting objects:  30% (4/13)        
remote: Counting objects:  38% (5/13)        
remote: Counting objects:  46% (6/13)        
remote: Counting objects:  53% (7/13)        
remote: Counting objects:  61% (8/13)        
remote: Counting objects:  69% (9/13)        
remote: Counting objects:  76% (10/13)        
remote: Counting objects:  84% (11/13)        
remote: Counting objects:  92% (12/13)        
remote: Counting objects: 100% (13/13)        
remote: Counting objects: 100% (13/13), done.        
remote: Compressing objects:  25% (1/4)        
remote: Compressing objects:  50% (2/4)        
remote: Compressing objects:  75% (3/4)        
remote: Compressing objects: 100% (4/4)        
remote: Compressing objects: 100% (4/4), done.        
remote: Total 14 (delta 9), reused 10 (delta 9), pack-reused 1        
From https://github.com/python/cpython
 * branch                  main       -> FETCH_HEAD
Note: switching to '92abb0124037e5bc938fa870461a26f64c56095b'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by switching back to a branch.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -c with the switch command. Example:

  git switch -c <new-branch-name>

Or undo this operation with:

  git switch -

Turn off this advice by setting config variable advice.detachedHead to false

HEAD is now at 92abb01240 gh-112075: Add critical sections for most dict APIs (#114508)
Switched to and reset branch 'main'

make: *** [Makefile:2101: buildbottest] Error 2
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
fsc-eriker pushed a commit to fsc-eriker/cpython that referenced this pull request Feb 14, 2024
@DinoV @fsc-eriker
pythongh-112075: Add critical sections for most dict APIs (python#114508
cbee14f 
)

Starts adding thread safety to dict objects.


Use @critical_section for APIs which are exposed via argument clinic and don't directly correlate with a public C API which needs to acquire the lock
Use a _lock_held suffix for keeping changes to complicated functions simple and just wrapping them with a critical section
Acquire and release the lock in an existing function where it won't be overly disruptive to the existing logic
@DinoV
DinoV deleted the nogil_dict_locks branch May 31, 2024 18:23
@ZeroIntensity ZeroIntensity mentioned this pull request Apr 21, 2025
dict_set_fromkeys() calculates size of dictionary improperly #132762
Closed
@angela-tarantula angela-tarantula mentioned this pull request May 8, 2025
gh-132762: Fix underallocation bug in dict.fromkeys() and expand test coverage #133627
Merged
@Viicos Viicos mentioned this pull request Sep 9, 2026
dir() can raise RuntimeError: dictionary changed size during iteration on the free-threaded build #157217
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@colesbury colesbury colesbury approved these changes
@methane methane Awaiting requested review from methane methane is a code owner
@markshannon markshannon Awaiting requested review from markshannon markshannon is a code owner

Uh oh!

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

Assignees

No one assigned

Labels

skip news 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

@DinoV @bedevere-bot @methane @colesbury
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 01402520 — 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.