bpo-34037: test_asyncio uses shutdown_default_executor() by vstinner · Pull Request #16284 · python/cpython

@vstinner Apologies for the late response, I've been taking it easier today since I think I've been doing a bit too much recently, trying to avoid the infamous open source burnout you've talked about a few times. (:

Overall, the tests look correct to me. My only concern is a potential timeout issue from the part in close_loop() (lines 511-516 in test_asyncio/utils.py):

     def close_loop(loop):
         if loop._default_executor is not None:
             if not loop.is_closed():
                 loop.run_until_complete(loop.shutdown_default_executor())
             else:
                 loop._default_executor.shutdown(wait=True)

Since shutdown_default_executor() already attempts to call _default_executor.shutdown(wait=True) within the thread it creates, if there are any issues with joining the threads, presumably calling _default_executor.shutdown(wait=True) again would not resolve the issue and could end up resulting in a complete timeout for test_asyncio if any of the threads in the ThreadPoolExecutor were unable to be joined.

Therefore, I would recommend changing the else block to the following:

            else:
                loop._default_executor.shutdown(wait=False)

This will at least guarantee that the executor will be shutdown completely, regardless of the status of the threads. IMO, a timeout is the worst case scenario, since it gives us far less useful debugging information and makes the issue harder to resolve.

An alternative of this could be:

     def close_loop(loop):
         if loop._default_executor is not None:
             try:
                 loop.run_until_complete(loop.shutdown_default_executor())
             finally:
                 loop._default_executor.shutdown(wait=False)

This would provide further assurance that the executor will be shut down, since loop._default_executor.shutdown(wait=False) would always be called last. But this may come at the cost of slightly reducing performance, since it would be called redundantly in normal scenarios. For that reason, I prefer the first option.

What are your thoughts @asvetlov and @1st1?