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

Clean modules in the reversed order #77512

Closed
serhiy-storchaka opened this issue Apr 22, 2018 · 7 comments
Closed

Clean modules in the reversed order #77512

serhiy-storchaka opened this issue Apr 22, 2018 · 7 comments
Labels
3.8 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement

Comments

@serhiy-storchaka
Copy link
Member

BPO 33331
Nosy @warsaw, @brettcannon, @ncoghlan, @pitrou, @ericsnowcurrently, @serhiy-storchaka
PRs
  • bpo-33331: Clean modules in the reversed order in PyImport_Cleanup(). #6565
  • 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 2018-10-29.18:14:30.066>
    created_at = <Date 2018-04-22.10:38:18.118>
    labels = ['interpreter-core', 'type-feature', '3.8']
    title = 'Clean modules in the reversed order'
    updated_at = <Date 2018-10-29.18:14:30.064>
    user = 'https://github.com/serhiy-storchaka'

    bugs.python.org fields:

    activity = <Date 2018-10-29.18:14:30.064>
    actor = 'serhiy.storchaka'
    assignee = 'none'
    closed = True
    closed_date = <Date 2018-10-29.18:14:30.066>
    closer = 'serhiy.storchaka'
    components = ['Interpreter Core']
    creation = <Date 2018-04-22.10:38:18.118>
    creator = 'serhiy.storchaka'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 33331
    keywords = ['patch']
    message_count = 7.0
    messages = ['315603', '319135', '319136', '326798', '327037', '327048', '328844']
    nosy_count = 6.0
    nosy_names = ['barry', 'brett.cannon', 'ncoghlan', 'pitrou', 'eric.snow', 'serhiy.storchaka']
    pr_nums = ['6565']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue33331'
    versions = ['Python 3.8']

    @serhiy-storchaka
    Copy link
    Member Author

    Since dict is ordered, modules in sys.modules are ordered for the time of importing. Currently they are cleaned in PyImport_Cleanup() in the direct order -- from imported first to imported later. I wonder if cleaning them in the reversed order can solve some problems with the interpreter shutdown.

    For example reverting the order fixes bpo-33328 and may help in other cases.

    If revert the order, should only iterating weaklist be reverted (with setting all module globals to None), or iterating sys.modules (with setting sys.module values to None) too?

    @serhiy-storchaka serhiy-storchaka added 3.7 (EOL) end of life 3.8 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) labels Apr 22, 2018
    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented Jun 9, 2018

    I think the key concern here is that we *don't* consistently add modules to sys.modules in the order their bodies are executed: we add them in a kind of sawtooth order based on the imports from __main__, and the order of import statements in the imported modules.

    For example, given the following import chains:

    __main__ imports A then X
    A imports B which imports C which imports D
    X imports Y which imports B
    X then imports Z which imports C
    

    Then the order in which modules get added to sys.modules will be:

    __main__, A, B, C, D, X, Y, Z
    

    and they'll get cleaned up from left to right

    (We're making the assumption here that, for whatever reason, GC hasn't cleaned up A, X, and their dependencies after sys.modules got cleared)

    This means that in the status quo, unloading X, Y, and Z can have problems, since B, C, and D will already be gone.

    Reversing the order doesn't fix that, and if anything will make things worse, as it means that in the "A -> B -> C -> D" dependency chain, A now gets cleared *last*, instead of getting cleared first as it does today.

    So instead of just reversing the order, I wondering if what we may want to consider doing is to:

    1. Make sure that we're dropping all the interpreter's internal references to __main__ before clearing sys.modules
    2. When running through the module weakref list, force another cyclic GC run after each not-yet-cleared module gets cleaned up

    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented Jun 9, 2018

    It also occurred to me to ask whether or not moving modules to the end of sys.modules each time they're successfully imported would help solve the problem (albeit at the expense of making import cache hits more expensive).

    I don't think it does, though, since in my example above, the least-recently-imported ordering would end up looking like:

    __main__, A, D, X, Y, B, Z, C
    

    Since D was only imported by C, and hence never gets moved to the end later, even when C gets moved by the import from Z.

    Instead, if we truly wanted to solve the problem comprehensively, we'd need to:

    1. Use a context variable to track a stack of "currently running imports"
    2. Track (as a process global) a directed (potentially cyclic!) graph of which modules imported other modules (i.e. the import of the target module started while the importing module was the currently active import on the stack). Lazy imports would typically show up as being initiated by __main__.
    3. During shutdown, linearise the shutdown order for any modules which weren't already cleaned up by the cyclic GC.

    Taking better advantage of the existing cyclic GC seems like it should be simpler, though, and would hopefully be enough to handle all but the most complex edge cases.

    @serhiy-storchaka
    Copy link
    Member Author

    If move a module to the end of sys.modules when it have finished execution, we will get the following order:

    D, C, B, A, Y, Z, X, __main__
    

    In can be cleaned from the end, and each module will be cleaned before its dependencies.

    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented Oct 4, 2018

    Aye, that option sounds like it would work to me (as long as throwing an
    exception is counted as finishing execution, so the failed module gets
    moved to the end before getting cleaned up)

    @serhiy-storchaka
    Copy link
    Member Author

    While this change fixes bpo-33328, it doesn't help with the similar bpo-13044 which is more complex. Still I think there is a value of wiping modules in such order.

    @serhiy-storchaka
    Copy link
    Member Author

    New changeset c93c58b by Serhiy Storchaka in branch 'master':
    bpo-33331: Clean modules in the reversed order in PyImport_Cleanup(). (GH-6565)
    c93c58b

    @serhiy-storchaka serhiy-storchaka removed the 3.7 (EOL) end of life label Oct 29, 2018
    @serhiy-storchaka serhiy-storchaka added the type-feature A feature request or enhancement label Oct 29, 2018
    @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
    3.8 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants