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

MainThread association logic is fragile #75698

Open
pitrou opened this issue Sep 19, 2017 · 11 comments
Open

MainThread association logic is fragile #75698

pitrou opened this issue Sep 19, 2017 · 11 comments
Labels
3.8 (EOL) end of life 3.9 only security fixes docs Documentation in the Doc dir topic-subinterpreters type-feature A feature request or enhancement

Comments

@pitrou
Copy link
Member

pitrou commented Sep 19, 2017

BPO 31517
Nosy @tim-one, @ncoghlan, @pitrou, @vstinner, @fabioz, @asvetlov, @ericsnowcurrently, @aldwinaldwin, @int19h
Files
  • mainthread2.py
  • 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 = None
    created_at = <Date 2017-09-19.13:00:11.242>
    labels = ['expert-subinterpreters', 'type-feature', '3.8', '3.9', 'docs']
    title = 'MainThread association logic is fragile'
    updated_at = <Date 2020-06-03.16:42:33.468>
    user = 'https://github.com/pitrou'

    bugs.python.org fields:

    activity = <Date 2020-06-03.16:42:33.468>
    actor = 'vstinner'
    assignee = 'docs@python'
    closed = False
    closed_date = None
    closer = None
    components = ['Documentation', 'Subinterpreters']
    creation = <Date 2017-09-19.13:00:11.242>
    creator = 'pitrou'
    dependencies = []
    files = ['47153']
    hgrepos = []
    issue_num = 31517
    keywords = []
    message_count = 11.0
    messages = ['302521', '302523', '302583', '302589', '302601', '302602', '302603', '302606', '347264', '347266', '358744']
    nosy_count = 10.0
    nosy_names = ['tim.peters', 'ncoghlan', 'pitrou', 'vstinner', 'fabioz', 'asvetlov', 'docs@python', 'eric.snow', 'aldwinaldwin', 'int19h']
    pr_nums = []
    priority = 'normal'
    resolution = None
    stage = 'needs patch'
    status = 'open'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue31517'
    versions = ['Python 3.8', 'Python 3.9']

    @pitrou
    Copy link
    Member Author

    pitrou commented Sep 19, 2017

    The threading main_thread() instance is associated to the thread that first imports the threading module. In usual circumstances, this will indeed be the interpreter's main thread. However, it is not always the case. See attached reproducer.

    $ ./python mainthread2.py 
    child thread: <_MainThread(MainThread, started 140399567398656)>
    main thread: <_DummyThread(Dummy-1, started daemon 140399588386560)>
    Exception ignored in: <module 'threading' from '/home/antoine/cpython/default/Lib/threading.py'>
    Traceback (most recent call last):
      File "/home/antoine/cpython/default/Lib/threading.py", line 1268, in _shutdown
        assert tlock.locked()
    AssertionError:

    @pitrou pitrou added type-bug An unexpected behavior, bug, or error 3.7 (EOL) end of life labels Sep 19, 2017
    @pitrou
    Copy link
    Member Author

    pitrou commented Sep 19, 2017

    Fixing this will require thinking out what the "main thread" of an interpreter really is. Is it the thread in which PyInterpreterState_New() is called?

    (note this is not the same thing as the "main thread" in the signal module, which is process-global)

    @tim-one
    Copy link
    Member

    tim-one commented Sep 20, 2017

    Is there a problem here? I haven't heard of anyone even wondering about this before. threading.py worries about Threads created by threading.py, plus the magical (from its point of view) thread that first imports threading.py. Users mix in _thread threads, or raw C threads from extension modules, essentially at their own risk. Which risks are minimal, but can have visible consequences.

    I don't view that as being a real problem. It might help if, e.g., a Wiki somewhere spelled out the consequences under different Python implementations (while I don't know for sure, I _expect_ the current docs just say "in normal conditions, the main thread is the thread from which the Python interpreter was started" because it doesn't want to over-specify the behavior in an area nobody really cares about).

    @ncoghlan
    Copy link
    Contributor

    One place where this came up recently is in working out precisely how a Python-level subinterpreter API will interact with the threading API: https://mail.python.org/pipermail/python-dev/2017-September/149566.html

    That said, I do agree with Tim that the status quo isn't broken per se: we renamed thread to _thread in Python 3 for a reason, and that reason is that you really need to know how Python's threading internals work to do it safely.

    However, I do think we can treat this as a documentation enhancement request, where the _thread module docs could point out some of the requirements to ensure that low-level thread manipulation plays nice with the threading module.

    @ncoghlan ncoghlan added the docs Documentation in the Doc dir label Sep 20, 2017
    @ncoghlan ncoghlan added type-feature A feature request or enhancement and removed type-bug An unexpected behavior, bug, or error labels Sep 20, 2017
    @pitrou
    Copy link
    Member Author

    pitrou commented Sep 20, 2017

    Is there a problem here? I haven't heard of anyone even wondering about this before.

    This came while working on the PEP-556 implementation.

    @pitrou
    Copy link
    Member Author

    pitrou commented Sep 20, 2017

    That said, I do agree with Tim that the status quo isn't broken per se: we renamed thread to _thread in Python 3 for a reason

    This is not caused by _thread specifically, but any background thread created by C code that might invoke Python code. The reproducer just uses the _thread module out of convenience.

    @pitrou
    Copy link
    Member Author

    pitrou commented Sep 20, 2017

    I'm not sure this is an issue worth fixing, but I don't think it's a good idea to document such quirky semantics.

    @ncoghlan
    Copy link
    Contributor

    We had the quirks of import related threading deadlocks documented for a long time, not as a promise, but as a debugging aid (and a recommendation for "don't do that").

    I'd see this as being similar: we'd document that if you're using the _thread module, or otherwise allowing operating system threads not managed by the threading module to call in and run arbitrary Python code, then you should run "import threading" early in the actual main thread to make sure it gets associated correctly.

    @pitrou pitrou added 3.8 (EOL) end of life 3.9 only security fixes and removed 3.7 (EOL) end of life labels Jul 3, 2019
    @vstinner
    Copy link
    Member

    vstinner commented Jul 4, 2019

    Python internals already know who is the "main" thread: _PyRuntime.main_thread. It's maintained up to date, even after a fork, PyOS_AfterFork_Child() calls _PyRuntimeState_ReInitThreads() which does:

    // This was initially set in _PyRuntimeState_Init().
    runtime->main_thread = PyThread_get_thread_ident();
    

    I already added _thread._is_main_interpreter() to deny spawning daemon threads in subinterpreters: bpo-37266.

    We can add _thread._is_main_thread() which can reuse Modules/signalmodule.c code:

    static int
    is_main(_PyRuntimeState *runtime)
    {
        unsigned long thread = PyThread_get_thread_ident();
        PyInterpreterState *interp = _PyRuntimeState_GetThreadState(runtime)->interp;
        return (thread == runtime->main_thread
                && interp == runtime->interpreters.main);
    }

    For example, this function is used by signal.signal:

        if (!is_main(runtime)) {
            PyErr_SetString(PyExc_ValueError,
                            "signal only works in main thread");
            return NULL;
        }

    @vstinner
    Copy link
    Member

    vstinner commented Jul 4, 2019

    bpo-37416 has been marked as a duplicate of this issue. It contains snippet.py to reproduce a bug.

    @ericsnowcurrently
    Copy link
    Member

    probably a duplicate: issue bpo-39042 "Use the runtime's main thread ID in the threading module."

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.8 (EOL) end of life 3.9 only security fixes docs Documentation in the Doc dir topic-subinterpreters type-feature A feature request or enhancement
    Projects
    Status: Todo
    Development

    No branches or pull requests

    5 participants