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-145446: Add critical section in functools module for `PyDict_Next` by brijkapadia · Pull Request #145487 · python/cpython

Conversation

@brijkapadia

@brijkapadia brijkapadia commented Mar 3, 2026 •
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 whenever PyDict_Next is called in Module/_functoolsmodule.c in the free-threaded build.

Additionally, I had to manually remove the lock created by a critical section in case of an early return due to an error.

  • Issue: Missing critical sections for PyDict_Next calls in _functoolsmodule.c #145446

Sorry, something went wrong.

Uh oh!

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

All reactions
Brij Brij
Add critical section in functools module for
d1b94db
@brijkapadia
brijkapadia requested a review from rhettinger as a code owner March 3, 2026 23:17
@bedevere-app bedevere-app Bot added the awaiting review label Mar 3, 2026
@bedevere-app bedevere-app Bot mentioned this pull request Mar 3, 2026
Missing critical sections for PyDict_Next calls in _functoolsmodule.c #145446
Closed
blurb-it Bot and others added 2 commits March 3, 2026 23:21
@blurb-it
📜🤖 Added by blurb_it.
378493d
@brijkapadia
Update 2026-03-03-23-21-40.gh-issue-145446.0c-TJX.rst
ec87ef9
vstinner
vstinner reviewed Mar 4, 2026
View reviewed changes
Comment thread Modules/_functoolsmodule.c Resolved

Uh oh!

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

@brijkapadia
error logic change
6c786a5
vstinner
vstinner approved these changes Mar 10, 2026
View reviewed changes

@vstinner vstinner 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 bedevere-app Bot added awaiting merge and removed awaiting review labels Mar 10, 2026
colesbury
colesbury requested changes Mar 10, 2026
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.

You don't need to lock keyword arguments dictionaries. They're not exposed to other threads and cannot be modified concurrently.

This is true even if you splat a dict: foo(**mydict). The called function gets a copy of mydict.

Sorry, something went wrong.

Uh oh!

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

All reactions
@bedevere-app bedevere-app Bot added awaiting changes and removed awaiting merge labels Mar 10, 2026
@bedevere-app

bedevere-app Bot commented Mar 10, 2026

Copy link
Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

All reactions

Sorry, something went wrong.

Uh oh!

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

@brijkapadia
remove critical section for keywords
a2a0428
@brijkapadia

brijkapadia commented Mar 10, 2026

Copy link
Copy Markdown
Contributor Author

I have made the requested changes; please review again

All reactions

Sorry, something went wrong.

Uh oh!

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

@bedevere-app bedevere-app Bot added awaiting change review and removed awaiting changes labels Mar 10, 2026
@bedevere-app

bedevere-app Bot commented Mar 10, 2026

Copy link
Copy Markdown

Thanks for making the requested changes!

@vstinner, @colesbury: please review the changes made to this pull request.

All reactions

Sorry, something went wrong.

Uh oh!

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

@bedevere-app
bedevere-app Bot requested review from colesbury and vstinner March 10, 2026 22:18
@colesbury

colesbury commented Mar 10, 2026

Copy link
Copy Markdown
Contributor

I don't think the remaining critical sections are needed either.

All reactions

Sorry, something went wrong.

Uh oh!

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

@brijkapadia

brijkapadia commented Mar 10, 2026

Copy link
Copy Markdown
Contributor Author

Okay, after thinking about it I see what you mean. I can close the PR if the critical sections are not needed.

All reactions

Sorry, something went wrong.

Uh oh!

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

@brijkapadia

brijkapadia commented Mar 11, 2026

Copy link
Copy Markdown
Contributor Author

@colesbury After further reflection, I actually think that you need the critical section in the remaining cases. For example, without the critical section, this code results in a data race:

from functools import partial 
from threading import Thread

p = partial(lambda: None)

def f():
    for _ in range(100):
        repr(p)

def g():
    for i in range(100):
        p.keywords[f"{i}"] = i


t1 = Thread(target=f)
t2 = Thread(target=g)

t1.start()
t2.start()

t1.join()
t2.join()

I attached the full TSan report here although it is quite long and I don't think it is that illuminating.

However, the only remaining cases are for the kw entry of the partial object. As such, a better solution might be to transition this kw dict to a frozendict (see issue #145478).

Let me know if you think it is worth continuing this PR or if we should close it.

All reactions

Sorry, something went wrong.

Uh oh!

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

colesbury
colesbury approved these changes Mar 11, 2026
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.

Makes sense. Thanks for the example

Sorry, something went wrong.

Uh oh!

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

All reactions
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting change review labels Mar 11, 2026
@encukou
encukou merged commit 17eb035 into python:main Mar 12, 2026
91 of 95 checks passed

Uh oh!

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

@bedevere-app bedevere-app Bot removed the awaiting merge label Mar 12, 2026
@encukou encukou added awaiting merge needs backport to 3.14 bugs and security fixes labels Mar 12, 2026
@miss-islington-app

miss-islington-app Bot commented Mar 12, 2026

Copy link
Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

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

@miss-islington-app

miss-islington-app Bot commented Mar 12, 2026

Copy link
Copy Markdown

Sorry, @bkap123 and @encukou, I could not cleanly backport this to 3.14 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 17eb0354ff3110b27f811343c2d4b3c85f2685d5 3.14
All reactions

Sorry, something went wrong.

Uh oh!

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

@miss-islington-app miss-islington-app Bot assigned encukou Mar 12, 2026
@encukou

encukou commented Mar 12, 2026

Copy link
Copy Markdown
Member

Thank you!
Let's merge since a part of this can be backported to 3.14, which doesn't have frozendict.

All reactions

Sorry, something went wrong.

Uh oh!

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

@brijkapadia
brijkapadia deleted the critical branch March 12, 2026 13:49
brijkapadia added a commit to brijkapadia/cpython that referenced this pull request Mar 12, 2026
@brijkapadia
[3.14] pythongh-145446: Add critical section in functools module for …
f44781d
…`PyDict_Next` (pythonGH-145487)

(cherry picked from commit 17eb035)

Co-authored-by: bkap123 <[email protected]>
@bedevere-app

bedevere-app Bot commented Mar 12, 2026

Copy link
Copy Markdown

GH-145879 is a backport of this pull request to the 3.14 branch.

All reactions

Sorry, something went wrong.

Uh oh!

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

@bedevere-app bedevere-app Bot removed the needs backport to 3.14 bugs and security fixes label Mar 12, 2026
encukou pushed a commit that referenced this pull request Mar 13, 2026
@brijkapadia
[3.14] gh-145446: Add critical section in functools module for `PyDic…
4856992
…t_Next` (GH-145487) (GH-145879)

(cherry picked from commit 17eb035)
ljfp pushed a commit to ljfp/cpython that referenced this pull request Apr 25, 2026
@brijkapadia @ljfp
pythongh-145446: Add critical section in functools module for `PyDict…
e2a86e3
…_Next` (pythonGH-145487)
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
@rhettinger rhettinger Awaiting requested review from rhettinger rhettinger is a code owner
@vstinner vstinner Awaiting requested review from vstinner

Uh oh!

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

Assignees

@encukou encukou

Labels

awaiting merge

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

@brijkapadia @colesbury @encukou @vstinner
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.