Skip to content

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

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
colesbury merged 9 commits into from
May 22, 2024
Merged

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

colesbury merged 9 commits into from
May 22, 2024

Conversation

@MojoVampire
Copy link
Contributor

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

@MojoVampireMojoVampire requested a review from DinoVMay 21, 2024 15:39
@DinoVDinoV changed the title Fix issue 119247gh-119247: Fix issue 119247May 21, 2024
@bedevere-appbedevere-appbot mentioned this pull request May 21, 2024
Closed
@DinoVDinoV requested a review from colesburyMay 21, 2024 15:44
colesbury
colesbury reviewed May 21, 2024
View reviewed changes
Copy link
Contributor

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

@colesburycolesbury changed the title gh-119247: Fix issue 119247gh-119247: Add macros to use PySequence_Fast safely in free-threaded buildMay 21, 2024
@colesbury
Copy link
Contributor

colesbury commented May 21, 2024

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

@colesburycolesbury added the needs backport to 3.13 bugs and security fixes label May 21, 2024
@MojoVampire
Copy link
ContributorAuthor

MojoVampire commented May 21, 2024

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

colesbury
colesbury reviewed May 21, 2024
View reviewed changes
MojoVampireand 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
ContributorAuthor

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

colesbury commented May 22, 2024

I'll merge it after the CI passes

@colesburycolesbury enabled auto-merge (squash) May 22, 2024 16:45
@colesburycolesbury merged commit baf347d into python:mainMay 22, 2024
@miss-islington-app
Copy link

miss-islington-appbot commented May 22, 2024

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-appbot commented May 22, 2024

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

@bedevere-appbedevere-appbot removed the needs backport to 3.13 bugs and security fixes label May 22, 2024
@MojoVampireMojoVampire 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

eendebakpt commented May 24, 2024

@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

eendebakpt commented May 24, 2024

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

@JelleZijlstraJelleZijlstra 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 freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@colesburycolesburycolesbury approved these changes

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