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

Jump to bottom

Add PyInterpreterState.runtime. #80999

Closed
ericsnowcurrently opened this issue May 6, 2019 · 9 comments
Closed

Add PyInterpreterState.runtime. #80999

ericsnowcurrently opened this issue May 6, 2019 · 9 comments
Assignees
@ericsnowcurrently
Labels
3.8 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs)

Comments

@ericsnowcurrently
Copy link
Member

BPO 36818
Nosy @vstinner, @ericsnowcurrently
PRs
  • bpo-36818: Add PyInterpreterState.runtime field. #13129
  • Revert "bpo-36818: Add PyInterpreterState.runtime field. (gh-13129)" #13795
    		•

    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 = 'https://github.com/ericsnowcurrently'
closed_at = <Date 2020-03-09.23:28:59.157>
created_at = <Date 2019-05-06.18:45:44.904>
labels = ['interpreter-core', '3.8']
title = 'Add PyInterpreterState.runtime.'
updated_at = <Date 2020-03-09.23:28:59.156>
user = 'https://github.com/ericsnowcurrently'

    bugs.python.org fields:

    activity = <Date 2020-03-09.23:28:59.156>
actor = 'vstinner'
assignee = 'eric.snow'
closed = True
closed_date = <Date 2020-03-09.23:28:59.157>
closer = 'vstinner'
components = ['Interpreter Core']
creation = <Date 2019-05-06.18:45:44.904>
creator = 'eric.snow'
dependencies = []
files = []
hgrepos = []
issue_num = 36818
keywords = ['patch']
message_count = 9.0
messages = ['341595', '342043', '342134', '344142', '344513', '344514', '344515', '344558', '363789']
nosy_count = 2.0
nosy_names = ['vstinner', 'eric.snow']
pr_nums = ['13129', '13795']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue36818'
versions = ['Python 3.8']
    @ericsnowcurrently
    Copy link
    Member Author

    Currently we use the _PyRuntime static global to access the runtime state in various places. At the same time, in thread contexts we get access to the thread state from Thread-Local Storage and the interpreter state by indirection from there. We should do the same for the runtime state instead of using the global directly.

    My plan is to add a PyInterpreterState.runtime field. It can then be used in the same way we use PyThreadState.interp to access the interpreter state.

    @ericsnowcurrently ericsnowcurrently added the 3.8 only security fixes label May 6, 2019
    @ericsnowcurrently ericsnowcurrently self-assigned this May 6, 2019
    @ericsnowcurrently ericsnowcurrently added the interpreter-core (Objects, Python, Grammar, and Parser dirs) label May 6, 2019
    @vstinner
    Copy link
    Member

    I don't see the point of PyInterpreterState.runtime:

    • I understood that we should have and only one _PyRuntimeState instance: _PyRuntime. Why not accessing it directly?
    • I understood that most _PyRuntimeState fields should be moved to PyInterpreterState. Once this refactoring will be done, we will almost never have to access _PyRuntime.

    @vstinner
    Copy link
    Member

    Eric is working on a different approach: https://bugs.python.org/issue36854

    @ericsnowcurrently
    Copy link
    Member Author

    New changeset 396e0a8 by Eric Snow in branch 'master':
    bpo-36818: Add PyInterpreterState.runtime field. (gh-13129)
    396e0a8

    @vstinner
    Copy link
    Member

    vstinner commented Jun 4, 2019

    New changeset 0fd2c30 by Victor Stinner in branch 'master':
    Revert "bpo-36818: Add PyInterpreterState.runtime field. (gh-13129)" (GH-13795)
    0fd2c30

    @vstinner
    Copy link
    Member

    vstinner commented Jun 4, 2019

    This change introduced a regression:
    https://bugs.python.org/issue37135#msg344511

    I had to revert the change to fix Python for 3.8 beta1 release.

    @vstinner vstinner reopened this Jun 4, 2019
    @vstinner
    Copy link
    Member

    vstinner commented Jun 4, 2019

    By the way, I have questions about the rationale of this change.

    Currently we use the _PyRuntime static global to access the runtime state in various places. At the same time, in thread contexts we get access to the thread state from Thread-Local Storage and the interpreter state by indirection from there. We should do the same for the runtime state instead of using the global directly.

    Right now I'm confused. _PyRuntimeState contains scatted states: things which should be moved to PyInterpreterState or even PyThreadState, and things which should remain in _PyRuntimeState. My notes on this topic:
    https://pythoncapi.readthedocs.io/runtime.html

    Is PyInterpreterState.runtime a temporary fix until things are moved?

    Or do you plan to never access _PyRuntime directly, and always through interp->runtime?

    It seems like in the short term, we must continue to access "_PyRuntime" through a separated "runtime" pointer passed to functions, to handle daemon threads during Python shutdown.

    @vstinner
    Copy link
    Member

    vstinner commented Jun 4, 2019

    See https://bugs.python.org/issue37135#msg344509 to reproduce https://bugs.python.org/issue37135 crash without multiprocessing: with this procedure, I reproduce the crash immediately on Linux. No need for a magic super slow buildbot.

    @vstinner
    Copy link
    Member

    vstinner commented Mar 9, 2020

    I added PyInterpreterState.runtime in bpo-36710:

    commit 01b1cc1
    Author: Victor Stinner <vstinner@python.org>
    Date: Wed Nov 20 02:27:56 2019 +0100

    bpo-36710: Add PyInterpreterState.runtime field (GH-17270)

Add PyInterpreterState.runtime field: reference to the _PyRuntime
global variable. This field exists to not have to pass runtime in
addition to tstate to a function.  Get runtime from tstate:
tstate->interp->runtime.

Remove "_PyRuntimeState *runtime" parameter from functions already
taking a "PyThreadState *tstate" parameter.

_PyGC_Init() first parameter becomes "PyThreadState *tstate".

    --

    This change introduced a regression:
    https://bugs.python.org/issue37135#msg344511

    Funny/not funny, I introduced the same bug and I fixed it in bpo-39877.

    @vstinner vstinner closed this as completed Mar 9, 2020
    @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

    @ericsnowcurrently ericsnowcurrently

    Labels
    3.8 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs)
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    2 participants
    @vstinner @ericsnowcurrently