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.

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

Sub-Interpreters : Document PyThreadState_DeleteCurrent() #82059

Closed
nanjekyejoannah opened this issue Aug 16, 2019 · 19 comments
Closed

Sub-Interpreters : Document PyThreadState_DeleteCurrent() #82059

nanjekyejoannah opened this issue Aug 16, 2019 · 19 comments
Labels
3.9 only security fixes docs Documentation in the Doc dir

Comments

@nanjekyejoannah
Copy link
Member

BPO 37878
Nosy @ncoghlan, @vstinner, @ericsnowcurrently, @miss-islington, @nanjekyejoannah
PRs
  • bpo-37878: Make PyThreadState_DeleteCurrent() Internal #15315
  • bpo-38266: Revert bpo-37878: Make PyThreadState_DeleteCurrent() Internal #16558
  • bpo-37878: PyThreadState_DeleteCurrent() was not removed #20489
  • [3.9] bpo-37878: PyThreadState_DeleteCurrent() was not removed (GH-20489) #20496
    		•

    Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
closed_at = <Date 2019-10-04.16:01:16.823>
created_at = <Date 2019-08-16.19:53:55.547>
labels = ['3.9', 'docs']
title = 'Sub-Interpreters : Document PyThreadState_DeleteCurrent()'
updated_at = <Date 2020-05-28.16:34:50.517>
user = 'https://github.com/nanjekyejoannah'

    bugs.python.org fields:

    activity = <Date 2020-05-28.16:34:50.517>
actor = 'miss-islington'
assignee = 'docs@python'
closed = True
closed_date = <Date 2019-10-04.16:01:16.823>
closer = 'vstinner'
components = ['Documentation']
creation = <Date 2019-08-16.19:53:55.547>
creator = 'nanjekyejoannah'
dependencies = []
files = []
hgrepos = []
issue_num = 37878
keywords = ['patch']
message_count = 19.0
messages = ['349881', '349954', '349960', '349979', '350671', '350773', '351210', '351211', '353079', '353080', '353369', '353923', '353925', '353927', '353936', '353949', '370236', '370237', '370240']
nosy_count = 7.0
nosy_names = ['ncoghlan', 'vstinner', 'docs@python', 'eric.snow', 'wenzel', 'miss-islington', 'nanjekyejoannah']
pr_nums = ['15315', '16558', '20489', '20496']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue37878'
versions = ['Python 3.9']
    @nanjekyejoannah
    Copy link
    Member Author

    I just noticed that PyThreadState_DeleteCurrent()in Python/pystate.c is not documented.

    Relevant documentation should go in Doc/c-api/init.rst If no one objects to this.

    @nanjekyejoannah nanjekyejoannah added easy 3.8 only security fixes 3.9 only security fixes labels Aug 16, 2019
    @nanjekyejoannah nanjekyejoannah added the docs Documentation in the Doc dir label Aug 16, 2019
    @nanjekyejoannah nanjekyejoannah changed the title sub-ineterpreters : Document PyThreadState_DeleteCurrent() ? Sub-ineterpreters : Document PyThreadState_DeleteCurrent() ? Aug 16, 2019
    @nanjekyejoannah nanjekyejoannah changed the title Sub-ineterpreters : Document PyThreadState_DeleteCurrent() ? Sub-Interpreters : Document PyThreadState_DeleteCurrent() Aug 16, 2019
    @vstinner
    Copy link
    Member

    I'm not sure why PyThreadState_DeleteCurrent() is exposed. Why would anyone use it?

    It is used internally by the _thread module when the thread function completes.

    I suggest to make the function internal instead of documenting it.

    Eric, Joannah: what do you think?

    @nanjekyejoannah
    Copy link
    Member Author

    From what I know the GIL is released by calling PyEval_ReleaseThread() or PyEval_SaveThread().

    I see we can use PyThreadState_Delete() to clean up-specifically destroy thread state but does not release the GIL and requires a thread state. On the other hand, PyThreadState_DeleteCurrent() allows to both clean up the current thread state - no thread state is required as well release the GIL which is convenient. I would not fight so much to keep it public given we have PyEval_ReleaseThread() or PyEval_SaveThread().

    @vstinner
    Copy link
    Member

    Is it safe to call PyThreadState_DeleteCurrent()?

    @vstinner vstinner removed the easy label Aug 19, 2019
    @nanjekyejoannah
    Copy link
    Member Author

    I updated the PR to make the function internal.

    @vstinner
    Copy link
    Member

    I did a GitHub code search on "PyThreadState_DeleteCurrent" in C code:
    https://github.com/search?l=C&q=PyThreadState_DeleteCurrent&type=Code

    I looked at the first 6 pages: I only found copies of the Python source code, but no *call* to this function. It seems like this function is not used outside Python, so it's fine to remove it.

    Anyway, if an user complains lately, we can add it back. It's fine.

    In case of doubt, in this case, I'm fine with removing the function.

    @vstinner
    Copy link
    Member

    vstinner commented Sep 5, 2019

    New changeset 2bc43cd by Victor Stinner (Joannah Nanjekye) in branch 'master':
    bpo-37878: Remove PyThreadState_DeleteCurrent() function (GH-15315)
    2bc43cd

    @vstinner
    Copy link
    Member

    vstinner commented Sep 5, 2019

    Thanks Joannah.

    @vstinner vstinner closed this as completed Sep 5, 2019
    @nanjekyejoannah
    Copy link
    Member Author

    Update, Some user of this function just complained on the merged pull requests here : #15315 (comment).

    Do we revert and deprecate ? cc @victor

    @wenzel
    Copy link
    Mannequin

    wenzel mannequin commented Sep 24, 2019

    Hi,

    pybind11 developer here. A bit on context of our usage of this function:

    Pybind11 supports some advanced GIL-related tricks that require access this function. For instance, pybind11 can intercept a Python function call on the main() thread and delete the associated thread state, launching a new thread and continuing Python execution there (with a newly created thread state).

    Kind of crazy, but why is this useful? UI libraries. On some platforms, it is not legal to poll UI events on any thread other than the main thread. This means that it's impossible to implement a proper UI event loop because Python is hogging the main thread. With the functionality in pybind11's gil_scoped_acquire, I can launch an event polling loop on the main thread, continue running an interactive Python session on another thread, and even swap them back e.g. when the user interface is no longer needed.

    Best,
    Wenzel

    @ericsnowcurrently
    Copy link
    Member

    I've re-opened this issue to address the original point: documenting PyThreadState_DeleteCurrent(). FWIW, I don't see a problem with documenting it if we're keeping it around (which it looks like we are).

    We will address reverting #59520 in bpo-38266.

    @vstinner
    Copy link
    Member

    vstinner commented Oct 4, 2019

    New changeset 8855e47 by Victor Stinner (Joannah Nanjekye) in branch 'master':
    bpo-38266: Revert bpo-37878: Make PyThreadState_DeleteCurrent() Internal (GH-16558)
    8855e47

    @vstinner
    Copy link
    Member

    vstinner commented Oct 4, 2019

    bpo-38266: Revert bpo-37878: Make PyThreadState_DeleteCurrent() Internal (GH-16558)

    Oh, I didn't notice something in the review: the revert moved PyThreadState_DeleteCurrent() definition from Include/pystate.h to Include/cpython/pystate.h. Previously, it was defined in Include/pystate.h, see the commit removing it:
    2bc43cd

    Joannah: can you please move the definition back to Include/pystate.h, at the same place it was before the removal, please?

    The revert excluded PyThreadState_DeleteCurrent() from the stable API (Py_LIMITED_API).

    @vstinner vstinner removed the 3.8 only security fixes label Oct 4, 2019
    @vstinner
    Copy link
    Member

    vstinner commented Oct 4, 2019

    To be clear: this issue only impacts Python 3.8. The function was only removed from the master branch. It was still in the 3.8 branch.

    @nanjekyejoannah
    Copy link
    Member Author

    Oh, I didn't notice something in the review: the revert moved >PyThreadState_DeleteCurrent() definition from Include/pystate.h to >Include/cpython/pystate.h. Previously, it was defined in > Include/pystate.h, see the commit removing it:

    I made this change as it was requested by Eric snow in bpo-38266.

    "when we un-revert we should be sure to move PyThreadState_DeleteCurrent() from Include/pystate.h to Include/cpython/pystate.h."

    @vstinner
    Copy link
    Member

    vstinner commented Oct 4, 2019

    "when we un-revert we should be sure to move PyThreadState_DeleteCurrent() from Include/pystate.h to Include/cpython/pystate.h."

    Ah, I didn't see this comment. If Eric endorses this change, I'm fine with it :-)

    I close the issue.

    If someone wants to modify PyThreadState_DeleteCurrent() to automatically call PyThreadState_Clear(), please open a new issue for that.

    @vstinner vstinner closed this as completed Oct 4, 2019
    @vstinner
    Copy link
    Member

    New changeset fda7f6d by Victor Stinner in branch 'master':
    bpo-37878: PyThreadState_DeleteCurrent() was not removed (GH-20489)
    fda7f6d

    @vstinner
    Copy link
    Member

    I was very confused by the history of this issue. To be clear, PyThreadState_DeleteCurrent() is now documented in Python 3.9:
    https://docs.python.org/dev/c-api/init.html#c.PyThreadState_DeleteCurrent

    @miss-islington
    Copy link
    Contributor

    New changeset 6a47864 by Miss Islington (bot) in branch '3.9':
    bpo-37878: PyThreadState_DeleteCurrent() was not removed (GH-20489)
    6a47864

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees
    No one assigned
    Labels
    3.9 only security fixes docs Documentation in the Doc dir
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    4 participants
    @vstinner @ericsnowcurrently @miss-islington @nanjekyejoannah