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.

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

gh-128400: Stop-the-world when manually calling faulthandler #128422

Merged
merged 3 commits into from
Jan 2, 2025

Conversation

ZeroIntensity
Copy link
Member

@ZeroIntensity ZeroIntensity commented Jan 2, 2025

@colesbury colesbury merged commit c9356fe into python:main Jan 2, 2025
51 checks passed
@miss-islington-app
Copy link

Thanks @ZeroIntensity 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 Jan 2, 2025
…ythonGH-128422)

(cherry picked from commit c9356fe)

Co-authored-by: Peter Bierma <zintensitydev@gmail.com>
@colesbury
Copy link
Contributor

Thanks!

@bedevere-app
Copy link

bedevere-app bot commented Jan 2, 2025

GH-128423 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 Jan 2, 2025
@nascheme
Copy link
Member

nascheme commented Jan 2, 2025

I'm not sure including the test is a good idea. It looks a bit expensive to me. That code was intended to reliability trigger a crash when using git bisect. I would suspect you could reduce the loop count from 100 to 20 and still reliability trigger it. Maybe even 10 would be enough. OTOH, I don't think the SEGV happened when the dump output was sent to a file, only to the terminal. So with a unit test, when stdout is captured by default, I think that test code will not actually trigger the crash. The existing FaultHandlerTests.test_dump_traceback_threads was triggering the bug, just not very consistently. Maybe that would be good enough? Or, maybe add a couple loops there?

@nascheme
Copy link
Member

nascheme commented Jan 2, 2025

I tested by backing out the change to faulthandler.c. That test case does trigger a crash but not on every run. So, reducing the loop iterations is probably not a good idea. So, I think we can just leave that test as is.

Thanks for fixing this bug!

@ZeroIntensity
Copy link
Member Author

Ah wait, I did forget @support.requires_resource("cpu")--will the test break due to RuntimeError: can't start new thread on some buildbots?

@ZeroIntensity ZeroIntensity deleted the stop-the-world-faulthandler branch January 2, 2025 19:13
@colesbury
Copy link
Contributor

I think @threading_helper.requires_working_threading() should be sufficient. The test takes ~110 ms on my machine so I don't think it needs a @support.requires_resource("cpu").

colesbury pushed a commit that referenced this pull request Jan 2, 2025
…GH-128422) (GH-128423)

(cherry picked from commit c9356fe)

Co-authored-by: Peter Bierma <zintensitydev@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants