8000 gh-120973: Fix thread-safety issues with `threading.local` by mpage · Pull Request #121655 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

gh-120973: Fix thread-safety issues with threading.local #121655

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 8 commits into from
Jul 19, 2024
Merged

gh-120973: Fix thread-safety issues with threading.local #121655

merged 8 commits into from
Jul 19, 2024

Conversation

mpage
Copy link
Contributor
@mpage mpage commented Jul 12, 2024 •
edited by bedevere-app bot
Loading

This is a small refactoring to the current design that allows us to avoid manually iterating over threads.

This should also fix gh-118490.

@mpage
Fix thread-safety issues with threading.local
8d41fbc 
This is a small refactoring to the current design that allows us to
avoid manually iterating over threads.
@mpage mpage requested a review from colesbury July 12, 2024 23:36
@bedevere-app bedevere-app bot mentioned this pull request Jul 12, 2024
Closed
@mpage mpage marked this pull request as ready for review July 13, 2024 03:20
@mpage mpage requested a review from ericsnowcurrently as a code owner July 13, 2024 03:20
ambv
ambv approved these changes Jul 15, 2024
View reviewed changes
Copy link
Contributor
@ambv ambv left a comment

Choose a reason for hiding this comment

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

Tested locally, works for me. Read through the implementation, clever trick with the sentinel. I find it just a tiny bit uglier that thread state now is concerned with thread locals, but it makes sense given the problem this PR is solving.

@colesbury colesbury added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Jul 15, 2024
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @colesbury for commit 8d41fbc 🤖

If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Jul 15, 2024
@colesbury colesbury added the needs backport to 3.13 bugs and security fixes label Jul 15, 2024
colesbury
colesbury reviewed Jul 15, 2024
View reviewed changes
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.

Overall, I think the design looks good. Some comments below.

@mpage mpage requested a review from colesbury July 17, 2024 00:00
@mpage
Copy link
Contributor Author
mpage commented Jul 17, 2024

@colesbury - I think this should be good to go. Would you please have another look?

colesbury
colesbury approved these changes Jul 17, 2024
View reviewed changes
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.

LGTM

I'll wait until today's releases finish before merging this.

@colesbury
Copy link
Contributor

!buildbot AMD64 Ubuntu NoGIL Refleaks

@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @colesbury for commit 9a7350d 🤖

The command will test the builders whose names match following regular expression: AMD64 Ubuntu NoGIL Refleaks

The builders matched are:

  • AMD64 Ubuntu NoGIL Refleaks PR

@colesbury colesbury self-assigned this Jul 19, 2024
@colesbury colesbury merged commit e059aa6 into python:main Jul 19, 2024
35 checks passed
@miss-islington-app
Copy link

Thanks @mpage for the PR, and @colesbury 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 19, 2024
@mpage @miss-islington
pythongh-120973: Fix thread-safety issues with threading.local (pyt…
1204d28 
…honGH-121655)

This is a small refactoring to the current design that allows us to
avoid manually iterating over threads.

This should also fix pythongh-118490.
(cherry picked from commit e059aa6)

Co-authored-by: mpage <mpage@meta.com>
@bedevere-app
Copy link
bedevere-app bot commented Jul 19, 2024

GH-122042 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 19, 2024
colesbury pushed a commit that referenced this pull request Jul 19, 2024
@miss-islington @mpage
[3.13] gh-120973: Fix thread-safety issues with threading.local (GH…
bf7f4f3 
…-121655) (#122042)

This is a small refactoring to the current design that allows us to
avoid manually iterating over threads.

This should also fix gh-118490.
(cherry picked from commit e059aa6)

Co-authored-by: mpage <mpage@meta.com>
@bedevere-bot
Copy link

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

Hi! The buildbot AMD64 Windows11 Non-Debug 3.x has failed when building commit e059aa6.

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/#builders/914/builds/4113) and take a look at the build logs.
  4. Check if the failure is related to this commit (e059aa6) 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/#builders/914/builds/4113

Failed tests:

  • test_int

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

==

Click to see traceback logs 
Traceback (most recent call last):
  File "b:\uildarea\3.x.ware-win11.nondebug\build\Lib\test\test_int.py", line 657, in test_denial_of_service_prevented_str_to_int
    self.assertLessEqual(sw_fail_extra_huge.seconds, sw_convert.seconds/2)
    ~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: 0.015625 not less than or equal to 0.0078125


Traceback (most recent call last):
  File "b:\uildarea\3.x.ware-win11.nondebug\build\Lib\test\test_int.py", line 646, in test_denial_of_service_prevented_str_to_int
    self.assertLessEqual(sw_fail_huge.seconds, sw_convert.seconds/2)
    ~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: 0.015625 not less than or equal to 0.0078125

@colesbury colesbury removed their assignment Jul 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ambv ambv ambv approved these changes

@colesbury colesbury colesbury approved these changes

@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently ericsnowcurrently is a code owner

Assignees
No one assigned
Labels
skip news
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

test_threading_local_clear_race segmentation fault in free-threaded CI
4 participants
@mpage @bedevere-bot @colesbury @ambv
0