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

test.support.import_fresh_module fails to correctly block submodules when fresh is specified #84354

Closed
pganssle opened this issue Apr 3, 2020 · 7 comments
Labels
3.9 only security fixes 3.10 only security fixes 3.11 only security fixes tests Tests in the Lib/test dir type-bug An unexpected behavior, bug, or error

Comments

@pganssle
Copy link
Member

pganssle commented Apr 3, 2020

BPO 40173
Nosy @brettcannon, @ncoghlan, @ericsnowcurrently, @serhiy-storchaka, @pganssle, @shihai1991
PRs
  • bpo-40173: Remove redundant import module of import_fresh_module() #19390
  • bpo-40173: Fix test.support.import_helper.import_fresh_module #28654
  • [3.10] bpo-40173: Fix test.support.import_helper.import_fresh_module(… #28657
  • [3.9] bpo-40173: Fix test.support.import_helper.import_fresh_module()… #28658
  • Files
  • test_support_repro.zip: Module to reproduce the issue
  • 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 2021-10-01.07:08:55.732>
    created_at = <Date 2020-04-03.15:18:43.817>
    labels = ['type-bug', 'tests', '3.9', '3.10', '3.11']
    title = 'test.support.import_fresh_module fails to correctly block submodules when fresh is specified'
    updated_at = <Date 2021-10-01.07:08:55.732>
    user = 'https://github.com/pganssle'

    bugs.python.org fields:

    activity = <Date 2021-10-01.07:08:55.732>
    actor = 'serhiy.storchaka'
    assignee = 'none'
    closed = True
    closed_date = <Date 2021-10-01.07:08:55.732>
    closer = 'serhiy.storchaka'
    components = ['Tests']
    creation = <Date 2020-04-03.15:18:43.817>
    creator = 'p-ganssle'
    dependencies = []
    files = ['49031']
    hgrepos = []
    issue_num = 40173
    keywords = ['patch']
    message_count = 7.0
    messages = ['365702', '365846', '402961', '402970', '402972', '402973', '402975']
    nosy_count = 6.0
    nosy_names = ['brett.cannon', 'ncoghlan', 'eric.snow', 'serhiy.storchaka', 'p-ganssle', 'shihai1991']
    pr_nums = ['19390', '28654', '28657', '28658']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue40173'
    versions = ['Python 3.9', 'Python 3.10', 'Python 3.11']

    @pganssle
    Copy link
    Member Author

    pganssle commented Apr 3, 2020

    It seems that test.support.import_fresh_module gets tripped up with its module blocking when you attempt to get a fresh copy of a submodule of a module where you are also importing the module that you are trying to block (bit of a doozy of a sentence there...). So, for example, with the following configuration in mymodule/init.py:

        from .other import other
        
        try:
            from ._a import attr
        except ImportError:
            from ._b import attr

    (Assuming _a.attr = "A" and _b.attr = "B"), if you attempt to do:

        m = test.support.import_fresh_module("mymodule", fresh=("mymodule._other",), blocked=("mymodule._a"))

    Then you'll find that m.attr is pulled from _a.attr. Here's a small script to demonstrate:

        from test.support import import_fresh_module
        import sys
    
        def import_ab(fresh_other):
            fresh = ("mymodule._other", ) if fresh_other else ()
    
            mods_out = []
            for to_block in "_b", "_a":
                blocked = (f"mymodule.{to_block}",)
    
                mods_out.append(import_fresh_module("mymodule",
                                                    fresh=fresh, blocked=blocked))
            return mods_out
    
    
        for fresh_other in [True, False]:
            mymodule_a, mymodule_b = import_ab(fresh_other)
    
            qualifier = "With" if fresh_other else "Without"
            print(f"{qualifier} a fresh import of mymodule._other")
    
            print(f"a: {mymodule_a.attr}")
            print(f"b: {mymodule_b.attr}")
            print()

    When you run it with a suitably configured module on Python 3.8:

    $ python importer.py 
    With a fresh import of mymodule._other
    a: A
    b: A

    Without a fresh import of mymodule._other
    a: A
    b: B

    It also happens if you add mymodule._a or mymodule._b to the fresh list when you are trying to block the other one.

    I think the problem is that in the step where _save_and_remove_module is called on fresh_name (see here:

    for fresh_name in fresh:
    _save_and_remove_module(fresh_name, orig_modules)
    ), it's necessarily populating sys.modules with a fresh import of the top-level module we're trying to import (mymodule) before the blocking goes into effect, then the final call to importlib.import_module just hits that cache.

    I think either of the following options will fix this issue:

    1. Switching the order of how "fresh" and "blocked" are resolved or
    2. Deleting sys.modules[name] if it exists immediately before calling `importlib.import_module(name)

    That said, I'm still having some weird statefulness problems if I block a C module's import and *then* block a Python module's import, so there may be some other underlying pathology to the current approach.

    @pganssle pganssle added 3.7 (EOL) end of life 3.8 only security fixes 3.9 only security fixes tests Tests in the Lib/test dir type-bug An unexpected behavior, bug, or error labels Apr 3, 2020
    @shihai1991
    Copy link
    Member

    I *think* the problem is that in the step where _save_and_remove_module is called on fresh_name (see here:

    for fresh_name in fresh:
    _save_and_remove_module(fresh_name, orig_modules)
    )

    Looks like deleting a module name after __import__(name) is not good enought in https://github.com/python/cpython/blob/master/Lib/test/support/__init__.py#L244.(some redundant submodules should be removed, no?)

    paul's example can be passed in PR19390.

    @serhiy-storchaka
    Copy link
    Member

    I just encountered this bug after rewriting datetime tests in bpo-45229. It is also the cause of bpo-40058.

    Not knowing about this issue I have wrote different patch than Hai Shi. It makes the code of import_fresh_module() clearer and fixes many other issues.

    @serhiy-storchaka serhiy-storchaka added 3.10 only security fixes 3.11 only security fixes and removed 3.7 (EOL) end of life 3.8 only security fixes labels Sep 30, 2021
    @serhiy-storchaka
    Copy link
    Member

    New changeset ec4d917 by Serhiy Storchaka in branch 'main':
    bpo-40173: Fix test.support.import_helper.import_fresh_module() (GH-28654)
    ec4d917

    @serhiy-storchaka
    Copy link
    Member

    New changeset 7873884 by Serhiy Storchaka in branch '3.10':
    [3.10] bpo-40173: Fix test.support.import_helper.import_fresh_module() (GH-28654) (GH-28657)
    7873884

    @serhiy-storchaka
    Copy link
    Member

    New changeset f7e99c9 by Serhiy Storchaka in branch '3.9':
    [3.9] bpo-40173: Fix test.support.import_helper.import_fresh_module() (GH-28654) (GH-28658)
    f7e99c9

    @serhiy-storchaka
    Copy link
    Member

    _save_and_remove_module in the old code did too much and too little. It tested that the fresh module is importable, saved the current state of the fresh module and its submodules and removed the fresh module and its submodules. Since it tested importability before saving all states, it can have a side effect of importing other fresh module, and since it did it before blocking, the imported "fresh" module could import the blocked modules.

    1. Switching the order of how "fresh" and "blocked" are resolved or

    It does not help, because the step of resolving "fresh" includes saving the sys.modules state, and the step of resolving "blocked" modifies sys.modules. Also the step of resolving "fresh" can modify sys.modules before saving its sate.

    1. Deleting sys.modules[name] if it exists immediately before calling `importlib.import_module(name)

    It would be not enough, because the problem can occur in one of additional fresh modules imported by the initial module. We need to remove all fresh modules before attempting to import them.

    The new import_fresh_module() consists of the following steps:

    1. Save the state of all fresh and blocked modules and their submodules.
    2. Remove the fresh modules and their submodules and block the blocked modules and their submodules.
    3. Import all additional fresh modules and return None if any of them is not importable.
    4. Import the initial module and raise ImportError if it is not importable.
    5. Restore states of all fresh and blocked modules and their submodules.

    It does not save and restore the state of all modules, because some indirectly imported modules can not support repeated importing. It can be reconsidered in new versions.

    @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.9 only security fixes 3.10 only security fixes 3.11 only security fixes tests Tests in the Lib/test dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants