Skip to content

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

Merged
colesbury merged 8 commits into
python:mainfrom
mpage:gh-120973-fix-local-clear
Jul 19, 2024
Merged

gh-120973: Fix thread-safety issues with threading.local#121655
colesbury merged 8 commits into
python:mainfrom
mpage:gh-120973-fix-local-clear

Conversation

@mpage

@mpage mpage commented Jul 12, 2024 •
edited by bedevere-app Bot
Loading

Copy link
Copy Markdown
Contributor

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

@ambv ambv 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.

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.

Comment thread Modules/_threadmodule.c
@colesbury colesbury added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Jul 15, 2024
@bedevere-bot

bedevere-bot commented Jul 15, 2024

Copy link
Copy Markdown

🤖 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

@colesbury colesbury 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.

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

Comment thread Modules/_threadmodule.c Outdated
Comment thread Modules/_threadmodule.c Outdated
Comment thread Modules/_threadmodule.c
Comment thread Modules/_threadmodule.c Outdated
Comment thread Modules/_threadmodule.c Outdated
Comment thread Modules/_threadmodule.c Outdated
@mpage mpage requested a review from colesbury July 17, 2024 00:00
@mpage

mpage commented Jul 17, 2024

Copy link
Copy Markdown
Contributor Author

@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

@colesbury colesbury 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

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

@colesbury

colesbury commented Jul 19, 2024

Copy link
Copy Markdown
Contributor

!buildbot AMD64 Ubuntu NoGIL Refleaks

@bedevere-bot

bedevere-bot commented Jul 19, 2024

Copy link
Copy Markdown

🤖 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
@miss-islington-app

miss-islington-app Bot commented Jul 19, 2024

Copy link
Copy Markdown

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

bedevere-app Bot commented Jul 19, 2024

Copy link
Copy Markdown

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

bedevere-bot commented Jul 19, 2024

Copy link
Copy Markdown

⚠️⚠️⚠️ 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
@efimov-mikhail efimov-mikhail mentioned this pull request Nov 1, 2025
Open
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