bpo-40115: Fix refleak in test_asyncio by aeros · Pull Request #19282 · python/cpython

My change to remove daemon threads in concurrent.futures (#19149) revealed a subtle issue in test_events.test_run_in_executor_cancel: it does not properly clean up the executor's resources. This should be done by calling loop.shutdown_default_executor() prior to loop.close(), as is done in asyncio.run().

Before:

[aeros:~/repos/aeros-cpython]$ ./python -m test --fail-env-changed -R 3:3 test_asyncio -m test.test_asyncio.test_events.EPollEventLoopTests.test_run_in_executor_cancel
0:00:00 load avg: 0.63 Run tests sequentially
0:00:00 load avg: 0.63 [1/1] test_asyncio
beginning 6 repetitions
123456
......
test_asyncio leaked [1, 1, 1] references, sum=3
test_asyncio leaked [2, 1, 1] memory blocks, sum=4
test_asyncio failed

== Tests result: FAILURE ==

1 test failed:
    test_asyncio

Total duration: 3.2 sec
Tests result: FAILURE

After:

[aeros:~/repos/aeros-cpython]$ ./python -m test --fail-env-changed -R 3:3 test_asyncio -m test.test_asyncio.test_events.EPollEventLoopTests.test_run_in_executor_cancel
0:00:00 load avg: 0.24 Run tests sequentially
0:00:00 load avg: 0.24 [1/1] test_asyncio
beginning 6 repetitions
123456
......

== Tests result: SUCCESS ==

1 test OK.

Total duration: 3.5 sec
Tests result: SUCCESS

/cc @vstinner @pitrou @1st1

https://bugs.python.org/issue40115

🤖 New build scheduled with the buildbot fleet by @aeros for commit 13fa9a966ccf1a9c996085483a3ea1d351dee409 🤖

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

Prior to merging, we should wait for the refleak tests to pass.

Edit: I confirmed the refleak in test_threading incorrectly, so I'm currently redoing it.

I redid the tests locally directly on top of b61b818, and it was passing for test_threading, so it looks like it was introduced in a later commit:

[aeros:~/repos/aeros-cpython]$ ./python -m test --fail-env-changed -R 3:3 test_threading (bpo39812-cf-remove-daemon) 
0:00:00 load avg: 0.65 Run tests sequentially
0:00:00 load avg: 0.65 [1/1] test_threading
beginning 6 repetitions
123456
......
test_threading passed in 1 min

== Tests result: SUCCESS ==

1 test OK.

Total duration: 1 min
Tests result: SUCCESS

@vstinner From carefully looking over all of the buildbot logs and my own local testing, I believe the refleaks showing in the tests are entirely separate from both this PR and #19149. I strongly suspect they were introduced in-between b61b818 and the latest commit to master.

With that in mind, is it okay to merge this PR instead of reverting b61b818?

Without this change, ./python -m test --fail-env-changed -R 3:3 test_asyncio -m test.test_asyncio.test_events.EPollEventLoopTests.test_run_in_executor_cancel leaks.

With this change, it no longer leaks. So I confirm that this PR fix https://bugs.python.org/issue40115

On "buildbot/AMD64 Fedora Stable Refleaks PR", I see:

test_threading leaked [38, 38, 38] references

That's an unrelated regression: I created https://bugs.python.org/issue40149 I used test.bisect_cmd to identify one leaking test and then I used git bisect to identify which commit introduced the regression.

With that in mind, is it okay to merge this PR instead of reverting b61b818?

Sure.

@vstinner Thanks for the clarification. It's my first time addressing a regression that I introduced, so I just wanted to double check.

Also, I was unaware of test.bisect_cmd, I'll look into that. I've more recently starting making use of git bisect though, after reading about it in a Committers topic on discuss.python.org.

(Note: the GitHub Actions Ubuntu test seems to be intermittently failing in the "Install dependencies" stage across several PRs. If someone hasn't already, I'll likely open a python-dev thread tomorrow to bring attention to it.)

Oh crap, did that just revert the commit in a single click @vstinner? I saw something in the UI with the post-commit checks and wanted to check up on it; I accidentally misclicked "revert commit" instead of "view details".

Edit: Never mind, fortunately it looks like that just created a separate branch in the cpython repository titled revert-19282-bpo40115-test_asyncio-run_in_executor_cancel. I'll reach out on python-committers for the appropriate way to clean up that branch. Sorry about the confusion.

Edit2: Ned helped to clean up the branch after my email to python-committers, so there is no issue now. I'll try to be more careful w/ that in the future and keep in mind the active branches page for future reference.

Oh crap, did that just revert the commit in a single click @vstinner?

You cannot push a commit directly. Changes must go through a PR. I don't think that a single click is enough to revert a change :-)

You cannot push a commit directly. Changes must go through a PR. I don't think that a single click is enough to revert a change :-)

Yeah, I figured that out after I realized that it just created a branch; it's good to know for sure though. Thanks for clarifying, I'm still very new to having commit privileges. :-)

(It gave me quite a bit of anxiety in the moment, but after Ned's response I realized that it was a much bigger deal in my mind than it actually was.)