Clean modules in the reversed order

[serhiy-storchaka]

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]

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?

[ncoghlan]

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]

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]

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]

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]

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]

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