Skip to content

PEP 788: Add some minor clarifications #4391

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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

ZeroIntensity
Copy link
Member

@ZeroIntensity ZeroIntensity commented Apr 27, 2025

This doesn't change anything about the PEP, but fixes a few clarity issues that quickly came up. Sorry for the churn and extra PR here :(


📚 Documentation preview 📚: https://pep-previews--4391.org.readthedocs.build/pep-0788/

Comment on lines +354 to +355
For backwards compatibility, all thread states created by existing APIs,
including :c:func:`PyGILState_Ensure`, will remain daemon by default.
Copy link
Member

@AA-Turner AA-Turner Apr 27, 2025

Choose a reason for hiding this comment

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

Perhaps include the rationale you posted on Discourse here? If it's fully covered by the "We can’t change finalization behavior for PyGILState_Ensure" section, I think it would be useful to reference/link to that here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, I thought the section on this was pretty clear. I'll just link to that.

Copy link
Member

Choose a reason for hiding this comment

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

I think the confusion is that "finalisation behaviour" section makes no reference to daemon status, so it's not immediately obvious that this is the reason. The first explicit mention of backwards compatability is this section in Specification, so it might be useful to add some prose in earlier sections, to give the reader more context.

Copy link
Member Author

Choose a reason for hiding this comment

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

I linked to it there, and also added some additional information to the section in the motivation. Let me know if that's any better.

@ZeroIntensity
Copy link
Member Author

ZeroIntensity commented Apr 27, 2025

While we're here, please let me know if there are any other unclear sections. I have a bit of expert blindness, and I'd really rather make this the only follow-up for English issues.

@AA-Turner
Copy link
Member

I'd really rather make this the only follow-up for English issues.

There's no problem at all with more follow-ups, we'd rather have a clearer PEP than any arbitary limit on the number of edits allowed!

A

Co-authored-by: Victor Stinner <[email protected]>
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

In the case where it is useful to let the interpreter finalize, such as in a signal handler where there’s no guarantee that the thread will start, strong references to an interpreter can be acquired through PyInterpreterState_Lookup().

I suggest to remove mention of "signal handler". IMO a signal handler should not and cannot use the Python C API. There are too many constraints to write async-signal safe code.

@vstinner
Copy link
Member

You may add a PyThreadState_GetDaemon() function.

@ZeroIntensity
Copy link
Member Author

I have a few concerns about PyThreadState_GetDaemon. Let's do that in a different PR where I can update the rationale and specification, because we'll likely need to change how threading finalizes its threads for that to be practical.

@ZeroIntensity
Copy link
Member Author

@AA-Turner @hugovk Could one of you give this a quick review? This is supposed to just add a few clarifications, not change the proposal. I'll do a big "round 1 comments" PR separately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants