Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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.

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

Already on GitHub?Sign in to your account

Merged
colesbury merged 9 commits intopython:mainfromMojoVampire:fix-issue-119247
May 22, 2024

Conversation

MojoVampire
Copy link
Contributor

@MojoVampireMojoVampire commentedMay 21, 2024•
edited by bedevere-appbot
Loading

AddPy_BEGIN_CRITICAL_SECTION_SEQUENCE_FAST andPy_END_CRITICAL_SECTION_SEQUENCE_FAST macros and updatestr.join to use them. Also add a regression test that would crash reliably without this patch.

@MojoVampireMojoVampire requested a review fromDinoVMay 21, 2024 15:39
@DinoVDinoV changed the titleFix issue 119247gh-119247: Fix issue 119247May 21, 2024
@DinoVDinoV requested a review fromcolesburyMay 21, 2024 15:44
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 titlegh-119247: Fix issue 119247gh-119247: Add macros to use PySequence_Fast safely in free-threaded buildMay 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.

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

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

MojoVampireand others added9 commitsMay 22, 2024 16:09
…sts segfault or die with assertion failures >95% of the time without locking macro, pass reliably with locking macro
Use braces to restrict scope of conditional lockingCo-authored-by: Sam Gross <colesbury@gmail.com>
Co-authored-by: Sam Gross <colesbury@gmail.com>
Co-authored-by: Sam Gross <colesbury@gmail.com>
… total time to run tests (both tests combine to less than 200 ms in debug build on my laptop)
…section include Python.h which provides listobject.h)
…onize, and perform multiple joins per loop to increase chance of repro without synchronization
@MojoVampire
Copy link
ContributorAuthor

MojoVampire commentedMay 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

MojoVampire reacted with thumbs up emoji

@colesburycolesburyenabled auto-merge (squash)May 22, 2024 16:45
@colesburycolesbury merged commitbaf347d intopython:mainMay 22, 2024
@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 requestMay 22, 2024
…eaded build (pythonGH-119315)Add `Py_BEGIN_CRITICAL_SECTION_SEQUENCE_FAST` and`Py_END_CRITICAL_SECTION_SEQUENCE_FAST` macros and update `str.join` to usethem. Also add a regression test that would crash reliably without thispatch.(cherry picked from commitbaf347d)Co-authored-by: Josh {*()} Rosenberg <26495692+MojoVampire@users.noreply.github.com>
@bedevere-app
Copy link

GH-119419 is a backport of this pull request to the3.13 branch.

@bedevere-appbedevere-appbot removed the needs backport to 3.13bugs and security fixes labelMay 22, 2024
@MojoVampireMojoVampire deleted the fix-issue-119247 branchMay 22, 2024 17:46
colesbury pushed a commit that referenced this pull requestMay 22, 2024
…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 usethem. Also add a regression test that would crash reliably without thispatch.(cherry picked from commitbaf347d)Co-authored-by: Josh {*()} Rosenberg <26495692+MojoVampire@users.noreply.github.com>
@eendebakpt
Copy link
Contributor

@MojoVampire Thanks for the PR, thePy_BEGIN_CRITICAL_SECTION_SEQUENCE_FAST is just what I needed. I have a question on the usage ofPy_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, thePy_BEGIN_CRITICAL_SECTION_SEQUENCE_FAST is just what I needed. I have a question on the usage ofPy_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 thePy_END_CRITICAL_SECTION_SEQUENCE_FAST.

estyxx pushed a commit to estyxx/cpython that referenced this pull requestJul 17, 2024
…eaded build (python#119315)Add `Py_BEGIN_CRITICAL_SECTION_SEQUENCE_FAST` and`Py_END_CRITICAL_SECTION_SEQUENCE_FAST` macros and update `str.join` to usethem. Also add a regression test that would crash reliably without thispatch.
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
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

4 participants
@MojoVampire@colesbury@eendebakpt@DinoV
[8]ページ先頭
©2009-2025 Movatter.jp