msg297940 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) *  |
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: Alyssa Coghlan (ncoghlan) *  |
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: Alyssa Coghlan (ncoghlan) *  |
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) *  |
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: Alyssa Coghlan (ncoghlan) *  |
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) *  |
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) *  |
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: Alyssa Coghlan (ncoghlan) *  |
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) *  |
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) *  |
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) *  |
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: Alyssa Coghlan (ncoghlan) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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: Alyssa Coghlan (ncoghlan) *  |
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) *  |
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.
|
|
Date |
User |
Action |
Args |
2022-04-11 14:58:48 | admin | set | github: 75059 |
2017-08-09 03:19:53 | larry | set | messages:
+ msg299964 |
2017-08-09 03:08:51 | ncoghlan | set | nosy:
+ doko, petr.viktorin messages:
+ msg299963
|
2017-08-07 21:12:03 | ned.deily | set | messages:
+ msg299871 |
2017-08-07 08:29:50 | larry | set | messages:
+ msg299833 |
2017-08-07 08:28:37 | serhiy.storchaka | set | messages:
+ msg299832 |
2017-08-07 07:47:49 | larry | set | nosy:
+ larry messages:
+ msg299828
|
2017-07-27 10:18:26 | serhiy.storchaka | set | status: open -> closed resolution: fixed stage: patch review -> resolved |
2017-07-27 10:16:23 | serhiy.storchaka | set | messages:
+ msg299304 |
2017-07-27 10:16:03 | serhiy.storchaka | set | messages:
+ msg299303 |
2017-07-27 09:48:27 | serhiy.storchaka | set | pull_requests:
+ pull_request2965 |
2017-07-27 09:43:08 | serhiy.storchaka | set | pull_requests:
+ pull_request2964 |
2017-07-27 09:24:40 | serhiy.storchaka | set | messages:
+ msg299297 |
2017-07-26 16:11:17 | vaultah | set | messages:
+ msg299251 |
2017-07-26 14:00:05 | vaultah | set | nosy:
+ vaultah messages:
+ msg299231
|
2017-07-24 18:20:45 | serhiy.storchaka | set | pull_requests:
+ pull_request2901 |
2017-07-24 10:22:07 | ned.deily | set | nosy:
+ ned.deily messages:
+ msg298950
|
2017-07-23 06:44:07 | serhiy.storchaka | set | messages:
+ msg298889 |
2017-07-16 04:44:27 | serhiy.storchaka | set | messages:
+ msg298417 |
2017-07-12 07:25:45 | serhiy.storchaka | set | pull_requests:
+ pull_request2744 |
2017-07-12 07:18:03 | serhiy.storchaka | set | messages:
+ msg298201 |
2017-07-12 06:56:24 | serhiy.storchaka | set | messages:
+ msg298200 |
2017-07-12 05:58:09 | serhiy.storchaka | set | pull_requests:
+ pull_request2743 |
2017-07-12 05:44:34 | ncoghlan | set | messages:
+ msg298196 |
2017-07-12 03:54:43 | serhiy.storchaka | set | messages:
+ msg298194 |
2017-07-12 03:50:05 | serhiy.storchaka | set | messages:
+ msg298193 |
2017-07-11 08:37:48 | vstinner | set | pull_requests:
+ pull_request2731 |
2017-07-09 14:25:08 | serhiy.storchaka | set | messages:
+ msg297992 stage: patch review |
2017-07-09 14:18:50 | serhiy.storchaka | set | pull_requests:
+ pull_request2704 |
2017-07-09 06:32:15 | ncoghlan | set | messages:
+ msg297986 versions:
- Python 2.7 |
2017-07-09 03:40:58 | serhiy.storchaka | set | title: SystemError on importing module that deletes itself from sys.modules -> SystemError on importing module from unloaded package |
2017-07-08 21:28:34 | ppperry | set | title: SystemError on import -> SystemError on importing module that deletes itself from sys.modules |
2017-07-08 18:36:40 | brett.cannon | set | messages:
+ msg297965 |
2017-07-08 18:33:37 | pitrou | set | nosy:
+ pitrou messages:
+ msg297964
|
2017-07-08 07:19:28 | ncoghlan | set | messages:
+ msg297947 |
2017-07-08 06:28:16 | serhiy.storchaka | set | messages:
+ msg297944 |
2017-07-08 06:19:30 | ncoghlan | set | messages:
+ msg297943 |
2017-07-08 06:14:28 | ncoghlan | set | messages:
+ msg297942 |
2017-07-08 05:07:16 | serhiy.storchaka | create | |