test__xxsubinterpreters leaked [3, 4, 3] memory blocks, sum=1

[pablogsal]

BPO	35984
Nosy	@vstinner, @ericsnowcurrently, @izbyshev, @pablogsal, @miss-islington
PRs	bpo-35984: _xxsubinterpreters: Fix memory leak in _channel_send() #11845

^{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/izbyshev'
closed_at = <Date 2019-02-15.22:35:12.462>
created_at = <Date 2019-02-13.11:10:03.433>
labels = ['3.8', 'tests', 'performance']
title = 'test__xxsubinterpreters leaked [3, 4, 3] memory blocks, sum=1'
updated_at = <Date 2019-02-15.22:35:12.462>
user = 'https://github.com/pablogsal'

bugs.python.org fields:

activity = <Date 2019-02-15.22:35:12.462>
actor = 'eric.snow'
assignee = 'izbyshev'
closed = True
closed_date = <Date 2019-02-15.22:35:12.462>
closer = 'eric.snow'
components = ['Tests']
creation = <Date 2019-02-13.11:10:03.433>
creator = 'pablogsal'
dependencies = []
files = []
hgrepos = []
issue_num = 35984
keywords = ['patch']
message_count = 11.0
messages = ['335408', '335409', '335411', '335412', '335414', '335421', '335461', '335462', '335477', '335652', '335653']
nosy_count = 5.0
nosy_names = ['vstinner', 'eric.snow', 'izbyshev', 'pablogsal', 'miss-islington']
pr_nums = ['11845']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'resource usage'
url = 'https://bugs.python.org/issue35984'
versions = ['Python 3.8']

[pablogsal]

The reafleak buildbots have detected memory block leaks in test__xxsubinterpreters:

Ran 112 tests in 4.721s
OK (skipped=5)
.
test__xxsubinterpreters leaked [3, 4, 3] memory blocks, sum=10
1 test failed again:
test__xxsubinterpreters
== Tests result: FAILURE then FAILURE ==

https://buildbot.python.org/all/#/builders/1/builds/502
https://buildbot.python.org/all/#/builders/80/builds/511

Bisecting shows 16f842d is the first bad commit. This was introduced in PR11822.

[vstinner]

Bisecting shows 16f842d is the first bad commit. This was introduced in PR11822.

That's issue bpo-35972.

[vstinner]

$ ./python -m test.bisect_cmd -R 3:3 test__xxsubinterpreters 

found:

test.test__xxsubinterpreters.ShareableTypeTests.test_non_shareable_int

[vstinner]

I'm mentoring Alexey Izbyshev.

@alexey: Do you want to work on this issue?

[izbyshev]

I'll look into it later today. An obvious guess is that my test simply exposed an existing leak because the exception code path wasn't tested before AFAIK, but I need to check it.

[vstinner]

I'll look into it later today. An obvious guess is that my test simply exposed an existing leak because the exception code path wasn't tested before AFAIK, but I need to check it.

Right. I don't think that your change introduced a regression.

[ericsnowcurrently]

Alexey, even though the refleak does not appear to be related to your earlier PR, you are welcome to keep working on this issue. :) If you do then please add me as a reviewer on whatever PR you make. Also, I'd be glad to answer any questions you have about the _xxsubinterpreters module. The more folks that understand that code, the better. :)

On the other hand, if you decide at this point that you'd rather do something else than track down refleaks I introduced then I'd totally understand. :) In that case feel free to assign this issue to me at that time.

[ericsnowcurrently]

FYI, the _xxsubinterpreters module serves several purposes. First, it allows us to more thoroughly test the subinterpreter functionality of CPython (doing so via _testembed and _testcapi is messy), effectively in test__xxsubinterpreters. Second, it is the foundation for helpers (which I intend on adding to test.support in the 3.8 time frame) for easily using subinterpreters in the test suite in lieu of subprocesses. Third, ultimately it will be the low-level implementation of PEP-554.

[izbyshev]

Thank you for your introduction about _xxsubinterpreters, Eric.

This particular leak is easy: it's right in _channel_send(). I've submitted a PR.

I've also done a quick scan of neighboring code, and it seems there are other leaks as well, e.g.:

• PyThread_free_lock() is not called at

  cpython/Modules/_xxsubinterpretersmodule.c

  Line 761 in dcb68f4

  PyMem_Free(chan);

  (and below)

• data is not released and freed at

  cpython/Modules/_xxsubinterpretersmodule.c

  Line 1387 in dcb68f4

  return NULL;

Do you think it'd make sense to go through the module to find and fix leaks? Or is this code in an early stage for such cleanup?

As a side note, such leaks should be easily found by static analyzers such as Coverity (assuming it understands CPython allocation functions like PyMem_NEW), so it might make sense to check out its reports on the module.

[miss-islington]

New changeset 3643322 by Miss Islington (bot) (Alexey Izbyshev) in branch 'master':
bpo-35984: _xxsubinterpreters: Fix memory leak in _channel_send() (GH-11845)
3643322

[ericsnowcurrently]

Thanks, Alexey. I've merged the PR.

As to finding and fixing leaks, I'd welcome any help you have to offer. :) Feel free to open an separate issue. One issue covering all the ones you find should be enough. If you find any non-trivial leaks or other issues then they should probably get separate issues. Thanks again!