FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-119247: Add macros to use PySequence_Fast safely in free-threaded build by MojoVampire · Pull Request #119315 · python/cpython · GitHub

/ cpython Public

gh-119247: Add macros to use PySequence_Fast safely in free-threaded build - #119315

Merged
colesbury merged 9 commits into
python:mainfrom
MojoVampire:fix-issue-119247
May 22, 2024
ConversationCommits (9)ChecksFiles changed
Merged

gh-119247: Add macros to use PySequence_Fast safely in free-threaded build#119315
colesbury merged 9 commits into
python:mainfrom
MojoVampire:fix-issue-119247

Conversation

MojoVampire commented May 21, 2024 •
edited by bedevere-app Bot
Loading

Copy link
Copy Markdown
Contributor

Add Py_BEGIN_CRITICAL_SECTION_SEQUENCE_FAST and Py_END_CRITICAL_SECTION_SEQUENCE_FAST macros and update str.join to use them. Also add a regression test that would crash reliably without this patch.

MojoVampire requested a review from DinoV May 21, 2024 15:39
DinoV changed the title Fix issue 119247 gh-119247: Fix issue 119247 May 21, 2024
bedevere-app Bot mentioned this pull request May 21, 2024
Closed
DinoV requested a review from colesbury May 21, 2024 15:44
colesbury reviewed May 21, 2024
View reviewed changes

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks @MojoVampire - this looks good. I left some comments below.

Comment thread Include/internal/pycore_critical_section.h Outdated
Comment thread Include/internal/pycore_critical_section.h Outdated
Comment thread Include/internal/pycore_critical_section.h Outdated
Comment thread Include/internal/pycore_critical_section.h Outdated
Comment thread Lib/test/test_free_threading/test_str.py Outdated
Comment thread Lib/test/test_free_threading/test_str.py
colesbury changed the title gh-119247: Fix issue 119247 gh-119247: Add macros to use PySequence_Fast safely in free-threaded build May 21, 2024

colesbury commented May 21, 2024

Copy link
Copy Markdown
Contributor

I've edited the PR title and comment a bit. Feel free to edit them further, if you would like.

colesbury added the needs backport to 3.13 bugs and security fixes label May 21, 2024
MojoVampire requested a review from colesbury May 21, 2024 19:03

MojoVampire commented May 21, 2024

Copy link
Copy Markdown
Contributor Author

@colesbury: I think I've addressed everything. Thanks for the quick review!

colesbury reviewed May 21, 2024
View reviewed changes
Comment thread Lib/test/test_free_threading/test_str.py
Comment thread Lib/test/test_free_threading/test_str.py Outdated
MojoVampire requested a review from colesbury May 22, 2024 14:48
MojoVampire and others added 9 commits May 22, 2024 16:09
Initial work on critical sections for fast sequences while developing…
b22f6e2 
… test for regressions
Test for efficacy of fast sequence locking macro on str.join, both te…
2b4fe19 
…sts segfault or die with assertion failures >95% of the time without locking macro, pass reliably with locking macro
Blurb for change
536defc
Update Include/internal/pycore_critical_section.h
0475c8e 
Use braces to restrict scope of conditional locking

Co-authored-by: Sam Gross <colesbury@gmail.com>
Tweak comments per review
1f3cec0 
Co-authored-by: Sam Gross <colesbury@gmail.com>
Use braced if per review
707852d 
Co-authored-by: Sam Gross <colesbury@gmail.com>
Per review, refactor extend test to extend multiple times, and reduce…
66f0e6c 
… total time to run tests (both tests combine to less than 200 ms in debug build on my laptop)
Remove listobject.h include per review (all users of pycore_critical_…
515ee56 
…section include Python.h which provides listobject.h)
Reduce readers in test_racing_join_replace to 10, use Event to synchr…
254a5a5 
…onize, and perform multiple joins per loop to increase chance of repro without synchronization

MojoVampire commented May 22, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@DinoV & @colesbury: What needs to happen to perform the final merge? I rebased a few minutes ago to be sure it was fully up-to-date with main, not really sure what the next step is now that it's been reviewed.

colesbury commented May 22, 2024

Copy link
Copy Markdown
Contributor

I'll merge it after the CI passes

colesbury enabled auto-merge (squash) May 22, 2024 16:45
colesbury merged commit baf347d into python:main May 22, 2024

miss-islington-app Bot commented May 22, 2024

Copy link
Copy Markdown

Thanks @MojoVampire 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 May 22, 2024
pythongh-119247: Add macros to use PySequence_Fast safely in free-thr…
5860dc7 
…eaded build (pythonGH-119315)

Add `Py_BEGIN_CRITICAL_SECTION_SEQUENCE_FAST` and
`Py_END_CRITICAL_SECTION_SEQUENCE_FAST` macros and update `str.join` to use
them. Also add a regression test that would crash reliably without this
patch.
(cherry picked from commit baf347d)

Co-authored-by: Josh {*()} Rosenberg <26495692+MojoVampire@users.noreply.github.com>

bedevere-app Bot commented May 22, 2024

Copy link
Copy Markdown

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

bedevere-app Bot removed the needs backport to 3.13 bugs and security fixes label May 22, 2024
MojoVampire deleted the fix-issue-119247 branch May 22, 2024 17:46
colesbury pushed a commit that referenced this pull request May 22, 2024
[3.13] gh-119247: Add macros to use PySequence_Fast safely in free-th…
0841606 
…readed build (GH-119315) (#119419)

Add `Py_BEGIN_CRITICAL_SECTION_SEQUENCE_FAST` and
`Py_END_CRITICAL_SECTION_SEQUENCE_FAST` macros and update `str.join` to use
them. Also add a regression test that would crash reliably without this
patch.
(cherry picked from commit baf347d)

Co-authored-by: Josh {*()} Rosenberg <26495692+MojoVampire@users.noreply.github.com>

eendebakpt commented May 24, 2024

Copy link
Copy Markdown
Contributor

@MojoVampire Thanks for the PR, the Py_BEGIN_CRITICAL_SECTION_SEQUENCE_FAST is just what I needed. I have a question on the usage of Py_BEGIN_CRITICAL_SECTION_SEQUENCE_FAST : can it also be used when code inside the critical section tries to acquire a lock on the same object?

I am trying to use it to make _json thread safe, but my test case keeps locking in the free-threaded build. Usage is here: https://github.com/python/cpython/pull/119438/files#diff-efe183ae0b85e5b8d9bbbc588452dd4de80b39fd5c5174ee499ba554217a39edR1667

eendebakpt commented May 24, 2024

Copy link
Copy Markdown
Contributor

@MojoVampire Thanks for the PR, the Py_BEGIN_CRITICAL_SECTION_SEQUENCE_FAST is just what I needed. I have a question on the usage of Py_BEGIN_CRITICAL_SECTION_SEQUENCE_FAST : can it also be used when code inside the critical section tries to acquire a lock on the same object?

I am trying to use it to make _json thread safe, but my test case keeps locking in the free-threaded build. Usage is here: https://github.com/python/cpython/pull/119438/files#diff-efe183ae0b85e5b8d9bbbc588452dd4de80b39fd5c5174ee499ba554217a39edR1667

Found it! Works, but one has to take care of goto statements in the code that can jump beyond the Py_END_CRITICAL_SECTION_SEQUENCE_FAST.

JelleZijlstra mentioned this pull request May 28, 2024
Closed
estyxx pushed a commit to estyxx/cpython that referenced this pull request Jul 17, 2024
pythongh-119247: Add macros to use PySequence_Fast safely in free-thr…
36480ac 
…eaded build (python#119315)

Add `Py_BEGIN_CRITICAL_SECTION_SEQUENCE_FAST` and
`Py_END_CRITICAL_SECTION_SEQUENCE_FAST` macros and update `str.join` to use
them. Also add a regression test that would crash reliably without this
patch.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

colesbury colesbury approved these changes
DinoV Awaiting requested review from DinoV

Assignees

No one assigned

Labels

topic-free-threading

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL