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.

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

That's issue bpo-35972.

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

found:

test.test__xxsubinterpreters.ShareableTypeTests.test_non_shareable_int

I'm mentoring Alexey Izbyshev.

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

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.

> 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.

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.

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.

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.

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

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!

stage: patch review -> resolved