bpo-32604: Fix reference leak in select module by vstinner · Pull Request #20600 · python/cpython

the-knights-who-say-ni

Fix reference leak in PyInit_select() of the select module:
remove Py_INCREF(poll_Type).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to see if, with this patch, I could solve the refleaks we have been discussing @vstinner on subinterpreters but I can still see the leaks. Am I missing something? Looks like you said you got a success on your end.

I tried to see if, with this patch, I could solve the refleaks we have been discussing @vstinner on subinterpreters but I can still see the leaks. Am I missing something? Looks like you said you got a success on your end.

Which commit did you try? Did you rebase the commit? I used the commit 9d17cbf.

I have run your test in my vm, it have been fixed, thanks.

$ ./python -m test -R 3:3 test_subinterpreters
0:00:00 load avg: 0.76 Run tests sequentially
0:00:00 load avg: 0.76 [1/1] test_subinterpreters
beginning 6 repetitions
123456
......

== Tests result: SUCCESS ==

1 test OK.

Total duration: 465 ms
Tests result: SUCCESS

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well I 've run the test code on my local and I detect the current master branch is leaked
and this PR fixes the issue.

   def test_bpo_32604(self):
        code = f"""import select
a = select.poll()
        """
        ret = test.support.run_in_subinterp(code)
        self.assertEqual(ret, 0)

Thanks for the work @vstinner

@nanjekyejoannah:

I tried to see if, with this patch, I could solve the refleaks we have been discussing @vstinner on subinterpreters but I can still see the leaks. Am I missing something? Looks like you said you got a success on your end.

I talked to Joannah in private and she just used the wrong command to test the fix :-)

@corona10:

Well I 've run the test code on my local and I detect the current master branch is leaked
and this PR fixes the issue.

Thanks for this nice manual test!

Yes, I confirmed with the correct command and this fixes the leak. Thanks @vstinner