classification
Title: SystemError on importing module from unloaded package
Type: behavior Stage: resolved
Components: Interpreter Core Versions: Python 3.7, Python 3.6, Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: brett.cannon, doko, encukou, eric.snow, larry, ncoghlan, ned.deily, pitrou, serhiy.storchaka, vaultah
Priority: normal Keywords:

Created on 2017-07-08 05:07 by serhiy.storchaka, last changed 2017-08-09 03:19 by larry. This issue is now closed.

Files
File name Uploaded Description Edit
import-systemerror.zip serhiy.storchaka, 2017-07-08 05:07
Pull Requests
URL Status Linked Edit
PR 2639 merged serhiy.storchaka, 2017-07-09 14:18
PR 2665 haypo, 2017-07-11 08:37
PR 2676 merged serhiy.storchaka, 2017-07-12 05:58
PR 2677 merged serhiy.storchaka, 2017-07-12 07:25
PR 2851 merged serhiy.storchaka, 2017-07-24 18:20
PR 2912 merged serhiy.storchaka, 2017-07-27 09:43
PR 2913 merged serhiy.storchaka, 2017-07-27 09:48
Messages (28)
msg297940 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-07-08 05:07
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).
msg297942 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2017-07-08 06:14
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.
msg297943 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2017-07-08 06:19
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.
msg297944 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-07-08 06:28
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.
msg297947 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2017-07-08 07:19
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.
msg297964 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2017-07-08 18:33
I concur with Serhiy: SystemError is a smell that C code isn't taking appropriate precautions before dealing with user code or data.
msg297965 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2017-07-08 18:36
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.
msg297986 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2017-07-09 06:32
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.
msg297992 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-07-09 14:25
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".
msg298193 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-07-12 03:50
New changeset 8a9cd20edca7d01b68292036029ae3735ce65edd by Serhiy Storchaka in branch 'master':
bpo-30876: Relative import from unloaded package now reimports the package (#2639)
https://github.com/python/cpython/commit/8a9cd20edca7d01b68292036029ae3735ce65edd
msg298194 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-07-12 03:54
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.
msg298196 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2017-07-12 05:44
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.
msg298200 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-07-12 06:56
When backported issue30876 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 issue26367, but only in the 3.6 branch. The original example in issue26367 still returns a module with a wrong name in 3.5.

Why issue26367 changes were applied to 3.5 only partially? I afraid that the absence of this check may interfere with removing SystemError.
msg298201 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-07-12 07:18
Ah, this is because issue18018 was fixed only in 3.6. Issue18018 is similar to this issue, it is about confusing SystemError. As a side effect it solved an example from issue26367 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.
msg298417 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-07-16 04:44
New changeset 28343e3392ca7b1ec7151f68d4d92c90efb91e50 by Serhiy Storchaka in branch '3.6':
[3.6] bpo-30876: Relative import from unloaded package now reimports the package (GH-2639) (#2676)
https://github.com/python/cpython/commit/28343e3392ca7b1ec7151f68d4d92c90efb91e50
msg298889 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-07-23 06:44
New changeset c824cc8426a16dd9f3949a3ed135523d37787bae by Serhiy Storchaka in branch '3.5':
[3.5] Backport bpo-30876 (GH-2639), bpo-18018 and bpo-26367. (#2677)
https://github.com/python/cpython/commit/c824cc8426a16dd9f3949a3ed135523d37787bae
msg298950 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2017-07-24 10:22
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'
msg299231 - (view) Author: Dmitry Kazakov (vaultah) * Date: 2017-07-26 14:00
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?
msg299251 - (view) Author: Dmitry Kazakov (vaultah) * Date: 2017-07-26 16:11
Please ignore my last message, I didn't notice the existing pull request... Sorry.
msg299297 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-07-27 09:24
New changeset d5ed47dea25e04a3a144eddf99a4ac4a29242dbc by Serhiy Storchaka in branch 'master':
bpo-30814, bpo-30876: Add new import test files to projects. (#2851)
https://github.com/python/cpython/commit/d5ed47dea25e04a3a144eddf99a4ac4a29242dbc
msg299303 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-07-27 10:16
New changeset 95b16a9705d6b4d31c016c014e59744fc33d53ea by Serhiy Storchaka in branch '3.6':
[3.6] bpo-30814, bpo-30876: Add new import test files to projects. (GH-2851). (#2912)
https://github.com/python/cpython/commit/95b16a9705d6b4d31c016c014e59744fc33d53ea
msg299304 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-07-27 10:16
New changeset f9fbed19a964e55ee703005823d8a7408f83d7f4 by Serhiy Storchaka in branch '3.5':
[3.5] bpo-30876: Add new import test files to projects. (GH-2851). (#2913)
https://github.com/python/cpython/commit/f9fbed19a964e55ee703005823d8a7408f83d7f4
msg299828 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2017-08-07 07:47
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 (19b2890014d3098147d16475c492a47a43893768) built and tested successfully on July 26.  But once this revision (f9fbed19a964e55ee703005823d8a7408f83d7f4) 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.
msg299832 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-08-07 08:28
This is a FreeBSD bug. It isn't related to this change. See issue31044 for details.
msg299833 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2017-08-07 08:29
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 ;-)
msg299871 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2017-08-07 21:12
Just to clarify:  the revision added since 3.5.4rc1, f9fbed19a964e55ee703005823d8a7408f83d7f4 ([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.
msg299963 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2017-08-09 03:08
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.
msg299964 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2017-08-09 03:19
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.
History
Date User Action Args
2017-08-09 03:19:53larrysetmessages: + msg299964
2017-08-09 03:08:51ncoghlansetnosy: + doko, encukou
messages: + msg299963
2017-08-07 21:12:03ned.deilysetmessages: + msg299871
2017-08-07 08:29:50larrysetmessages: + msg299833
2017-08-07 08:28:37serhiy.storchakasetmessages: + msg299832
2017-08-07 07:47:49larrysetnosy: + larry
messages: + msg299828
2017-07-27 10:18:26serhiy.storchakasetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2017-07-27 10:16:23serhiy.storchakasetmessages: + msg299304
2017-07-27 10:16:03serhiy.storchakasetmessages: + msg299303
2017-07-27 09:48:27serhiy.storchakasetpull_requests: + pull_request2965
2017-07-27 09:43:08serhiy.storchakasetpull_requests: + pull_request2964
2017-07-27 09:24:40serhiy.storchakasetmessages: + msg299297
2017-07-26 16:11:17vaultahsetmessages: + msg299251
2017-07-26 14:00:05vaultahsetnosy: + vaultah
messages: + msg299231
2017-07-24 18:20:45serhiy.storchakasetpull_requests: + pull_request2901
2017-07-24 10:22:07ned.deilysetnosy: + ned.deily
messages: + msg298950
2017-07-23 06:44:07serhiy.storchakasetmessages: + msg298889
2017-07-16 04:44:27serhiy.storchakasetmessages: + msg298417
2017-07-12 07:25:45serhiy.storchakasetpull_requests: + pull_request2744
2017-07-12 07:18:03serhiy.storchakasetmessages: + msg298201
2017-07-12 06:56:24serhiy.storchakasetmessages: + msg298200
2017-07-12 05:58:09serhiy.storchakasetpull_requests: + pull_request2743
2017-07-12 05:44:34ncoghlansetmessages: + msg298196
2017-07-12 03:54:43serhiy.storchakasetmessages: + msg298194
2017-07-12 03:50:05serhiy.storchakasetmessages: + msg298193
2017-07-11 08:37:48hayposetpull_requests: + pull_request2731
2017-07-09 14:25:08serhiy.storchakasetmessages: + msg297992
stage: patch review
2017-07-09 14:18:50serhiy.storchakasetpull_requests: + pull_request2704
2017-07-09 06:32:15ncoghlansetmessages: + msg297986
versions: - Python 2.7
2017-07-09 03:40:58serhiy.storchakasettitle: SystemError on importing module that deletes itself from sys.modules -> SystemError on importing module from unloaded package
2017-07-08 21:28:34ppperrysettitle: SystemError on import -> SystemError on importing module that deletes itself from sys.modules
2017-07-08 18:36:40brett.cannonsetmessages: + msg297965
2017-07-08 18:33:37pitrousetnosy: + pitrou
messages: + msg297964
2017-07-08 07:19:28ncoghlansetmessages: + msg297947
2017-07-08 06:28:16serhiy.storchakasetmessages: + msg297944
2017-07-08 06:19:30ncoghlansetmessages: + msg297943
2017-07-08 06:14:28ncoghlansetmessages: + msg297942
2017-07-08 05:07:16serhiy.storchakacreate