Skip to content
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

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

Merged
merged 9 commits into from
May 22, 2024
Merged

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

merged 9 commits into from
May 22, 2024

Conversation

MojoVampire
Copy link
Contributor

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

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 MojoVampire requested a review from DinoV May 21, 2024 15:39
@DinoV DinoV changed the title Fix issue 119247 gh-119247: Fix issue 119247 May 21, 2024
@bedevere-app bedevere-app bot mentioned this pull request May 21, 2024
Open
@DinoV DinoV requested a review from colesbury May 21, 2024 15:44
colesbury
colesbury reviewed May 21, 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.

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

Include/internal/pycore_critical_section.h Outdated Show resolved Hide resolved
Include/internal/pycore_critical_section.h Outdated Show resolved Hide resolved
Include/internal/pycore_critical_section.h Outdated Show resolved Hide resolved
Include/internal/pycore_critical_section.h Outdated Show resolved Hide resolved
Lib/test/test_free_threading/test_str.py Outdated Show resolved Hide resolved
Lib/test/test_free_threading/test_str.py Show resolved Hide resolved
@colesbury 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
Copy link
Contributor

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

@colesbury colesbury added the needs backport to 3.13 bugs and security fixes label May 21, 2024
@MojoVampire
Copy link
Contributor Author

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

MojoVampire and others added 9 commits May 22, 2024 16:09
@MojoVampire
Initial work on critical sections for fast sequences while developing…
b22f6e2 
… test for regressions
@MojoVampire
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
@MojoVampire
Blurb for change
536defc
@MojoVampire @colesbury
Update Include/internal/pycore_critical_section.h
0475c8e 
Use braces to restrict scope of conditional locking

Co-authored-by: Sam Gross <colesbury@gmail.com>
@MojoVampire @colesbury
Tweak comments per review
1f3cec0 
Co-authored-by: Sam Gross <colesbury@gmail.com>
@MojoVampire @colesbury
Use braced if per review
707852d 
Co-authored-by: Sam Gross <colesbury@gmail.com>
@MojoVampire
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)
@MojoVampire
Remove listobject.h include per review (all users of pycore_critical_…
515ee56 
…section include Python.h which provides listobject.h)
@MojoVampire
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
Copy link
Contributor Author

MojoVampire commented May 22, 2024 •
edited
Loading

@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
Copy link
Contributor

I'll merge it after the CI passes

@colesbury colesbury enabled auto-merge (squash) May 22, 2024 16:45
@colesbury colesbury merged commit baf347d into python:main May 22, 2024
36 checks passed
@miss-islington-app
Copy link

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
@MojoVampire @miss-islington
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
Copy link

bedevere-app bot commented May 22, 2024

GH-119419 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 May 22, 2024
@MojoVampire MojoVampire deleted the fix-issue-119247 branch May 22, 2024 17:46
colesbury pushed a commit that referenced this pull request May 22, 2024
@miss-islington @MojoVampire
[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
Copy link
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
Copy link
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 JelleZijlstra mentioned this pull request May 28, 2024
Closed
estyxx pushed a commit to estyxx/cpython that referenced this pull request Jul 17, 2024
@MojoVampire @estyxx
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.
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

@DinoV 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
@MojoVampire @colesbury @eendebakpt @DinoV