msg136729 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2011-05-24 10:07 |
Looks like either packaging or test_packaging forgets to clean up after itself:
results for 9a16fa0c9548 on branch "default"
--------------------------------------------
test_packaging leaked [193, 193, 193] references, sum=579
|
msg136737 - (view) |
Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * |
Date: 2011-05-24 11:23 |
Probably because new extension modules are built and imported on every run.
|
msg136738 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2011-05-24 11:26 |
> Probably because new extension modules are built and imported on every run.
Well, test_distutils does the same, doesn't it?
|
msg136741 - (view) |
Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * |
Date: 2011-05-24 11:37 |
I could not find any test in distutils/tests that imports extension modules.
|
msg136742 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2011-05-24 11:41 |
DistributionTestCase.test_hooks_get_run() leaks some references, I'm trying to understand where exactly.
|
msg136744 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2011-05-24 11:47 |
Let's see:
-> test_command_bdist_dumb.py leaks:
test_packaging leaked [7, 7] references, sum=14
-> test_dist.py leaks:
test_packaging leaked [65, 65] references, sum=130
-> test_mixin2to3.py leaks:
test_packaging leaked [60, 60] references, sum=120
-> test_pypi_dist.py leaks:
test_packaging leaked [28, 28] references, sum=56
-> test_pypi_simple.py leaks:
test_packaging leaked [12, 12] references, sum=24
-> test_uninstall.py leaks:
test_packaging leaked [5, 5] references, sum=10
-> test_util.py leaks:
test_packaging leaked [40, 40] references, sum=80
|
msg136745 - (view) |
Author: Roundup Robot (python-dev) |
Date: 2011-05-24 12:01 |
New changeset 70675864717b by Victor Stinner in branch 'default':
Issue #12167: packaging.tests.support, LoggingCatcher restores correctly the
http://hg.python.org/cpython/rev/70675864717b
New changeset 28c1f8480090 by Victor Stinner in branch 'default':
Issue #12167: packaging.tests.test_dist unloads the temporary module
http://hg.python.org/cpython/rev/28c1f8480090
|
msg136746 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2011-05-24 12:03 |
In test_command_bdist, the leak is in test_simple_built().
|
msg136747 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2011-05-24 12:06 |
In test_mixin2to3.py, the leak is shared between the 3 test methods.
|
msg136748 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2011-05-24 12:09 |
In test_pypi_dist, the leak is shared between test_download() and test_unpack().
|
msg138120 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2011-06-10 17:20 |
Is someone investigating this?
|
msg138124 - (view) |
Author: Éric Araujo (eric.araujo) * |
Date: 2011-06-10 17:49 |
I’m afraid I don’t understand enough to fix this. I looked at test_simple_built in test_command_bdist_dumb and found no C code involved.
|
msg138125 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2011-06-10 17:50 |
> I’m afraid I don’t understand enough to fix this. I looked at
> test_simple_built in test_command_bdist_dumb and found no C code
> involved.
It means that either packaging or test_packaging keeps references to
more and more objects.
You don't need to know C, but you have to investigate.
|
msg138126 - (view) |
Author: Éric Araujo (eric.araujo) * |
Date: 2011-06-10 17:52 |
Ah, it’s just refcounting issues? I can probably use the gc module to find them, and add del statements where appropriate to release objects.
|
msg138396 - (view) |
Author: Roundup Robot (python-dev) |
Date: 2011-06-15 22:01 |
New changeset fd6446a88fe3 by Victor Stinner in branch 'default':
Issue #12167: Fix a reafleak in packaging.tests.PyPIServer constructor
http://hg.python.org/cpython/rev/fd6446a88fe3
|
msg138398 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2011-06-15 22:04 |
test_dist and test_bdist_dumb leak because of the _path_created global variable of packaging.util, used indirectly by copy_tree().
# cache for by mkpath() -- in addition to cheapening redundant calls,
# eliminates redundant "creating /foo/bar/baz" messages in dry-run mode
_path_created = set()
I don't how/when this set should be cleared.
|
msg138399 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2011-06-15 22:05 |
Note: #12133 has a patch to fix the ResourceWarning of test_pypi_simple.
|
msg138530 - (view) |
Author: Roundup Robot (python-dev) |
Date: 2011-06-17 17:41 |
New changeset 27a70dfc38cc by Éric Araujo in branch 'default':
Packaging tests: don’t let an internal cache grow indefinitely.
http://hg.python.org/cpython/rev/27a70dfc38cc
|
msg138531 - (view) |
Author: Éric Araujo (eric.araujo) * |
Date: 2011-06-17 17:41 |
> I could not find any test in distutils/tests that imports extension modules.
test_build_ext builds and imports xx.
|
msg138533 - (view) |
Author: Éric Araujo (eric.araujo) * |
Date: 2011-06-17 17:43 |
We’re down to 100 refleaks. Thanks to the negative refleaks of test_pydoc, we now have a few refleaks to spare! Seriously, what does a negative refleak mean?
|
msg138543 - (view) |
Author: R. David Murray (r.david.murray) * |
Date: 2011-06-17 18:46 |
It means that objects were garbage collected. The refleak test runs the test multiple times, and ignores the first N runs to allow the object count to "settle". But sometimes it either doesn't settle, or later runs end up with objects getting garbage collected that were created in earlier runs.
|
msg138569 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2011-06-18 07:43 |
> test_build_ext builds and imports xx.
I changed test test to use a subprocess:
New changeset 144cea8db9a5 by Victor Stinner in branch 'default':
Issue #12333: run tests on the new module in a subprocess
http://hg.python.org/cpython/rev/144cea8db9a5
It should fix refleaks because it is not possible to unload a module written in C.
|
msg138596 - (view) |
Author: Éric Araujo (eric.araujo) * |
Date: 2011-06-18 20:49 |
> I changed test test to use a subprocess:
Yes, in packaging. I replied to an earlier question about distutils:
>>> I could not find any test in distutils/tests that imports extension modules.
>> test_build_ext builds and imports xx.
|
msg139917 - (view) |
Author: Andreas Stührk (Trundle) * |
Date: 2011-07-06 03:38 |
At least some of the remaining refleaks are caused by lib2to3. lib2to3 uses a logger with the filename as logger name (see `lib2to3.fixer_base.BaseFix.set_filename()`), but as the tests use a temporary file with an arbitrary name, a new logger is created on each test run iteration.
|
msg139973 - (view) |
Author: Andreas Stührk (Trundle) * |
Date: 2011-07-07 12:36 |
Attached is a patch that replaces `lib2to3.fixer_Base.BaseFix.set_filename()` during tests. With the patch applied, I don't get any refleaks for packaging.
Another approach would be to simply remove the logging attribute of lib2to3 fixers, as it isn't used anyway.
|
msg139984 - (view) |
Author: Éric Araujo (eric.araujo) * |
Date: 2011-07-07 17:42 |
Thanks a lot for the diagnosis.
To fix it, I don’t find the monkey-patching approach very good, I’d prefer properly using the logging API to remove handlers upon cleanup. Would you like to look into that?
|
msg139985 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2011-07-07 17:46 |
> To fix it, I don’t find the monkey-patching approach very good, I’d
> prefer properly using the logging API to remove handlers upon cleanup.
I don't think such stuff exists.
|
msg139986 - (view) |
Author: Éric Araujo (eric.araujo) * |
Date: 2011-07-07 17:56 |
See http://hg.python.org/cpython/file/tip/Lib/packaging/tests/support.py#l81
|
msg139987 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2011-07-07 17:57 |
> See http://hg.python.org/cpython/file/tip/Lib/packaging/tests/support.py#l81
AFAIU, the problem is that 2to3 creates a whole new logger for each
different file. So it's not about cleaning the handlers, but cleaning up
the loggers as well. And logging (deliberately, according to Vinay)
doesn't provide any API to destroy loggers (they are de facto eternal).
|
msg139988 - (view) |
Author: Éric Araujo (eric.araujo) * |
Date: 2011-07-07 17:59 |
Thanks for clarifying, I had misread. IMO, using one logger per file is a lib2to3 bug.
|
msg140044 - (view) |
Author: Vinay Sajip (vinay.sajip) * |
Date: 2011-07-08 23:18 |
I can confirm what Andreas said - I just removed the two lines referencing the logger in BaseFix, and ran the regression tests - with no failures. So I, too, think those lines should go.
|
msg140121 - (view) |
Author: Éric Araujo (eric.araujo) * |
Date: 2011-07-11 13:08 |
Let me clarify my position: I think there is a bug in lib2to3 that should be fixed, after what the refleak would go. (I’ll report it soon if nobody does it before.) If Benjamin rejects the bug, then I’ll agree with the monkey-patch.
|
msg140156 - (view) |
Author: Vinay Sajip (vinay.sajip) * |
Date: 2011-07-11 18:00 |
I've created #12536 to cover the lib2to3 issue.
|
msg140428 - (view) |
Author: Éric Araujo (eric.araujo) * |
Date: 2011-07-15 15:54 |
It looks like we don’t have refleaks anymore! I still have one question.
Victor reported some time ago that packaging.util._path_created was a cache that was never cleared; I fixed that in 27a70dfc38cc, but recently I’ve found that regrtest itself clears the similar distutils.util._path_created; I wonder which approach is better: one global cleaning in regrtest or piecemeal cleanup in each leaking test case?
I’ve also made a patch to register the caches used by packaging.database with the regrtest unclean environment warning; can someone review it?
|
msg140451 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2011-07-15 18:16 |
> I’ve also made a patch to register the caches used by
> packaging.database with the regrtest unclean environment warning; can
> someone review it?
I would call .copy() on the original dicts rather than remembering an
explicit empty dict.
|
msg140576 - (view) |
Author: Éric Araujo (eric.araujo) * |
Date: 2011-07-18 13:44 |
> I would call .copy() on the original dicts rather than remembering an
> explicit empty dict.
I thought about that and decided to use an empty dict as a way to add a check that the caches should start empty. Maybe it was misguided and I should instead add a unit test for that, or not bother altogether.
|
msg141385 - (view) |
Author: Éric Araujo (eric.araujo) * |
Date: 2011-07-29 14:48 |
I edited my patch to use a copy instead of an explicit empty dict, but I found a bug. The restore code unpacks the saved_caches object to (cache, items), but saved_caches is (id(cache), cache, cache.copy()). I’m surprised the unpacking works; I don’t want to commit before I understand that.
|
msg144994 - (view) |
Author: Roundup Robot (python-dev) |
Date: 2011-10-06 11:24 |
New changeset e76c6aaff135 by Éric Araujo in branch 'default':
Add regrtest check for caches in packaging.database (see #12167)
http://hg.python.org/cpython/rev/e76c6aaff135
|
|
Date |
User |
Action |
Args |
2022-04-11 14:57:17 | admin | set | github: 56376 |
2011-10-06 11:44:28 | eric.araujo | set | status: open -> closed resolution: fixed stage: needs patch -> resolved |
2011-10-06 11:24:04 | python-dev | set | messages:
+ msg144994 |
2011-07-29 14:48:56 | eric.araujo | set | messages:
+ msg141385 |
2011-07-18 13:44:11 | eric.araujo | set | messages:
+ msg140576 |
2011-07-15 18:16:56 | pitrou | set | messages:
+ msg140451 |
2011-07-15 15:55:00 | eric.araujo | set | files:
+ packaging-caches.diff
messages:
+ msg140428 |
2011-07-11 18:00:01 | vinay.sajip | set | messages:
+ msg140156 |
2011-07-11 13:08:11 | eric.araujo | set | assignee: tarek -> eric.araujo messages:
+ msg140121 |
2011-07-08 23:18:33 | vinay.sajip | set | nosy:
+ vinay.sajip messages:
+ msg140044
|
2011-07-07 17:59:51 | eric.araujo | set | messages:
+ msg139988 |
2011-07-07 17:57:42 | pitrou | set | messages:
+ msg139987 |
2011-07-07 17:56:25 | eric.araujo | set | messages:
+ msg139986 |
2011-07-07 17:46:15 | pitrou | set | messages:
+ msg139985 |
2011-07-07 17:42:59 | eric.araujo | set | messages:
+ msg139984 |
2011-07-07 12:36:08 | Trundle | set | files:
+ issue12167_patch_2to3.patch
nosy:
+ benjamin.peterson messages:
+ msg139973
keywords:
+ patch |
2011-07-06 03:38:43 | Trundle | set | nosy:
+ Trundle messages:
+ msg139917
|
2011-06-18 20:49:40 | eric.araujo | set | messages:
+ msg138596 |
2011-06-18 07:43:17 | vstinner | set | messages:
+ msg138569 |
2011-06-17 18:46:12 | r.david.murray | set | nosy:
+ r.david.murray messages:
+ msg138543
|
2011-06-17 17:43:45 | eric.araujo | set | messages:
+ msg138533 |
2011-06-17 17:41:57 | eric.araujo | set | messages:
+ msg138531 |
2011-06-17 17:41:35 | python-dev | set | messages:
+ msg138530 |
2011-06-15 22:05:13 | vstinner | set | messages:
+ msg138399 |
2011-06-15 22:04:04 | vstinner | set | messages:
+ msg138398 |
2011-06-15 22:01:09 | python-dev | set | messages:
+ msg138396 |
2011-06-10 17:52:30 | eric.araujo | set | messages:
+ msg138126 |
2011-06-10 17:50:31 | pitrou | set | messages:
+ msg138125 |
2011-06-10 17:49:39 | eric.araujo | set | messages:
+ msg138124 |
2011-06-10 17:20:33 | pitrou | set | messages:
+ msg138120 |
2011-05-24 12:09:10 | pitrou | set | messages:
+ msg136748 |
2011-05-24 12:06:42 | pitrou | set | messages:
+ msg136747 |
2011-05-24 12:03:32 | pitrou | set | messages:
+ msg136746 |
2011-05-24 12:01:54 | python-dev | set | nosy:
+ python-dev messages:
+ msg136745
|
2011-05-24 11:47:49 | pitrou | set | messages:
+ msg136744 |
2011-05-24 11:41:03 | vstinner | set | nosy:
+ vstinner messages:
+ msg136742
|
2011-05-24 11:37:41 | amaury.forgeotdarc | set | messages:
+ msg136741 |
2011-05-24 11:26:18 | pitrou | set | messages:
+ msg136738 |
2011-05-24 11:23:17 | amaury.forgeotdarc | set | nosy:
+ amaury.forgeotdarc messages:
+ msg136737
|
2011-05-24 10:53:30 | nadeem.vawda | set | nosy:
+ nadeem.vawda
|
2011-05-24 10:07:25 | pitrou | create | |