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

SystemError on importing module from unloaded package #75059

Closed
serhiy-storchaka opened this issue Jul 8, 2017 · 28 comments
Closed

SystemError on importing module from unloaded package #75059

serhiy-storchaka opened this issue Jul 8, 2017 · 28 comments
Labels
3.7 (EOL) end of life interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error

Comments

@serhiy-storchaka
Copy link
Member

BPO 30876
Nosy @brettcannon, @doko42, @ncoghlan, @pitrou, @larryhastings, @ned-deily, @encukou, @ericsnowcurrently, @serhiy-storchaka, @vaultah
PRs
  • bpo-30876: Relative import from unloaded package now reimports the package #2639
  • bpo-30891: importlib _find_and_load() try/except #2665
  • [3.6] bpo-30876: Relative import from unloaded package now reimports the package (GH-2639) #2676
  • [3.5] bpo-30876: Relative import from unloaded package now reimports the package (GH-2639) #2677
  • bpo-30814, bpo-30876: Add new import test files to projects. #2851
  • [3.6] bpo-30814, bpo-30876: Add new import test files to projects. (GH-2851). #2912
  • [3.5] bpo-30876: Add new import test files to projects. (GH-2851). #2913
  • Files
  • import-systemerror.zip
  • 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 2017-07-27.10:18:26.840>
    created_at = <Date 2017-07-08.05:07:16.228>
    labels = ['interpreter-core', 'type-bug', '3.7']
    title = 'SystemError on importing module from unloaded package'
    updated_at = <Date 2017-08-09.03:19:53.605>
    user = 'https://github.com/serhiy-storchaka'

    bugs.python.org fields:

    activity = <Date 2017-08-09.03:19:53.605>
    actor = 'larry'
    assignee = 'none'
    closed = True
    closed_date = <Date 2017-07-27.10:18:26.840>
    closer = 'serhiy.storchaka'
    components = ['Interpreter Core']
    creation = <Date 2017-07-08.05:07:16.228>
    creator = 'serhiy.storchaka'
    dependencies = []
    files = ['46997']
    hgrepos = []
    issue_num = 30876
    keywords = []
    message_count = 28.0
    messages = ['297940', '297942', '297943', '297944', '297947', '297964', '297965', '297986', '297992', '298193', '298194', '298196', '298200', '298201', '298417', '298889', '298950', '299231', '299251', '299297', '299303', '299304', '299828', '299832', '299833', '299871', '299963', '299964']
    nosy_count = 10.0
    nosy_names = ['brett.cannon', 'doko', 'ncoghlan', 'pitrou', 'larry', 'ned.deily', 'petr.viktorin', 'eric.snow', 'serhiy.storchaka', 'vaultah']
    pr_nums = ['2639', '2665', '2676', '2677', '2851', '2912', '2913']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue30876'
    versions = ['Python 3.5', 'Python 3.6', 'Python 3.7']

    @serhiy-storchaka
    Copy link
    Member Author

    It is possible to get SystemError on import (see attached archive).

    $ ./python -c 'import package.module1'
    Traceback (most recent call last):
      File "<string>", line 1, in <module>
      File "/home/serhiy/py/cpython/package/module1.py", line 3, in <module>
        from . import module2
    SystemError: Parent module 'package' not loaded, cannot perform relative import

    SystemError means a programming error in interpreter core or extension. It is comparable to an assert in C code, but without immediate crashing. Since this situation can be provoked by user code, it should be replaced with other exception (KeyError, RuntimeError, ImportError, etc).

    @serhiy-storchaka serhiy-storchaka added 3.7 (EOL) end of life interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error labels Jul 8, 2017
    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented Jul 8, 2017

    I don't think we're that strict with SystemError - once folks are messing about with deleting things from the sys module, they *are* writing their own system level code, and may end up provoking SystemError if they corrupt the interpreter state in the process.

    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented Jul 8, 2017

    To summarise what the attached source archive is doing, module1.py is essentially:

        import sys
        del sys.modules(__package__)
        from . import module2

    So the only way to trigger this is by corrupting the import state, which seems like an appropriate use of SystemError to me.

    @serhiy-storchaka
    Copy link
    Member Author

    I don't know other way to provoke SystemError by Python code. Always if SystemError was leaked this considered a bug and was fixed.

    When unload package you need to remove its name and names of its submodules from sys.modules. This is a common case. If the submodule is imported at the same time in other thread you can get SystemError (randomly, with very small probability). I think that if the error can't be avoided, SystemError is a wrong exception for this case.

    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented Jul 8, 2017

    If there are intermittent concurrent problems associated with this behaviour, I think that may be a sign that the current management of the per-module import locks is inadequate, since it isn't adequately accounting for direct manipulation of sys.modules in user code.

    Fully resolving that would probably mean ensuring that:

    1. For submodule imports, we always acquire the parent module locks in a consistent order, and then hang onto them until the submodule import is complete rather than letting them go as soon as the parent module import is complete
    2. We move the current sys.modules to sys._modules, and make sys.modules a proxy mapping that acquires the relevant import locks in the same consistent order for set and delete operations before mutating sys._modules

    If we did that, then we should consistently get one of the following two orders:

    1. Thread A imports package, package.module1, package.module2; Thread B then unloads (and maybe reloads) package; or
    2. Thread B then unloads (and maybe reloads) package; Thread A imports package, package.module1, package.module2

    By contrast, at the moment there's a window where the following can happen:

    • Thread A finishes importing "package" and starts importing "package.module1"
    • Thread B unloads "package"
    • Thread A hits SystemError through no fault of its own when it hits the "from . import module2" line

    That said, formulating the problem that way does suggest another potential resolution: in this scenario, we could treat "from . import module2" *exactly* the same way we would handle "import package.module2", which would be to just import "package" again, rather than complaining that it "should" already be imported.

    In addition to being vastly simpler to implement, that approach would have the virtue of also fixing the "del sys.modules(package)" case, not just the potential race condition.

    @pitrou
    Copy link
    Member

    pitrou commented Jul 8, 2017

    I concur with Serhiy: SystemError is a smell that C code isn't taking appropriate precautions before dealing with user code or data.

    @brettcannon
    Copy link
    Member

    So this is very old semantics from the Python 2 days. I think Nick's idea of transforming the import to import pkg.mod when pkg isn't in sys.modules is probably the simplest, cleanest solution if we're going to change this.

    @pppery pppery mannequin changed the title SystemError on import SystemError on importing module that deletes itself from sys.modules Jul 8, 2017
    @serhiy-storchaka serhiy-storchaka changed the title SystemError on importing module that deletes itself from sys.modules SystemError on importing module from unloaded package Jul 9, 2017
    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented Jul 9, 2017

    OK, so at least for 3.7, we'll replace the SystemError with a recursive import of the missing parent package, just as we'd expect to see with an absolute import.

    I'm classing this as "Won't fix" for the native import system in 2.7 - folks wanting the fix there will need to switch to using importlib2 (assuming that gets updated to include the fix at some point).

    That leaves 3.5 and 3.6, and I'd suggest we defer making a decision about whether or not to backport the fix to the maintenance branches until after we see how complex the patch for 3.7 ends up being.

    @serhiy-storchaka
    Copy link
    Member Author

    It is easy to replace the SystemError with a recursive import of the missing parent package. It is enough to remove raising the SystemError.

    The other effect of this change is that relative import from the top-level module now raises ImportError "attempted relative import with no known parent package" instead of SystemError "Parent module '' not loaded, cannot perform relative import".

    @serhiy-storchaka
    Copy link
    Member Author

    New changeset 8a9cd20 by Serhiy Storchaka in branch 'master':
    bpo-30876: Relative import from unloaded package now reimports the package (bpo-2639)
    8a9cd20

    @serhiy-storchaka
    Copy link
    Member Author

    What about other branches? Should we backport this change to them?

    I think that even if not backport this change we should change SystemError to more appropriate exception.

    @ncoghlan
    Copy link
    Contributor

    The fix is unintrusive enough that I'm +1 for also fixing it in 3.6 and 3.5.

    Trying to fix it in 2.7 would likely be more trouble than it's worth, but I also wouldn't be opposed to fixing it there if you or anyone else wanted to do it.

    @serhiy-storchaka
    Copy link
    Member Author

    When backported bpo-30876 to 3.5 I found that the lines

        elif not package:
            raise ImportError('attempted relative import with no known parent '
                              'package')
    

    don't exist in 3.5. They where added in bpo-26367, but only in the 3.6 branch. The original example in bpo-26367 still returns a module with a wrong name in 3.5.

    Why bpo-26367 changes were applied to 3.5 only partially? I afraid that the absence of this check may interfere with removing SystemError.

    @serhiy-storchaka
    Copy link
    Member Author

    Ah, this is because bpo-18018 was fixed only in 3.6. bpo-18018 is similar to this issue, it is about confusing SystemError. As a side effect it solved an example from bpo-26367 in C builtin __import__.

    I think that both raising SystemError and returning a module with a wrong name are bugs that worth to be fixed in 3.5.

    @serhiy-storchaka
    Copy link
    Member Author

    New changeset 28343e3 by Serhiy Storchaka in branch '3.6':
    [3.6] bpo-30876: Relative import from unloaded package now reimports the package (GH-2639) (bpo-2676)
    28343e3

    @serhiy-storchaka
    Copy link
    Member Author

    New changeset c824cc8 by Serhiy Storchaka in branch '3.5':
    [3.5] Backport bpo-30876 (GH-2639), bpo-18018 and bpo-26367. (bpo-2677)
    c824cc8

    @ned-deily
    Copy link
    Member

    When running tests from installed location, test_import now fails on master, 3.6, and 3.5 (including v3.5.4rc1) with:

    ======================================================================
    ERROR: test_import_from_unloaded_package (test.test_import.RelativeImportTests)
    ----------------------------------------------------------------------

    Traceback (most recent call last):
      File "/Users/nad/Projects/PyDev/active/dev/3x/root/uxd/lib/python3.7/test/test_import/__init__.py", line 704, in test_import_from_unloaded_package
        import package2.submodule1
    ModuleNotFoundError: No module named 'package2'

    @vaultah
    Copy link
    Mannequin

    vaultah mannequin commented Jul 26, 2017

    test_concurrency (test.test_import.ImportTests) seems to fail, too, with a similar error:

    ModuleNotFoundError: No module named 'package'
    

    Apparently, this happens because the LIBSUBDIRS variable in Makefile doesn't include all the subdirectories of test/test_import/data. Adding the following two lines fixed the issue for me:

    test/test_import/data/package \
    test/test_import/data/package2 \

    Should I submit a patch?

    @vaultah
    Copy link
    Mannequin

    vaultah mannequin commented Jul 26, 2017

    Please ignore my last message, I didn't notice the existing pull request... Sorry.

    @serhiy-storchaka
    Copy link
    Member Author

    New changeset d5ed47d by Serhiy Storchaka in branch 'master':
    bpo-30814, bpo-30876: Add new import test files to projects. (bpo-2851)
    d5ed47d

    @serhiy-storchaka
    Copy link
    Member Author

    New changeset 95b16a9 by Serhiy Storchaka in branch '3.6':
    [3.6] bpo-30814, bpo-30876: Add new import test files to projects. (GH-2851). (bpo-2912)
    95b16a9

    @serhiy-storchaka
    Copy link
    Member Author

    New changeset f9fbed1 by Serhiy Storchaka in branch '3.5':
    [3.5] bpo-30876: Add new import test files to projects. (GH-2851). (bpo-2913)
    f9fbed1

    @larryhastings
    Copy link
    Contributor

    It seems this change broke the 3.5 build on AMD FreeBSD:

    http://buildbot.python.org/all/builders/AMD64%20FreeBSD%20CURRENT%20Debug%203.5

    On the 3.5 branch, the previous revision (19b2890) built and tested successfully on July 26. But once this revision (f9fbed1) was checked in on July 27 the build started failing.

    I haven't included this fix in 3.5.4. Please fix the buildbot and it'll go into 3.5.5.

    @serhiy-storchaka
    Copy link
    Member Author

    This is a FreeBSD bug. It isn't related to this change. See bpo-31044 for details.

    @larryhastings
    Copy link
    Contributor

    Okay, that's good news. That means I don't have to accept a fix for it in the 3.5 branch *after* 3.5 transitions to security-fixes-only mode tomorrow ;-)

    @ned-deily
    Copy link
    Member

    Just to clarify: the revision added since 3.5.4rc1, f9fbed1 ([3.5] bpo-30876: Add new import test files to projects) fixes a test failure seen on all non-Windows platforms when running tests from an installed Python. It's probably not worth retagging 3.5.4 final at this point just for this.

    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented Aug 9, 2017

    Tagging Petr Viktorin and Matthias Klose, as this may impact distro builds of 3.5.4 that run the tests over the installed version rather than directly from the VCS checkout.

    It would be slightly helpful to not have to carry the "fix Python's self-tests" patch indefinitely, but it's also not likely to be a major cause of patch conflicts with security updates, so it shouldn't matter much either way.

    @larryhastings
    Copy link
    Contributor

    If the change has been checked into the 3.5 branch, it'll go out with 3.5.5.

    TBH we should probably stop accepting bug fixes into a branch when it hits rc1 of "last release to accept bugfixes" (e.g. 3.5.4rc1). There are two or three bugfixes in the 3.5 branch that I cherry-picked around when I made 3.5.4 final. I'm going to let them simply ship in 3.5.5 (eventually), even though 3.5.5 shouldn't have any "bug fixes". Maybe we'll be crisper when it comes to 3.6.xrc1 etc.

    @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.7 (EOL) end of life interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    6 participants