classification
Title: test_threading leaked [38, 38, 38] references, sum=114
Type: Stage: resolved
Components: Tests Versions: Python 3.9
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: corona10, shihai1991, vstinner
Priority: normal Keywords: patch

Created on 2020-04-02 01:54 by vstinner, last changed 2020-04-27 12:58 by vstinner. This issue is now closed.

Files
File name Uploaded Description Edit
test_leak.py vstinner, 2020-04-02 02:14
Pull Requests
URL Status Linked Edit
PR 19412 merged vstinner, 2020-04-07 16:05
Messages (11)
msg365557 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-04-02 01:54
New changeset 53e4c91725083975598350877e2ed8e2d0194114 by Dong-hee Na in branch 'master':
bpo-40077: Convert _abc module to use PyType_FromSpec() (GH-19202)
https://github.com/python/cpython/commit/53e4c91725083975598350877e2ed8e2d0194114

This change introduced a reference leak:

$ ./python -m test -R 3:3 test_threading -m test.test_threading.SubinterpThreadingTests.test_threads_join_2
0:00:00 load avg: 1.52 Run tests sequentially
0:00:00 load avg: 1.52 [1/1] test_threading
beginning 6 repetitions
123456
......
test_threading leaked [19, 19, 19] references, sum=57
test_threading leaked [12, 12, 12] memory blocks, sum=36
test_threading failed

== Tests result: FAILURE ==

1 test failed:
    test_threading

Total duration: 768 ms
Tests result: FAILURE
msg365559 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-04-02 02:14
Attached test_leak.py is enough to reproduce the leak. It's related to subinterpreters and the _abc module.
msg365560 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-04-02 02:42
I'm not sure why, but trigger explicitly a second GC collection fix the issue.

diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c
index 2d5cb0ff78..d20ae01238 100644
--- a/Python/pylifecycle.c
+++ b/Python/pylifecycle.c
@@ -1295,6 +1295,7 @@ finalize_interp_clear(PyThreadState *tstate)
     /* Trigger a GC collection on subinterpreters*/
     if (!is_main_interp) {
         _PyGC_CollectNoFail();
+        _PyGC_CollectNoFail();
     }
 
     finalize_interp_types(tstate, is_main_interp);
msg365566 - (view) Author: Dong-hee Na (corona10) * (Python committer) Date: 2020-04-02 03:19
FYI,
first gc collect 772
secondary gc collect 4
msg365695 - (view) Author: Dong-hee Na (corona10) * (Python committer) Date: 2020-04-03 12:15
gc: collectable <type 0x7fc033c3cbd0>
gc: collectable <dict 0x104d79830>
gc: collectable <tuple 0x104d725a0>
gc: collectable <builtin_function_or_method 0x104d79890>

is not collected for the first time.
msg365705 - (view) Author: Dong-hee Na (corona10) * (Python committer) Date: 2020-04-03 15:52
Running 
from abc import ABCMeta
on the subinterpreter makes same size of leak.
msg365912 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-04-07 16:36
New changeset 9cc3ebd7e04cb645ac7b2f372eaafa7464e16b9c by Victor Stinner in branch 'master':
bpo-40149: Implement traverse in _abc._abc_data (GH-19412)
https://github.com/python/cpython/commit/9cc3ebd7e04cb645ac7b2f372eaafa7464e16b9c
msg365913 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-04-07 16:50
In _abcmodule_exec(), when _abc_data type is created, it's created with refcnt=3:
* 1 strong reference (normal)
* +1 ref from tp_dict['__new__'] slot
* +1 ref from tp_mro

type_traverse() visits tp_dict and tp_mro, so it's fine.

In Py_EndInterpreter(), PyInterpreterState_Clear() clears os.register_atfork() callbacks which were one of the last references to _abc_data type. The first call to _PyGC_CollectNoFail() destroys _abc_data *instances* but not the _abc_data type.

The following patch works around the issue:

diff --git a/Modules/_abc.c b/Modules/_abc.c
index 1efc98bf72..410dbcf96a 100644
--- a/Modules/_abc.c
+++ b/Modules/_abc.c
@@ -54,6 +54,7 @@ typedef struct {
 static int
 abc_data_traverse(_abc_data *self, visitproc visit, void *arg)
 {
+    Py_VISIT(Py_TYPE(self));
     Py_VISIT(self->_abc_registry);
     Py_VISIT(self->_abc_cache);
     Py_VISIT(self->_abc_negative_cache);


$ ./python -m test -R 3:3 test_threading -m test.test_threading.SubinterpThreadingTests.test_threads_join_2
(...)
Tests result: SUCCESS


I'm not sure why Py_VISIT(Py_TYPE(self)) is needed. Maybe it's a regression caused by commit 364f0b0f19cc3f0d5e63f571ec9163cf41c62958 of bpo-35810.

It sems like the GC doesn't take in account that instances of types allocated on the heap (if type->tp_flags has the Py_TPFLAGS_HEAPTYPE flag) hold a strong refeference to the type (PyObject.ob_type).

I created bpo-40217: "The garbage collector doesn't take in account that objects of heap allocated types hold a strong reference to their type".
msg365915 - (view) Author: Dong-hee Na (corona10) * (Python committer) Date: 2020-04-07 16:51
Wow Thank you for the summary :)
msg365938 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-04-07 20:41
> bpo-40149: Implement traverse in _abc._abc_data (GH-19412)

Pablo told me that this change is not correct: the overriden traverse function must call PyType_Type.tp_traverse (parent method).
msg367415 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-04-27 12:58
> I created bpo-40217: "The garbage collector doesn't take in account that objects of heap allocated types hold a strong reference to their type".

This issue is now fixed. I tested manually: I confirm that it does fix the test_threading leak, so I close the issue.

$ ./python -m test -R 3:3 test_threading
(...)
== Tests result: SUCCESS ==

1 test OK.

Total duration: 1 min 14 sec
Tests result: SUCCESS
History
Date User Action Args
2020-04-27 12:58:38vstinnersetstatus: open -> closed
resolution: fixed
messages: + msg367415

stage: patch review -> resolved
2020-04-07 20:41:47vstinnersetmessages: + msg365938
2020-04-07 16:58:08shihai1991setnosy: + shihai1991
2020-04-07 16:51:35corona10setmessages: + msg365915
2020-04-07 16:50:05vstinnersetmessages: + msg365913
2020-04-07 16:36:11vstinnersetmessages: + msg365912
2020-04-07 16:05:55vstinnersetkeywords: + patch
stage: patch review
pull_requests: + pull_request18772
2020-04-03 15:52:41corona10setmessages: + msg365705
2020-04-03 12:15:18corona10setmessages: + msg365695
2020-04-02 03:19:21corona10setmessages: + msg365566
2020-04-02 02:42:42vstinnersetmessages: + msg365560
2020-04-02 02:14:42vstinnersetfiles: + test_leak.py

messages: + msg365559
2020-04-02 01:54:05vstinnercreate