classification
Title: [Windows] SubinterpreterTest.test_callbacks_leak() of test_atexit leaks references
Type: resource usage Stage: resolved
Components: Tests, Windows Versions: Python 3.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: eric.snow, eryksun, matrixise, paul.moore, steve.dower, tim.golden, vstinner, zach.ware
Priority: normal Keywords:

Created on 2017-06-02 08:58 by vstinner, last changed 2017-06-10 08:59 by vstinner. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 1995 merged matrixise, 2017-06-08 09:55
Messages (16)
msg294990 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-06-02 08:58
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.
msg294995 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-06-02 09:07
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 ;-)
msg295006 - (view) Author: Stéphane Wirtel (matrixise) * (Python committer) Date: 2017-06-02 11:16
this issue can be executed on Linux, I think I am going to work on this one.
msg295323 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-06-07 09:06
The failing unit test was added by:

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

    Issue #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 6b4be195cd8868b76eb6fbe166acc39beee8ce36
Author: Eric Snow <ericsnowcurrently@gmail.com>
Date:   Mon May 22 21:36:03 2017 -0700

    bpo-22257: Small changes for PEP 432. (#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: 5d7a8d0c13737fd531b722ad76c505ef47aac96a (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 5d7a8d0c13737fd531b722ad76c505ef47aac96a
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 6b4be195cd8868b76eb6fbe166acc39beee8ce36.


@Eric Snow: Please don't fix the bug, please explain how to fix it ;-)
msg295324 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-06-07 09:16
> At the end, you should get the commit 6b4be195cd8868b76eb6fbe166acc39beee8ce36.

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?
msg295386 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-06-08 08:38
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).
msg295387 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-06-08 09:02
Oh, 1abcf6700b4da6207fe859de40c6c1bada6b4fec introduced two more reference leaks.
msg295390 - (view) Author: Stéphane Wirtel (matrixise) * (Python committer) Date: 2017-06-08 10:02
I have pushed a PR, if you can check it. Thanks
msg295392 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-06-08 10:09
-    SET_SYS_FROM_STRING_BORROW_INT_RESULT("warnoptions", warnoptions); 

Oh, it seems like the regression was introduced by me in the commit 8fea252a507024edf00d5d98881d22dc8799a8d3, 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.
msg295394 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-06-08 10:21
bpo-30536 has been marked as a duplicate of this issue: [EASY] SubinterpThreadingTests.test_threads_join_2() of test_threading leaks references.
msg295395 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-06-08 10:21
Sorry, that issue wasn't [EASY] at all! But I tried to help Stéphane & Louie as much as I could ;-)
msg295402 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-06-08 10:50
I created bpo-30598: Py_NewInterpreter() leaks a reference on warnoptions in _PySys_EndInit().
msg295406 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-06-08 11:13
New changeset ab1cb80b435a34e4f908c97cd2f3a7fe8add6505 by Victor Stinner (Stéphane Wirtel) in branch 'master':
bpo-30547: Fix multiple reference leaks (#1995)
https://github.com/python/cpython/commit/ab1cb80b435a34e4f908c97cd2f3a7fe8add6505
msg295428 - (view) Author: Stéphane Wirtel (matrixise) * (Python committer) Date: 2017-06-08 12:26
3.6 is not affected by this issue.
msg295614 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2017-06-10 06:26
Thanks for taking care of this, Victor, Stéphane & Louie!
msg295623 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-06-10 08:59
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 :-)
History
Date User Action Args
2017-06-10 08:59:03vstinnersetmessages: + msg295623
2017-06-10 06:26:18eric.snowsetmessages: + msg295614
2017-06-08 12:29:33vstinnersetstatus: open -> closed
resolution: fixed
stage: resolved
2017-06-08 12:26:30matrixisesetmessages: + msg295428
2017-06-08 11:13:22vstinnersetmessages: + msg295406
2017-06-08 10:50:49vstinnersetmessages: + msg295402
2017-06-08 10:47:04serhiy.storchakalinkissue30536 superseder
2017-06-08 10:21:38vstinnersetkeywords: - easy (C)

messages: + msg295395
title: [EASY][Windows] SubinterpreterTest.test_callbacks_leak() of test_atexit leaks references -> [Windows] SubinterpreterTest.test_callbacks_leak() of test_atexit leaks references
2017-06-08 10:21:07vstinnersetmessages: + msg295394
2017-06-08 10:09:29vstinnersetmessages: + msg295392
2017-06-08 10:02:30matrixisesetmessages: + msg295390
2017-06-08 09:55:51matrixisesetpull_requests: + pull_request2059
2017-06-08 09:02:52vstinnersetmessages: + msg295387
2017-06-08 08:38:46vstinnersetmessages: + msg295386
2017-06-07 09:16:33vstinnersetmessages: + msg295324
2017-06-07 09:06:15vstinnersetnosy: + eric.snow
messages: + msg295323
2017-06-02 11:16:27matrixisesetnosy: + matrixise
messages: + msg295006
2017-06-02 09:07:10vstinnersetkeywords: + easy (C)

messages: + msg294995
title: [Windows] SubinterpreterTest.test_callbacks_leak() of test_atexit leaks references -> [EASY][Windows] SubinterpreterTest.test_callbacks_leak() of test_atexit leaks references
2017-06-02 08:58:24vstinnercreate