GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-128400: Only show the current thread in `faulthandler` if the GIL is disabled by ZeroIntensity · Pull Request #128425 · python/cpython

Conversation

@ZeroIntensity

@ZeroIntensity ZeroIntensity commented Jan 2, 2025 •
edited by github-actions Bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

A few notes here:

  • Displaying other threads is useful for debugging, so some cases such as where the GIL is re-enabled or during a garbage collection (and therefore threads are stopped) still show all threads.
  • Py_FatalError still displays all threads, but that's a seperate bug--is it safe to stop-the-world during a fatal error?
  • Should this get backported?
  • Issue: Crash when using _Py_DumpTracebackThreads #128400

📚 Documentation preview 📚: https://cpython-previews--128425.org.readthedocs.build/

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
ZeroIntensity added 6 commits January 2, 2025 14:30
@ZeroIntensity
Only show the current thread while the GIL is disabled.
95424ce
@ZeroIntensity
Change the message.
8d35bb6
@ZeroIntensity
Fix tests.
aadadde
@ZeroIntensity
Fix tests.
170aa2a
@ZeroIntensity
Add documentation note.
ed23233
@ZeroIntensity
Add blurb
5622f42
@ZeroIntensity ZeroIntensity added the topic-free-threading label Jan 2, 2025
@ZeroIntensity
ZeroIntensity requested a review from colesbury January 2, 2025 20:24
@bedevere-app bedevere-app Bot mentioned this pull request Jan 2, 2025
Crash when using _Py_DumpTracebackThreads #128400
Closed
@bedevere-app bedevere-app Bot added the awaiting review label Jan 2, 2025
colesbury
colesbury reviewed Jan 2, 2025
View reviewed changes
Comment thread Modules/faulthandler.c Outdated
{
return 0;
}
if (tstate->interp->gc.collecting)

@colesbury colesbury Jan 2, 2025

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The gc.collecting field doesn't necessarily imply that all threads are paused:

  • gc.collecting is set before the stop-the-world pause
  • The GC resumes threads while calling finalizers

You could check tstate->interp->stoptheworld.world_stopped and maybe also that tstate matches stoptheworld.requester. (The faulting thread may not be attached to the interpreter when it crashes.)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@colesbury colesbury Jan 2, 2025

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not fully convinced that this is worth the extra complexity or the extra chance of crashing during the faulthandler:

  • Most crashes probably won't occur during a stop-the-world pause and for those that do, the stack traces of other threads are even less likely to be relevant (because they're paused).
  • Even during a stop-the-world pause, looping over the linked list of thread states is not safe without a HEAD_LOCK(), which you can't do during a signal handler.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@ZeroIntensity ZeroIntensity Jan 2, 2025

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I was on the fence about it too. I threw it in there because it seemed simple enough at first, but I think it's better to remove it too.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@colesbury

colesbury commented Jan 2, 2025

Copy link
Copy Markdown
Contributor

Py_FatalError still displays all threads, but that's a seperate bug--is it safe to stop-the-world during a fatal error?

I think it's best not to stop-the-world. Deadlocks that hide the fatal error message would not be fun.

ZeroIntensity reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ZeroIntensity added 2 commits January 2, 2025 16:19
@ZeroIntensity
Remove garbage collector shenanigans.
6089577
@ZeroIntensity
Be more clear in the docs.
b710acb
@ZeroIntensity

ZeroIntensity commented Jan 2, 2025

Copy link
Copy Markdown
Member Author

I'll deal with Py_FatalError in a follow-up (after this gets merged to prevent conflicts).

colesbury reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@ZeroIntensity ZeroIntensity changed the title gh-128400: Only show the current thread in faulthandler if other threads are active gh-128400: Only show the current thread in faulthandler if the GIL is disabled Jan 2, 2025
colesbury
colesbury reviewed Jan 2, 2025
View reviewed changes

@colesbury colesbury left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Some minor formatting suggestions below

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Modules/faulthandler.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Modules/faulthandler.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

ZeroIntensity and others added 2 commits January 2, 2025 17:41
@ZeroIntensity @colesbury
Update faulthandler.c
b1677f3
Co-authored-by: Sam Gross <[email protected]>
@ZeroIntensity @colesbury
Update faulthandler.c
11bb638
Co-authored-by: Sam Gross <[email protected]>
@ZeroIntensity

ZeroIntensity commented Jan 2, 2025

Copy link
Copy Markdown
Member Author

ASan failure looks unrelated.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

colesbury
colesbury approved these changes Jan 3, 2025
View reviewed changes

@colesbury colesbury left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting review labels Jan 3, 2025
@colesbury

colesbury commented Jan 3, 2025

Copy link
Copy Markdown
Contributor

Should this get backported?

I'd lean towards "no" for now

ZeroIntensity reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@colesbury
colesbury merged commit befcfdf into python:main Jan 3, 2025
@bedevere-app bedevere-app Bot removed the awaiting merge label Jan 3, 2025
@ZeroIntensity
ZeroIntensity deleted the faulthandler-nogil branch January 3, 2025 19:15
@colesbury colesbury mentioned this pull request Jan 3, 2025
gh-128002: fix many thread safety issues in asyncio #128147
Merged
WolframAlph pushed a commit to WolframAlph/cpython that referenced this pull request Jan 4, 2025
@ZeroIntensity @WolframAlph
pythongh-128400: Only show the current thread in faulthandler if th…
4b8a3c6
…e GIL is disabled (pythonGH-128425)
srinivasreddy pushed a commit to srinivasreddy/cpython that referenced this pull request Jan 8, 2025
@ZeroIntensity @srinivasreddy
pythongh-128400: Only show the current thread in faulthandler if th…
c520db5
…e GIL is disabled (pythonGH-128425)
@ZeroIntensity ZeroIntensity mentioned this pull request Jan 12, 2025
gh-128400: Only show the current thread in Py_FatalError on the free-threaded build #128758
Merged
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

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

topic-free-threading

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@ZeroIntensity @colesbury
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.