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

atexit module not safe in Python 3.0 with multiple interpreters #48450

Closed
grahamd mannequin opened this issue Oct 24, 2008 · 18 comments
Closed

atexit module not safe in Python 3.0 with multiple interpreters #48450

grahamd mannequin opened this issue Oct 24, 2008 · 18 comments
Assignees
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) release-blocker type-bug An unexpected behavior, bug, or error

Comments

@grahamd
Copy link
Mannequin

grahamd mannequin commented Oct 24, 2008

BPO 4200
Nosy @loewis, @warsaw, @tiran, @benjaminp
Files
  • atexit_modulestate.patch
  • 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/loewis'
    closed_at = <Date 2008-10-30.21:34:24.456>
    created_at = <Date 2008-10-24.22:45:53.853>
    labels = ['interpreter-core', 'type-bug', 'release-blocker']
    title = 'atexit module not safe in Python 3.0 with multiple interpreters'
    updated_at = <Date 2009-07-21.11:20:10.809>
    user = 'https://bugs.python.org/grahamd'

    bugs.python.org fields:

    activity = <Date 2009-07-21.11:20:10.809>
    actor = 'grahamd'
    assignee = 'loewis'
    closed = True
    closed_date = <Date 2008-10-30.21:34:24.456>
    closer = 'christian.heimes'
    components = ['Interpreter Core']
    creation = <Date 2008-10-24.22:45:53.853>
    creator = 'grahamd'
    dependencies = []
    files = ['11894']
    hgrepos = []
    issue_num = 4200
    keywords = ['patch', 'needs review']
    message_count = 18.0
    messages = ['75195', '75199', '75200', '75232', '75239', '75241', '75243', '75244', '75246', '75278', '75279', '75309', '75339', '75383', '90710', '90713', '90731', '90754']
    nosy_count = 5.0
    nosy_names = ['loewis', 'barry', 'christian.heimes', 'benjamin.peterson', 'grahamd']
    pr_nums = []
    priority = 'release blocker'
    resolution = 'fixed'
    stage = None
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue4200'
    versions = ['Python 3.0', 'Python 3.1']

    @grahamd
    Copy link
    Mannequin Author

    grahamd mannequin commented Oct 24, 2008

    In Python 3.0 the atexit module was translated from Python code to C code.

    Prior to Python 3.0, because it was implemented at C code level, the list of
    callbacks was specific to each sub interpreter. In Python 3.0 that appears to
    no longer be the case, with the list of callbacks being maintained globally as
    static C variable across all interpreters.

    The end result is that if a sub interpreter registers an atexit callback, that
    callback will be called within the context of the main interpreter on call of
    Py_Finalize(), and not in context of the sub interpreter against which it was
    registered.

    Various problems could ensue from this depending on whether or not the sub
    interpreter had also since been destroyed.

    Still need to validate the above, but from visual inspection looks to be a
    problem. Issue found because mod_wsgi will trigger atexit callbacks for a sub
    interpreter when the sub interpreter is being destroyed. This is because Python
    prior to 3.0 only did it from the main interpreter. In Python 3.0, this all
    seems to get messed up now as no isolation between callbacks for each
    interpreter.

    For mod_wsgi case, since it is explicitly triggering atexit callbacks for sub
    interpreter, in doing so it is actually calling all registered atexit callbacks
    across all interpreters for each sub interpreter being destroyed. They then
    again get called for Py_Finalize().

    Even if mod_wsgi weren't doing this, still a problem that Py_Finalize() calling
    atexit callbacks in context of main interpreter which were actually associated
    with sub interpreter.

    For mod_wsgi, will probably end up installing own 'atexit' module variant in
    sub interpreters to ensure separation.

    @grahamd grahamd mannequin added interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error labels Oct 24, 2008
    @tiran
    Copy link
    Member

    tiran commented Oct 25, 2008

    Ouch! We totally forgot about sub interpreters during the
    reimplementation of the atexit module.

    In my opinion it's a big bug for systems like mod_python and mod_wsgi.
    However it's too late to fix it for 3.0.0. Perhaps we can fix it in
    3.0.1. We always knew that Python 3.0 is going to have some bugs that
    can only be revealed by 3rd party code. I've added Barry to the nosy
    list in order to get his opinion as release manager.

    Do you have a suggestion how the problem should be solved? I'd store the
    atexit_callback information in PyInterpreterState instead of the module.
    Maybe Martin's PEP-3121 could come into play to rescue us.

    By the way PEP-3121 ... does the new module initialization code play
    nice with multiple sub interpreters?

    @grahamd
    Copy link
    Mannequin Author

    grahamd mannequin commented Oct 25, 2008

    I wouldn't be concerned about mod_python as likelihood that it will be ported is
    very low and even if it was it would be a long long way in the future. There is too
    much in mod_python itself unrelated to Python 3.0 that needs to be fixed before a
    port to Python 3.0 should be considered.

    As to mod_wsgi, I can work around it in the short term by installing an 'atexit'
    module replacement in sub interpreters and avoid the problem.

    As to a solution, yes, using PyInterpreterState would seem the most logical place,
    however there is a lot more to it than that.

    Prior to Python 3.0, any callbacks registered with atexit module in sub interpreters
    weren't called anyway. This is because Py_EndInterpreter() didn't trigger them, nor
    did Py_Finalize(). The latter is in Python 3.0 at the moment, but as pointed out
    that is a problem in itself.

    So, although one may register sub interpreter atexit callbacks against PyInterpreterState, what would be done with them. A decision would need to be made
    as to whether Py_EndInterpreter() should trigger them, or whether the status quo be
    maintained and nothing done with them.

    In the short term, ie., for Python 3.0.0, the simplest thing to do may be to have
    functions of atexit module silently not actually do anything for sub interpreters.

    The only place this would probably cause a problem would be for mod_wsgi where it
    was itself calling sys.exitfunc() on sub interpreters to ensure they were run. Since
    mod_wsgi has to change for Python 3.0 anyway, to call atexit._run_exitfuncs, with a
    bit more work mod_wsgi can just replace atexit module altogether in sub interpreter
    context and have mod_wsgi track the callback functions and trigger them.

    By having atexit module ignore stuff for sub interpreters, at least for now avoid
    problem of callbacks against sub interpreters being execute by Py_Finalize() in main
    interpreter context.

    And no I haven't looked at how PEP-3121 has changed things in Python 3.0. Up till
    now I hadn't seen any problems to suggest I may need to look at it.

    @tiran
    Copy link
    Member

    tiran commented Oct 26, 2008

    I'm adding Martin to the nosy list as well. He is the author of PEP-3121
    and he might be able to give more insight in the problem.

    @loewis
    Copy link
    Mannequin

    loewis mannequin commented Oct 26, 2008

    It would certainly be possible to produce per-interpreter callback lists
    through the module state. You declare a per-interpreter structure, add
    its size into atexitmodule, and refer to it through PyModule_GetDef,
    passing the self argument of the module-level functions.

    Reviewing the code, I have two unrelated remarks:

    • METH_NOARGS functions still take two arguments.
    • I would drop the double-indirection of the callback list. Instead,
      allocate a single block of atexit_callback, and use func==NULL as an
      indicator that the block is free.

    According to the documentation of the atexit module, it seems clear that
    it is a per-interpreter thing.

    @tiran
    Copy link
    Member

    tiran commented Oct 26, 2008

    I'm trying to come up with a patch. It's a little bit trickier than I
    thought. The atexit module registers "atexit_callfuncs()" with
    _Py_PyAtExit(). The "atexit_callfuncs()" function is called by
    Python/pythonrun.c when the interpreter exits. The function is cleared
    as "static void atexit_callfuncs(void)" so it doesn't get the module
    through self. However PyModule_GetDef requires self.

    In order to use PEP-3121 we have to add a mechanism to pass the module
    instance to atexit_callfuncs().

    @loewis
    Copy link
    Mannequin

    loewis mannequin commented Oct 26, 2008

    Christian Heimes wrote:

    However PyModule_GetDef requires self.

    (I actually meant PyModule_GetState).

    You can use PyState_FindModule to quickly lookup a module.

    @tiran
    Copy link
    Member

    tiran commented Oct 26, 2008

    Ah, thanks. I'll try PyState_FindModule().

    @tiran
    Copy link
    Member

    tiran commented Oct 26, 2008

    Can you review the patch, Martin? I've moved the three state variables
    into a module state struct. The patch also fixes the METH_NOARGS functions.

    @tiran
    Copy link
    Member

    tiran commented Oct 28, 2008

    Martin, can you please review and comment on my patch?

    Graham, does the patch solve your problem?

    @grahamd
    Copy link
    Mannequin Author

    grahamd mannequin commented Oct 28, 2008

    By visual inspection the intent looks correct, but can't actually test it
    until I can checkout Python code from source repository and apply patch as
    patch doesn't apply cleanly to 3.0rc1.

    With bpo-3723 and bpo-4213 now also having patches, will need to sit down and
    look at all of them and see if find any new issues. May take me a couple
    of days to get time.

    @benjaminp
    Copy link
    Contributor

    This patch looks fine.

    @grahamd
    Copy link
    Mannequin Author

    grahamd mannequin commented Oct 30, 2008

    In conjunction with bpo-3723 and bpo-4213, the attached atexit_modulestate.patch
    appears to fix issue for mod_wsgi.

    @tiran
    Copy link
    Member

    tiran commented Oct 30, 2008

    Fixed in r67054

    @tiran tiran closed this as completed Oct 30, 2008
    @grahamd
    Copy link
    Mannequin Author

    grahamd mannequin commented Jul 19, 2009

    I know this issue is closed, but for this patch, the code:

    + modstate = get_atexitmodule_state(module);
    +
    + if (modstate->ncallbacks == 0)
    + return;

    was added.

    Is there any condition under which modstate could be NULL.

    Haven't touched Python 3.0 support in mod_wsgi for a long time and when
    revisiting code with final Python 3.0, I find that I get Py_Finalize()
    crashing on process shutdown. It is crashing because modstate above is
    NULL.

    @benjaminp
    Copy link
    Contributor

    2009/7/19 Graham Dumpleton <report@bugs.python.org>:

    Graham Dumpleton <Graham.Dumpleton@gmail.com> added the comment:

    I know this issue is closed, but for this patch, the code:

    •    modstate = get_atexitmodule_state(module);
    •    if (modstate->ncallbacks == 0)
    •        return;

    was added.

    Is there any condition under which modstate could be NULL.

    The module is probably being deallocated before the callbacks are being called.

    Haven't touched Python 3.0 support in mod_wsgi for a long time and when
    revisiting code with final Python 3.0, I find that I get Py_Finalize()
    crashing on process shutdown. It is crashing because modstate above is
    NULL.

    @grahamd
    Copy link
    Mannequin Author

    grahamd mannequin commented Jul 20, 2009

    This new problem I am seeing looks like it may be linked to where the
    'atexit' module is initialised/imported in a sub interpreter but never
    in the main interpreter. I can avoid the crash by having:

        PyImport_ImportModule("atexit");
    
        Py_Finalize();

    At a guess, this is because:

        module = PyState_FindModule(&atexitmodule);
        if (module == NULL)
            return;

    still returns a module for case where imported in a sub interpreter but
    not in main interpreter, but then:

        modstate = GET_ATEXIT_STATE(module);
    if (modstate->ncallbacks == 0)
        return;
    

    returns NULL for modstate for the main interpreter as PyInit_atexit()
    had never been called for the main interpreter.

    The fix would appear to be to check modstate for being NULL and return.
    Ie.,

        module = PyState_FindModule(&atexitmodule);
        if (module == NULL)
            return;
        modstate = GET_ATEXIT_STATE(module);
    if (modstate == NULL)
        return;
    
    if (modstate->ncallbacks == 0)
        return;
    

    Does that make sense to anyone? If it does and I am correct, I'll create
    a new issue for it as original fix seems deficient.

    @grahamd
    Copy link
    Mannequin Author

    grahamd mannequin commented Jul 21, 2009

    Have created bpo-6531 for my new issue related to this patch.

    @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
    Labels
    interpreter-core (Objects, Python, Grammar, and Parser dirs) release-blocker type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants