[Windows] SubinterpreterTest.test_callbacks_leak() of test_atexit leaks references

@pfmoore

BPO	30547
Nosy	@pfmoore, @vstinner, @tjguk, @ericsnowcurrently, @zware, @eryksun, @zooba, @matrixise
PRs	bpo-30547: Fix multiple reference leaks #1995

^{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 2017-06-08.12:29:33.238>
created_at = <Date 2017-06-02.08:58:24.734>
labels = ['3.7', 'tests', 'OS-windows', 'performance']
title = '[Windows] SubinterpreterTest.test_callbacks_leak() of test_atexit leaks references'
updated_at = <Date 2017-06-10.08:59:03.709>
user = 'https://github.com/vstinner'

bugs.python.org fields:

activity = <Date 2017-06-10.08:59:03.709>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2017-06-08.12:29:33.238>
closer = 'vstinner'
components = ['Tests', 'Windows']
creation = <Date 2017-06-02.08:58:24.734>
creator = 'vstinner'
dependencies = []
files = []
hgrepos = []
issue_num = 30547
keywords = []
message_count = 16.0
messages = ['294990', '294995', '295006', '295323', '295324', '295386', '295387', '295390', '295392', '295394', '295395', '295402', '295406', '295428', '295614', '295623']
nosy_count = 8.0
nosy_names = ['paul.moore', 'vstinner', 'tim.golden', 'eric.snow', 'zach.ware', 'eryksun', 'steve.dower', 'matrixise']
pr_nums = ['1995']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'resource usage'
url = 'https://bugs.python.org/issue30547'
versions = ['Python 3.7']

http://buildbot.python.org/all/builders/AMD64%20Windows8.1%20Refleaks%203.6/builds/15/steps/test/logs/stdio

0:02:58 [ 28/405/2] test_atexit failed
......
beginning 6 repetitions
123456

test_atexit leaked [12, 12, 12] references, sum=36
test_atexit leaked [4, 4, 4] memory blocks, sum=12

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.

@eric

The failing unit test was added by:

commit 2d350fd
Author: Antoine Pitrou <solipsis@pitrou.net>
Date: Thu Aug 1 20:56:12 2013 +0200

Issue bpo-18619: Fix atexit leaking callbacks registered from sub-interpreters, and make it GC-aware.

Using git bisect, I found that the leak was introduced by:

commit 6b4be19
Author: Eric Snow <ericsnowcurrently@gmail.com>
Date: Mon May 22 21:36:03 2017 -0700

bpo-22257: Small changes for PEP-432. (bpo-1728)

PEP-432 specifies a number of large changes to interpreter startup code, including exposing a cleaner C-API. The major changes depend on a number of smaller changes. This patch includes all those smaller changes.

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 bisect start

git checkout master
make && ./python -m test -R 3:3 -m test_callbacks_leak test_atexit
# test fails
git bisect bad # bad=test fails (ref leak)

git checkout 5d7a8d0
make && ./python -m test -R 3:3 -m test_callbacks_leak test_atexit
# test pass
git bisect good # good=test pass (no leak)

make && ./python -m test -R 3:3 -m test_callbacks_leak test_atexit
# git bisect good or bad depending on the test result

# ... 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 ;-)

At the end, you should get the commit 6b4be19.

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

SET_SYS_FROM_STRING_BORROW_INT_RESULT("warnoptions", warnoptions);

Oh, it seems like the regression was introduced by me in the commit 8fea252, see:
http://bugs.python.org/issue18520#msg201472

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().

New changeset ab1cb80 by Victor Stinner (Stéphane Wirtel) in branch 'master':
bpo-30547: Fix multiple reference leaks (bpo-1995)
ab1cb80

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 :-)

Navigation Menu

Repository navigation

[Windows] SubinterpreterTest.test_callbacks_leak() of test_atexit leaks references #74732

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

[Windows] SubinterpreterTest.test_callbacks_leak() of test_atexit leaks references #74732

Description

Activity

vstinner commented on Jun 2, 2017

vstinner commented on Jun 2, 2017

matrixise commented on Jun 2, 2017

vstinner commented on Jun 7, 2017

vstinner commented on Jun 7, 2017

vstinner commented on Jun 8, 2017

vstinner commented on Jun 8, 2017

matrixise commented on Jun 8, 2017

vstinner commented on Jun 8, 2017

vstinner commented on Jun 8, 2017

vstinner commented on Jun 8, 2017

vstinner commented on Jun 8, 2017

vstinner commented on Jun 8, 2017

matrixise commented on Jun 8, 2017

ericsnowcurrently commented on Jun 10, 2017

vstinner commented on Jun 10, 2017

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions