asyncio: BaseEventLoop.close() shutdowns the executor without waiting causing leak of dangling threads

[vstinner]

BPO	34037
Nosy	@vstinner, @encukou, @asvetlov, @cjrh, @ambv, @1st1, @cmeyer, @aeros
PRs	Revert "bpo-34037, asyncio: add BaseEventLoop.wait_executor_on_close (GH-13786)" #13802 bpo-34037: Fix test_asyncio failure and add loop.shutdown_default_executor() #15735 bpo-34037: test_asyncio uses shutdown_default_executor() #16284 bpo-34037: Add Python API whatsnew for loop.shutdown_default_executor() #19634

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2020-04-21.21:31:38.061>
created_at = <Date 2018-07-03.22:57:26.558>
labels = ['tests', '3.9', 'expert-asyncio']
title = 'asyncio: BaseEventLoop.close() shutdowns the executor without waiting causing leak of dangling threads'
updated_at = <Date 2020-04-21.21:31:38.060>
user = 'https://github.com/vstinner'

bugs.python.org fields:

activity = <Date 2020-04-21.21:31:38.060>
actor = 'aeros'
assignee = 'none'
closed = True
closed_date = <Date 2020-04-21.21:31:38.061>
closer = 'aeros'
components = ['Tests', 'asyncio']
creation = <Date 2018-07-03.22:57:26.558>
creator = 'vstinner'
dependencies = []
files = []
hgrepos = []
issue_num = 34037
keywords = ['patch']
message_count = 30.0
messages = ['321006', '321534', '326050', '326052', '326259', '344475', '344476', '344477', '344545', '344547', '344548', '344552', '344686', '351231', '351232', '351349', '351359', '351384', '351394', '351396', '352788', '352789', '352797', '352798', '352822', '366812', '366821', '366936', '366939', '366943']
nosy_count = 8.0
nosy_names = ['vstinner', 'petr.viktorin', 'asvetlov', 'cjrh', 'lukasz.langa', 'yselivanov', 'cmeyer', 'aeros']
pr_nums = ['13802', '15735', '16284', '19634']
priority = 'high'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue34037'
versions = ['Python 3.9']

[vstinner]

AMD64 FreeBSD 10.x Shared 3.7:
http://buildbot.python.org/all/#/builders/124/builds/410

...
test_remove_fds_after_closing (test.test_asyncio.test_events.KqueueEventLoopTests) ... ok
test_run_in_executor (test.test_asyncio.test_events.KqueueEventLoopTests) ... ok
test_run_in_executor_cancel (test.test_asyncio.test_events.KqueueEventLoopTests) ...

Warning -- threading_cleanup() failed to cleanup 1 threads (count: 1, dangling: 2)
Dangling thread: <Thread(ThreadPoolExecutor-13_0, started daemon 34471091200)>
Dangling thread: <_MainThread(MainThread, started 34393318400)>

ok
test_run_until_complete (test.test_asyncio.test_events.KqueueEventLoopTests) ... ok
test_run_until_complete_nesting (test.test_asyncio.test_events.KqueueEventLoopTests) ... ok
...
1 test altered the execution environment:
test_asyncio

[vstinner]

It's easy to reproduce the issue on Linux:

diff --git a/Lib/test/test_asyncio/test_events.py b/Lib/test/test_asyncio/test_events.py
index 11cd950df1..df4c2b9849 100644
--- a/Lib/test/test_asyncio/test_events.py
+++ b/Lib/test/test_asyncio/test_events.py
@@ -359,7 +359,7 @@ class EventLoopTestsMixin:
             called = True
 
         def run():
-            time.sleep(0.05)
+            time.sleep(1.0)
 
         f2 = self.loop.run_in_executor(None, run)
         f2.cancel()

The problem is that BaseEventLoop.close() shutdowns its default executor without waiting:

    def close(self):
        ...
        executor = self._default_executor
        if executor is not None:
            self._default_executor = None
            executor.shutdown(wait=True)

I fixed a similar issue in socketserver:

• bpo-31233: for socketserver.ThreadingMixIn
• bpo-31151: for socketserver.ForkingMixIn
• bpo-33540: add block_on_close attr to socketserver

I suggest to wait by default, but maybe also add a block_on_close attribute to BaseEventLoop (default: False) just for backward compatibility.

What do you think Yury, Andrew, and Guido?

[vstinner]

Yury, Andrew: Do you know if the executor doesn't wait on purpose? Would it be possible to change that in Python 3.8?

[1st1]

Yury, Andrew: Do you know if the executor doesn't wait on purpose? Would it be possible to change that in Python 3.8?

Maybe. At least we need to add a "timeout" argument to asyncio.run() to let it wait for executor jobs.

I'm also thinking about making OS threads cancellable/interruptable in Python 3.8 (if they run pure Python code).

[vstinner]

Maybe. At least we need to add a "timeout" argument to asyncio.run() to let it wait for executor jobs.

The shutdown() method of concurrent.futures.Executor API doesn't accept a timeout. It waits for multiple things.

I added "block_on_close = True" class attribute to socketserver.ForkingMixIn and socketserver.ThreadingMixIn. By default, server_close() waits until all children complete, but the wait is non-blocking if block_on_close is false.

[vstinner]

I wrote PR 13786 to fix this issue.

[vstinner]

"executor.shutdown(wait=False)" was added by this change:

commit 4ca7355
Author: Antoine Pitrou <solipsis@pitrou.net>
Date: Sun Oct 20 00:54:10 2013 +0200

Issue bpo-19299: fix refleak test failures in test_asyncio

Guido van Rossum proposed to wait for the executor. Guido proposed " executor.shutdown(wait=False)", but he didn't comment why wait=False:

https://bugs.python.org/issue19299#msg200508

[vstinner]

New changeset 0f0a30f by Victor Stinner in branch 'master':
bpo-34037, asyncio: add BaseEventLoop.wait_executor_on_close (GH-13786)
0f0a30f

[ambv]

New changeset 7f9a2ae by Łukasz Langa in branch 'master':
Revert "bpo-34037, asyncio: add BaseEventLoop.wait_executor_on_close (GH-13786)" (bpo-13802)
7f9a2ae