GeezerVison

🤖 New build scheduled with the buildbot fleet by @kumaraditya303 for commit 100ad99 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

Refleaks bots are failing because of #94330

@kumaraditya303, thanks for the PR. One of the most important aims of PEP-687 is to communicate the changes before they happen. See Part 1: Preparation. I'd prefer if you please re-read the PEP and follow its guidelines as specified. You'll notice that opening a PR comes in the second half of the process.

One of the most important aims of PEP-687 is to communicate the changes before they happen. See Part 1: Preparation. I'd prefer if you please re-read the PEP and follow its guidelines as specified. You'll notice that opening a PR comes in the second half of the process.

I'll create an issue to discuss it but I don't expect much as the module has no global state and the implemented types are not performance critical. I had worked on this months ago just was waiting for PEP to get accepted :)

[...] as the module has no global state and the implemented types are not performance critical [...]

If the types do not access global module state they should remain static; this is explicitly noted in the PEP.

I'll create an issue to discuss it but I don't expect much [...]

Well, the important thing is that it gets done. Lack of communication is the single one issue that has contributed to heating the discussions regarding these changes. Even if we expect few people to participate in such a topic, it is of importance to raise awareness (and then wait at least for a week or two before continuing).

@erlend-aasland: Let's continue the discussion on the issue #94382

Removing do-not-merge as no alternative PR has been proposed so far.

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

And if you don't make the requested changes, you will be poked with soft cushions!

Removing do-not-merge as no alternative PR has been proposed so far.

Well, only three hours passed. That's a little narrow time-frame, don't you agree? :)

As of Petr's last comment, please re-label this with do-not-merge until there is an agreement.

Please add a NEWS entry explaining that the purpose of this PR is to fix memory leaks when the _multiprocessing extension module is loaded/unloaded multiple times: #94382 (comment)

This means I can remove do-not-merge, right ?

This means I can remove do-not-merge, right ?

That's unrelated. @erlend-aasland wrote:

As of Petr's last comment, please re-label this with do-not-merge until there is an agreement.

Since @erlend-aasland and @encukou wrote PEP 687, I understand that you need to agree on that specific PR.

Sorry, I'm no longer convinced that this PR fix an actual memory leak. The NEWS entry no looks wrong to me.

Test from #94382 (comment):

from test import support
import sys
for i in range(1, 10):
    support.run_in_subinterp("import multiprocessing")
    print("refs:", sys.gettotalrefcount(), "blocks: ", sys.getallocatedblocks())

Without this PR, Python leaks memory: refs +469, blocks +160.

refs: 162138 blocks:  47456
refs: 162201 blocks:  47476
refs: 162259 blocks:  47496
refs: 162317 blocks:  47516
refs: 162375 blocks:  47536
refs: 162433 blocks:  47556
refs: 162491 blocks:  47576
refs: 162549 blocks:  47596
refs: 162607 blocks:  47616

With this PR (rebased to main), Python still leaks memory: refs +469, blocks +160 (no change).

refs: 162138 blocks:  47446
refs: 162201 blocks:  47466
refs: 162259 blocks:  47486
refs: 162317 blocks:  47506
refs: 162375 blocks:  47526
refs: 162433 blocks:  47546
refs: 162491 blocks:  47566
refs: 162549 blocks:  47586
refs: 162607 blocks:  47606

Tell me if I did something wrong.

If I modify the test to import the _multiprocessing extension instead, I see no leak on the Python main branch (without this PR):

refs: 162674 blocks:  49018

refs: 162679 blocks:  49018
refs: 162679 blocks:  49018
refs: 162679 blocks:  49018
refs: 162679 blocks:  49018
refs: 162679 blocks:  49018
refs: 162679 blocks:  49018
refs: 162679 blocks:  49018
refs: 162679 blocks:  49018

Note: I added an empty line for readability.

I had only verified single leak #94382 (comment) which is fixed by this PR. If the multiple initialization is not fixed, there must be something in multiprocessing module which is causing it. Anyways, I'll wait for decision on the discourse now before putting anymore effort into this as there are more reason for this change other than multiple initialization.

This means I can remove do-not-merge, right ?

Please wait until there is an agreement for this change on Discourse.

FYI, I'll merge this in a day or two.

I'm hesitant to backport this change. Let me know if you disagree, Victor / Petr.

Thanks, Kumar!