classification
Title: behavior of test.support.import_fresh_module
Type: behavior Stage: patch review
Components: Tests Versions: Python 3.3
process
Status: closed Resolution: duplicate
Dependencies: Superseder: module shutdown procedure based on GC
View: 812369
Assigned To: Nosy List: brett.cannon, eli.bendersky, ezio.melotti, flox, ncoghlan
Priority: normal Keywords: patch

Created on 2012-02-16 21:36 by flox, last changed 2012-06-15 07:21 by ncoghlan. This issue is now closed.

Files
File name Uploaded Description Edit
test_fresh_import.py flox, 2012-02-16 21:36
test_fresh_import2.py flox, 2012-02-16 23:44
issue14035_fresh_modules.diff flox, 2012-02-17 08:15 review
Messages (8)
msg153503 - (view) Author: Florent Xicluna (flox) * (Python committer) Date: 2012-02-16 21:36
While writing tests xml.etree, I hit a strange behaviour of import_fresh_module.

How to reproduce:

- dummy/__init__.py
- dummy/foo.py
- dummy/bar.py
- test_fresh_import.py


# 'dummy/foo.py'
from dummy.bar import func

# 'dummy/bar.py'
fortytwo = 42
def func():
    assert fortytwo == 42

# 'test_fresh_import.py'
(see attachment)


# Output:
~ $ ./python.exe test_fresh_import.py 
OK dummy.foo
OK dummy.bar
OK dummy.bar
OK dummy.foo
Traceback (most recent call last):
  File "test_fresh_import.py", line 24, in <module>
    test_fresh(m)
  File "test_fresh_import.py", line 5, in test_fresh
    rv = module.func()
  File "./dummy/bar.py", line 6, in func
    assert fortytwo == 42
AssertionError
msg153513 - (view) Author: Florent Xicluna (flox) * (Python committer) Date: 2012-02-16 23:44
Trigger the same behavior without import_fresh_module. (test_fresh_import2.py)

If you uncomment line #A or #B, it succeed.
On the other side, if you comment line #C or #D, it succeed too.

The import machinery is a bit complex, indeed ...
msg153519 - (view) Author: Florent Xicluna (flox) * (Python committer) Date: 2012-02-17 00:33
Keeping reference of fresh modules solves the issue.
msg153529 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2012-02-17 03:45
Keeping module references implicitly in import_fresh_module will leak references like crazy in the test suite. The onus is on the code referencing module contents to ensure that the module globals remain valid.

If we get rid of the explicit clearing of module globals to break reference cycles, this problem will also go away, so closing as a dupe of #812369.
msg153531 - (view) Author: Eli Bendersky (eli.bendersky) * (Python committer) Date: 2012-02-17 04:02
Florent, I can reproduce the problem by leaving just the last import_fresh_module in test_fresh_import.py

And your patch fixes it, although as Nick says it's problematic in terms of ref leaks.

What I'm not sure about is why the extra reference is needed. The modules *are* kept in sys.modules, after all.

IIUC, this is probably causing the strange failure I was having with test_xml_etree_c not being able to use the cET_alias global in some tests.
msg153534 - (view) Author: Florent Xicluna (flox) * (Python committer) Date: 2012-02-17 08:15
I have updated the patch to a simpler form where I copy() sys.modules.
Strangely, I do not detect any refleak running the tests.

I plan to commit this fix, if we don't have leaks in the test suite.
msg153540 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2012-02-17 12:20
In the case of *dependencies* that get refreshed, no they're *not* kept in sys.modules - they get overwritten by the originals when the sys.modules state gets restored.

The problem almost certainly arises because something, somewhere is doing "from x import y", where y is a function that depends on module globals in 'x'.

If 'x' ever drops out of sys.modules (e.g. because it is a fresh copy only there temporarily during an import), the x.__dict__ will have every attribute set to None and calls to 'y' will fail. (In Florent's original example, it was his "dummy/foo.py" that set of alarm bells and prompted me to look up the reference for the module GC problem).


That's why I'm opposed to touching import_fresh_modules to sweep this problem under the rug - as long as module globals finalisation isn't GC based, keeping a reference to a function in a module without ensuring you also hold a reference to the module itself is always going to be somewhat dubious.
msg162842 - (view) Author: Eli Bendersky (eli.bendersky) * (Python committer) Date: 2012-06-15 04:48
This looks rejected to me. Any opposition to closing the issue?
History
Date User Action Args
2012-06-15 07:21:34ncoghlansetstatus: open -> closed
2012-06-15 04:48:44eli.benderskysetmessages: + msg162842
2012-02-17 12:20:40ncoghlansetmessages: + msg153540
2012-02-17 08:15:19floxsetfiles: + issue14035_fresh_modules.diff
resolution: duplicate
messages: + msg153534
2012-02-17 08:11:37floxsetfiles: - issue14035_fresh_modules.diff
2012-02-17 04:02:36eli.benderskysetstatus: closed -> open
resolution: duplicate -> (no value)
messages: + msg153531
2012-02-17 03:45:22ncoghlansetstatus: open -> closed
resolution: duplicate
superseder: module shutdown procedure based on GC
messages: + msg153529
2012-02-17 00:33:18floxsetfiles: + issue14035_fresh_modules.diff
keywords: + patch
messages: + msg153519

stage: patch review
2012-02-16 23:44:51floxsetfiles: + test_fresh_import2.py
nosy: + brett.cannon
messages: + msg153513

2012-02-16 21:45:00floxsetnosy: + ezio.melotti
2012-02-16 21:36:06floxcreate