New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Windows] SubinterpreterTest.test_callbacks_leak() of test_atexit leaks references #74732
Comments
0:02:58 [ 28/405/2] test_atexit failed test_atexit leaked [12, 12, 12] references, sum=36 Code of of the test: def test_callbacks_leak(self):
# This test shows a leak in refleak mode if atexit doesn't
# take care to free callbacks in its per-subinterpreter module
# state.
n = atexit._ncallbacks()
code = r"""if 1:
import atexit
def f():
pass
atexit.register(f)
del atexit
"""
ret = support.run_in_subinterp(code)
self.assertEqual(ret, 0)
self.assertEqual(atexit._ncallbacks(), n) Hum, I don't understand: the test leaks references on purpose? To reproduce the bug, use the command: python -m test -R 3:3 -m test_callbacks_leak test_atexit Note: Leak isolated my bisect_test.py of bpo-29512. |
I tagged this issue as easy. I consider that it's a good exercice for new contributors. Core developers: please let new contributors try to fix it since this issue is not critical, try to explain how to fix it rather than writing the fix ;-) |
this issue can be executed on Linux, I think I am going to work on this one. |
The failing unit test was added by: commit 2d350fd
Using git bisect, I found that the leak was introduced by: commit 6b4be19
To run a git bisection, start with an old commit, 1 month ago: 5d7a8d0 (May, 1). Spoiler: the test doesn't leak at this bisection. git bisect reset git checkout master git checkout 5d7a8d0 make && ./python -m test -R 3:3 -m test_callbacks_leak test_atexit # ... continue until git bisect finds the commit ... At the end, you should get the commit 6b4be19. @eric Snow: Please don't fix the bug, please explain how to fix it ;-) |
The commit is a giant change. So let me help you, the following change is strange. value is replaced whereas its value is non-NULL... Maybe it's the regression? ;-) diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c
index 90f8551..03601ea 100644
--- a/Python/pylifecycle.c
+++ b/Python/pylifecycle.c
@@ -291,6 +291,9 @@ import_init(PyInterpreterState *interp, PyObject *sysmod)
/* Install importlib as the implementation of import */
value = PyObject_CallMethod(importlib, "_install", "OO", sysmod, impmod);
+ if (value != NULL)
+ value = PyObject_CallMethod(importlib,
+ "_install_external_importers", "");
if (value == NULL) {
PyErr_Print();
Py_FatalError("Py_Initialize: importlib install failed"); Stéphane Wirtel (matrixise): "this issue can be executed on Linux, I think I am going to work on this one." Would you like to work on a patch? |
SubinterpreterTest.test_subinterps of test_capi also leaks. But it is likely the same bug than this issue (SubinterpreterTest.test_callbacks_leak() of test_atexit leaks). |
Oh, 1abcf67 introduced two more reference leaks. |
I have pushed a PR, if you can check it. Thanks |
Oh, it seems like the regression was introduced by me in the commit 8fea252, see: Or maybe it comes from recent changes made by Eric Snow and Nick Coghlan related to Python initiallization (PEP-432). I don't know. |
bpo-30536 has been marked as a duplicate of this issue: [EASY] SubinterpThreadingTests.test_threads_join_2() of test_threading leaks references. |
Sorry, that issue wasn't [EASY] at all! But I tried to help Stéphane & Louie as much as I could ;-) |
I created bpo-30598: Py_NewInterpreter() leaks a reference on warnoptions in _PySys_EndInit(). |
3.6 is not affected by this issue. |
Thanks for taking care of this, Victor, Stéphane & Louie! |
Eric Snow: "Thanks for taking care of this, Victor, Stéphane & Louie!" You're welcome. FYI I'm trying to look more closely at the new Refleaks buildbot, so now it should become easier to catch regressions. By the way, I like the work you did with Nick. Even if I don't understand everything, I know that we have to do it :-) |
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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: