bpo-40115: Fix refleak in test_asyncio

[aeros]

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

[bedevere-bot]

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

[aeros]

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

[aeros]

The Ubuntu PR test in Azure Pipelines seems to be failing because the following URL is dead: https://www.openssl.org/source/openssl-1.1.1d.tar.gz. See line 184 of the build log.

[vstinner]

LGTM.

[aeros]

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 

[aeros]

@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?

[vstinner]

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

[vstinner]

The Ubuntu PR test in Azure Pipelines seems to be failing because the following URL is dead: https://www.openssl.org/source/openssl-1.1.1d.tar.gz. See line 184 of the build log.

I fixed it with commit 224e1c3: you need to rebase your PR on top of it.

[vstinner]

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.

[aeros]

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

[aeros]

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.

[vstinner]

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 :-)

[aeros]

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