Reference leaks in subinterpreters

[pablogsal]

BPO	38962
Nosy	@vstinner, @ericsnowcurrently, @pablogsal, @miss-islington
PRs	bpo-38962: Fix reference leak in new_interpreter() #17453 bpo-38962: Fix reference leak in test_httpservers #17454 bpo-38962: Fix reference leak in the per-subinterpreter gc #17456 bpo-38962: Fix reference leak in the per-subinterpreter gc #17457

^{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 = 'https://github.com/vstinner'
closed_at = <Date 2019-12-04.12:12:28.455>
created_at = <Date 2019-12-03.19:55:50.781>
labels = ['tests', '3.9', 'performance']
title = 'Reference leaks in subinterpreters'
updated_at = <Date 2019-12-13.21:17:17.530>
user = 'https://github.com/pablogsal'

bugs.python.org fields:

activity = <Date 2019-12-13.21:17:17.530>
actor = 'eric.snow'
assignee = 'vstinner'
closed = True
closed_date = <Date 2019-12-04.12:12:28.455>
closer = 'vstinner'
components = ['Tests']
creation = <Date 2019-12-03.19:55:50.781>
creator = 'pablogsal'
dependencies = []
files = []
hgrepos = []
issue_num = 38962
keywords = ['patch']
message_count = 16.0
messages = ['357759', '357760', '357761', '357762', '357763', '357764', '357765', '357766', '357767', '357768', '357784', '357788', '357793', '357800', '357802', '358354']
nosy_count = 4.0
nosy_names = ['vstinner', 'eric.snow', 'pablogsal', 'miss-islington']
pr_nums = ['17453', '17454', '17456', '17457']
priority = 'high'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'resource usage'
url = 'https://bugs.python.org/issue38962'
versions = ['Python 3.9']

[pablogsal]

All the refleak build bots for master are reporting reference leaks in subinterpreter related tests:

https://buildbot.python.org/all/#/builders/126/builds/6/steps/5/logs/stdio
https://buildbot.python.org/all/#/builders/144/builds/6/steps/5/logs/stdio
https://buildbot.python.org/all/#/builders/206/builds/6
https://buildbot.python.org/all/#/builders/157/builds/6/steps/4/logs/stdio

......
test_atexit leaked [882, 882, 882] references, sum=2646
test_atexit leaked [12, 12, 12] memory blocks, sum=36
5 tests failed again:
test__xxsubinterpreters test_atexit test_capi test_httpservers
test_threading
== Tests result: FAILURE then FAILURE ==
401 tests OK.
10 slowest tests:

• test_asyncio: 33 min 34 sec
• test_concurrent_futures: 17 min 22 sec
• test_multiprocessing_spawn: 17 min 6 sec
• test_zipfile: 9 min 25 sec
• test_multiprocessing_forkserver: 9 min 2 sec
• test_multiprocessing_fork: 8 min 43 sec
• test_largefile: 7 min 32 sec
• test_lib2to3: 7 min 3 sec
• test_mailbox: 6 min 27 sec
• test_argparse: 5 min 5 sec
  5 tests failed:
  test__xxsubinterpreters test_atexit test_capi test_httpservers
  test_threading
  14 tests skipped:
  test_devpoll test_gdb test_ioctl test_kqueue test_msilib
  test_ossaudiodev test_startfile test_tix test_tk test_ttk_guionly
  test_winconsoleio test_winreg test_winsound test_zipfile64
  7 re-run tests:
  test__xxsubinterpreters test_atexit test_capi test_httpservers
  test_nntplib test_pty test_threading

Bisecting shows the following commit as the culprit:

ef5aa9a is the first bad commit
commit ef5aa9a
Author: Victor Stinner <vstinner@python.org>
Date: Wed Nov 20 00:38:03 2019 +0100

bpo-38858: Reorganize pycore_init_types() (GH-17265)

* Call _PyLong_Init() and _PyExc_Init() earlier
* new_interpreter() reuses pycore_init_types()

Python/pylifecycle.c | 31 +++++++++++--------------------
1 file changed, 11 insertions(+), 20 deletions(-)
bisect run success

Running * test.test_atexit.SubinterpreterTest.test_callbacks_leak is enough for reproducing the problem.

[pablogsal]

Actually, that commit has an independent refleak that was fixed (!) regarding the small integer cache. Seems that the actual commit that made the new leak is:

commit 2582d46
Author: Victor Stinner <vstinner@python.org>
Date: Fri Nov 22 19:24:49 2019 +0100

bpo-38858: new_interpreter() reuses pycore_init_builtins() (GH-17351)

new_interpreter() now calls _PyBuiltin_Init() to create the builtins
module and calls _PyImport_FixupBuiltin(), rather than using
_PyImport_FindBuiltin(tstate, "builtins").

pycore_init_builtins() is now responsible to initialize
intepr->builtins_copy: inline _PyImport_Init() and remove this
function.

Reverting this commit fixes the refleaks in atexit at least

[pablogsal]

Ok, the problem seems to be that _PyBuiltin_Init() returns a new reference that is not cleared once PyModule_GetDict() and _PyBuiltins_AddExceptions() is called.

[pablogsal]

Indeed, that fixes the problem in at least 2 modules:

./python -m test test_atexit -R :
0:00:00 load avg: 0.84 Run tests sequentially
0:00:00 load avg: 0.84 [1/1] test_atexit
beginning 9 repetitions
123456789
.........

== Tests result: SUCCESS ==

1 test OK.

Total duration: 1.5 sec
Tests result: SUCCESS

./python -m test test_capi -R :
0:00:00 load avg: 1.19 Run tests sequentially
0:00:00 load avg: 1.19 [1/1] test_capi
beginning 9 repetitions
123456789
..........
test_capi passed in 2 min 8 sec

== Tests result: SUCCESS ==

1 test OK.

Total duration: 2 min 8 sec
Tests result: SUCCESS

Although the rest of the tests still leak, so there is something else :(

[pablogsal]

Opened #17454 for the fixing the leak in http_servers

[pablogsal]

For the subinterpreter other leaks it seems that the commit that introduced the leak is:

7247407 is the first bad commit
commit 7247407
Author: Victor Stinner <vstinner@python.org>
Date: Wed Nov 20 12:25:50 2019 +0100

bpo-36854: Move _PyRuntimeState.gc to PyInterpreterState (GH-17287)

* Rename _PyGC_InitializeRuntime() to _PyGC_InitState()
* finalize_interp_clear() now also calls _PyGC_Fini() in
  subinterpreters (clear the GC state).

Include/internal/pycore_object.h | 2 +-
Include/internal/pycore_pymem.h | 2 +-
Include/internal/pycore_pystate.h | 7 +--
.../2019-11-20-12-01-37.bpo-36854.Zga_md.rst | 3 +
Modules/gcmodule.c | 72 ++++++++++++----------
Objects/object.c | 20 +++---
Python/pylifecycle.c | 3 +-
Python/pystate.c | 2 +-
8 files changed, 62 insertions(+), 49 deletions(-)
create mode 100644 Misc/NEWS.d/next/Core and Builtins/2019-11-20-12-01-37.bpo-36854.Zga_md.rst
bisect run success

[pablogsal]

Indeed, reverting commit 7247407 fixes the issues in test_threading

[pablogsal]

I created #17455 to revert commit 7247407 as it seems that the leak is more complex than the others.

If a solution is not found in a couple of days we need to merge it to fix the refleak buildbots.

[pablogsal]

This patch (which is wrong) fixes the reference issues:

diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c
index cce4783bc1..c354af18db 100644
--- a/Python/pylifecycle.c
+++ b/Python/pylifecycle.c
@@ -1276,6 +1276,7 @@ finalize_interp_clear(PyThreadState *tstate)
         _PyExc_Fini();
     }
 
+    PyGC_Collect();
     _PyGC_Fini(tstate);
 }

This suggests that the per-interpreter gc does not play well with the refleak checker.

[pablogsal]

Opened PR 17457 for the refleaks of test__xxsubinterpreters although I am not very convinced that we can call the GC at that point.

[vstinner]

New changeset 24f5cac by Victor Stinner (Pablo Galindo) in branch 'master':
bpo-38962: Fix reference leak in test_httpservers (GH-17454)
24f5cac