This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: test__xxsubinterpreters leaked [3, 4, 3] memory blocks, sum=1
Type: resource usage Stage: resolved
Components: Tests Versions: Python 3.8
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: izbyshev Nosy List: eric.snow, izbyshev, miss-islington, pablogsal, vstinner
Priority: normal Keywords: patch

Created on 2019-02-13 11:10 by pablogsal, last changed 2022-04-11 14:59 by admin. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 11845 merged izbyshev, 2019-02-13 21:08
Messages (11)
msg335408 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2019-02-13 11:10
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 16f842da3c862d76a1177bd8ef9579703c24fa5a is the first bad commit. This was introduced in PR11822.
msg335409 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-02-13 11:15
> Bisecting shows 16f842da3c862d76a1177bd8ef9579703c24fa5a is the first bad commit. This was introduced in PR11822.

That's issue bpo-35972.
msg335411 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-02-13 11:17
$ ./python -m test.bisect_cmd -R 3:3 test__xxsubinterpreters 

found:

test.test__xxsubinterpreters.ShareableTypeTests.test_non_shareable_int
msg335412 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-02-13 11:17
I'm mentoring Alexey Izbyshev.

@Alexey: Do you want to work on this issue?
msg335414 - (view) Author: Alexey Izbyshev (izbyshev) * (Python triager) Date: 2019-02-13 11:20
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.
msg335421 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-02-13 11:49
> 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.
msg335461 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2019-02-13 16:19
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.
msg335462 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2019-02-13 16:19
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.
msg335477 - (view) Author: Alexey Izbyshev (izbyshev) * (Python triager) Date: 2019-02-13 21:19
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 https://github.com/python/cpython/blob/dcb68f47f74b0cc8a1896d4a4c5a6b83c0bbeeae/Modules/_xxsubinterpretersmodule.c#L761 (and below)

* data is not released and freed at https://github.com/python/cpython/blob/dcb68f47f74b0cc8a1896d4a4c5a6b83c0bbeeae/Modules/_xxsubinterpretersmodule.c#L1387

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.
msg335652 - (view) Author: miss-islington (miss-islington) Date: 2019-02-15 22:29
New changeset 36433221f06d649dbd7e13f5fec948be8ffb90af by Miss Islington (bot) (Alexey Izbyshev) in branch 'master':
bpo-35984: _xxsubinterpreters: Fix memory leak in _channel_send() (GH-11845)
https://github.com/python/cpython/commit/36433221f06d649dbd7e13f5fec948be8ffb90af
msg335653 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2019-02-15 22:35
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!
History
Date User Action Args
2022-04-11 14:59:11adminsetgithub: 80165
2019-02-15 22:35:12eric.snowsetstatus: open -> closed
resolution: fixed
messages: + msg335653

stage: patch review -> resolved
2019-02-15 22:29:06miss-islingtonsetnosy: + miss-islington
messages: + msg335652
2019-02-13 21:19:23izbyshevsetmessages: + msg335477
2019-02-13 21:08:06izbyshevsetkeywords: + patch
stage: test needed -> patch review
pull_requests: + pull_request11876
2019-02-13 16:19:25eric.snowsetmessages: + msg335462
2019-02-13 16:19:15eric.snowsettype: resource usage
messages: + msg335461
stage: test needed
2019-02-13 11:49:43vstinnersetmessages: + msg335421
2019-02-13 11:20:59izbyshevsetassignee: izbyshev
messages: + msg335414
2019-02-13 11:17:53vstinnersetmessages: + msg335412
2019-02-13 11:17:17vstinnersetmessages: + msg335411
2019-02-13 11:16:09vstinnersetnosy: + izbyshev
2019-02-13 11:15:21vstinnersetmessages: + msg335409
2019-02-13 11:10:03pablogsalcreate