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

[Kyle Stanley]

Kyle Stanley  added the comment:

PR-19634 should address the primary issue of documenting in the 3.9 whatsnew 
that alternative event loops should implement loop.shutdown_default_executor().

For reference, here's the implementation in BaseEventLoop: 
https://github.com/python/cpython/blob/9c82ea7868a1c5ecf88891c627b5c399357eb05e/Lib/asyncio/base_events.py#L556-L574.
 I believe it could be used in alternative event loops without too much 
modification.

--
resolution:  -> fixed
stage: patch review -> resolved
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Kyle Stanley]

Kyle Stanley  added the comment:


New changeset 9c82ea7868a1c5ecf88891c627b5c399357eb05e by Kyle Stanley in 
branch 'master':
bpo-34037: Add Python API whatsnew for loop.shutdown_default_executor() (#19634)
https://github.com/python/cpython/commit/9c82ea7868a1c5ecf88891c627b5c399357eb05e


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Kyle Stanley]

Change by Kyle Stanley :


--
pull_requests: +18960
stage: resolved -> patch review
pull_request: https://github.com/python/cpython/pull/19634

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Kyle Stanley]

Kyle Stanley  added the comment:

> Kyle: Can you please add a short sentence there to document the new 
> requirement?

Yep, I'll open a PR soon. I'm glad this is coming up now rather than 
post-release, thanks for bringing attention to it.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Chris Meyer]

Change by Chris Meyer :


--
nosy: +cmeyer

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[STINNER Victor]

STINNER Victor  added the comment:

I reopen the issue.

> /usr/lib64/python3.9/asyncio/events.py:254: in shutdown_default_executor
> raise NotImplementedError

That means that the project (python-anyio) implements its own event loop which 
inherits from AbstractEventLoop, it doesn't use BaseEventLoop which implements 
shutdown_default_executor().

AbstractEventLoop documentation says:

https://docs.python.org/dev/library/asyncio-eventloop.html#asyncio.AbstractEventLoop

"The Event Loop Methods section lists all methods that an alternative 
implementation of AbstractEventLoop should have defined."

It points to the following documentation which lists shutdown_asyncgens():

It points to 
https://docs.python.org/dev/library/asyncio-eventloop.html#asyncio-event-loop


> I'm not all that familiar with asyncio, but it seems to me that all event 
> loops inherited from BaseEventLoop must be updated to implement this new 
> method to correctly work with run() in Python 3.9. Is that right?

Yes

> If it is, this needs at least a much more prominent What's New entry. Or the 
> hard NotImplementedError should turn into a warning.

Raising NotImplementedError is a deliberate design choice.

Documentation the new requirement in the following section sounds like a good 
idea.

https://docs.python.org/dev/whatsnew/3.9.html#changes-in-the-python-api

Kyle: Can you please add a short sentence there to document the new requirement?

--
resolution: fixed -> 
status: closed -> open

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Petr Viktorin]

Petr Viktorin  added the comment:

I got a report from a library that ties together asyncio and some other async 
libraries, getting errors like this:

tests/test_taskgroups.py:60: in test_run_natively
module.run(testfunc())
/usr/lib64/python3.9/asyncio/runners.py:48: in run
loop.run_until_complete(loop.shutdown_default_executor())
uvloop/loop.pyx:1451: in uvloop.loop.Loop.run_until_complete
???
/usr/lib64/python3.9/asyncio/events.py:254: in shutdown_default_executor
raise NotImplementedError
E   NotImplementedError

(more at: https://bugzilla.redhat.com/show_bug.cgi?id=1817681#c1 )

I'm not all that familiar with asyncio, but it seems to me that all event loops 
inherited from BaseEventLoop must be updated to implement this new method to 
correctly work with run() in Python 3.9. Is that right? If it is, this needs at 
least a much more prominent What's New entry. Or the hard NotImplementedError 
should turn into a warning.

--
nosy: +petr.viktorin

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Kyle Stanley]

Kyle Stanley  added the comment:

> Thanks, Kyle!

No problem, and thanks for all of the help from Andrew, Yury, and Victor!

> IMHO it will make asyncio more reliable, especially for tests on the CI.

Awesome, that was my primary intention. (:

> If it becomes an issue in Python 3.9 (executor hangs forever),

Feel free to add me to the nosy list if you have to open an issue for it, I'd 
be glad to help out with this if it becomes an issue.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[STINNER Victor]

STINNER Victor  added the comment:

asyncio.run() now shutdowns the default executor: nice, thanks! That was my 
request. IMHO it will make asyncio more reliable, especially for tests on the 
CI.

If it becomes an issue in Python 3.9 (executor hangs forever), we can add new 
parameters (to run()?) to put a timeout for example.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[STINNER Victor]

STINNER Victor  added the comment:


New changeset 079931d12223ec98cbf53185b90db48efa61f93f by Victor Stinner in 
branch 'master':
bpo-34037: test_asyncio uses shutdown_default_executor() (GH-16284)
https://github.com/python/cpython/commit/079931d12223ec98cbf53185b90db48efa61f93f


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[STINNER Victor]

Change by STINNER Victor :


--
pull_requests: +15869
pull_request: https://github.com/python/cpython/pull/16284

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Andrew Svetlov]

Andrew Svetlov  added the comment:


New changeset 9fdc64cf1266b6d5bf0503847b5c38e5edc53a14 by Andrew Svetlov (Kyle 
Stanley) in branch 'master':
bpo-34037: Fix test_asyncio failure and add loop.shutdown_default_executor() 
(GH-15735)
https://github.com/python/cpython/commit/9fdc64cf1266b6d5bf0503847b5c38e5edc53a14


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Andrew Svetlov]

Andrew Svetlov  added the comment:

Thnks, Kyle!

--
resolution:  -> fixed
stage: patch review -> resolved
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Caleb Hattingh]

Change by Caleb Hattingh :


--
nosy: +cjrh

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Yury Selivanov]

Yury Selivanov  added the comment:

> What do you think of the "shutdown_default_executor()" name?

Yeah, I think it's a better name!

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[STINNER Victor]

STINNER Victor  added the comment:

"shutdown_threadpool()" name

What do you think of the "shutdown_default_executor()" name?

The default executor can be overriden by set_default_executor():

def set_default_executor(self, executor):
if not isinstance(executor, concurrent.futures.ThreadPoolExecutor):
warnings.warn(
'Using the default executor that is not an instance of '
'ThreadPoolExecutor is deprecated and will be prohibited '
'in Python 3.9',
DeprecationWarning, 2)
self._default_executor = executor

The default executor should always be a thread pool, so "shutdown_threadpool()" 
name is also correct. I have no strong preference.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Yury Selivanov]

Yury Selivanov  added the comment:

Here's the API I propose to solve this problem: 
https://github.com/python/cpython/pull/15735#pullrequestreview-285389412

Summary:

* Add a new loop.shutdown_threadpool() method. Just like with 
shutdown_asyncgens() -- it would be invalid to call loop.run_in_executer() 
after loop.shutdown_threadpool() is called.

* Make asyncio.run() to call the new loop.shutdown_threadpool().

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[STINNER Victor]

STINNER Victor  added the comment:

I change the version to Python 3.9: the 3.8 branch don't accept new features 
anymore.

--
versions: +Python 3.9 -Python 3.8

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Kyle Stanley]

Kyle Stanley  added the comment:

I've opened PR-15735 which applies the same functionality as Victor's PR-13786, 
but adds the public getter and setter methods (for both AbstractEventLoop and 
BaseEventLoop) as requested by Andrew. 

Since this is still causing intermittent CI failures from test_asyncio, I think 
it's important to implement these changes in some form in the near future.

--
priority: normal -> high

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Kyle Stanley]

Change by Kyle Stanley :


--
pull_requests: +15390
pull_request: https://github.com/python/cpython/pull/15735

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Kyle Stanley]

Kyle Stanley  added the comment:

> Any plan to reapply my change, or fix it?

I can try to help with this. I'm not the most familiar with the internals of 
asyncio, but I think it would provide a good learning experience.

--
nosy: +aeros167

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[STINNER Victor]

STINNER Victor  added the comment:

This bug still occurs randomly on the CI. Example on AppVeyor:
https://ci.appveyor.com/project/python/cpython/builds/27209898

test_reader_callback (test.test_asyncio.test_events.SelectEventLoopTests) ... ok
test_remove_fds_after_closing 
(test.test_asyncio.test_events.SelectEventLoopTests) ... ok
test_run_in_executor (test.test_asyncio.test_events.SelectEventLoopTests) ... ok
test_run_in_executor_cancel 
(test.test_asyncio.test_events.SelectEventLoopTests) ... Warning -- 
threading_cleanup() failed to cleanup 1 threads (count: 1, dangling: 2)
Dangling thread: 
Dangling thread: <_MainThread(MainThread, started 1916)>
ok

Any plan to reapply my change, or fix it?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Andrew Svetlov]

Andrew Svetlov  added the comment:

I prefer postponing to think about it until all required 3.8 tasks are done :)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[STINNER Victor]

STINNER Victor  added the comment:

> The problem is that loop.close() can "hang" for minutes if there are pending 
> background jobs or slow DNS resolution requests.

Maybe concurrent.futures should be enhanced for that?

Or from asyncio, is it possible to poll the executor and emit a warning if some 
threads take longer than ? It would be great if we could 
display repr() of the slow job in that case.

Yury also asked for a timeout, but it's not supported by concurrent.futures 
yet, and I'm not sure what should be done in case of timeout? Retry? Log a 
warning?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Andrew Svetlov]

Andrew Svetlov  added the comment:

I still think that this is a valuable change but we need to discuss public API.

In fact, in my job we used to override default executor and wait for its 
shutdown with explicit waiting to make the system robust.

The problem is that loop.close() can "hang" for minutes if there are pending 
background jobs or slow DNS resolution requests.

We controlled our tasks running in the thread pool carefully and used aiodns 
for DNS resolving.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[STINNER Victor]

STINNER Victor  added the comment:

Łukasz Langa reverted my change (PR 13786). I pushed my change without any 
review to try to fix https://bugs.python.org/issue37137 which blocked the 
Python 3.8 beta1. Sadly, my change didn't fix this issue. The root cause was 
differently (and it's now fixed).

Andrew Svetlov asked to make some changes on my API. Yury Selivanov added "I 
don't like this last minute api decision. Please don't merge this.". See PR 
13786 for the discussion. In short, my change was reverted because of the API.

But I still consider that calling executor.shutdown(wait=False) in loop.close() 
in wrong: we must wait until it finishes. Otherwise, the code "leaks" threads 
which continue to run and so can cause troubles. I would prefer to wait by 
default.

Andrew, Yury: Would you be interested to rewrite my change in the "asyncio way"?

IMHO this change can wait Python 3.9, it's not a critical bug.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Łukasz Langa]

Łukasz Langa  added the comment:


New changeset 7f9a2ae78051877f4d966119e2fcd27ec77eda1d by Łukasz Langa in 
branch 'master':
Revert "bpo-34037, asyncio: add BaseEventLoop.wait_executor_on_close 
(GH-13786)" (#13802)
https://github.com/python/cpython/commit/7f9a2ae78051877f4d966119e2fcd27ec77eda1d


--
nosy: +lukasz.langa

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Łukasz Langa]

Change by Łukasz Langa :


--
keywords: +patch
pull_requests: +13688
stage:  -> patch review
pull_request: https://github.com/python/cpython/pull/13802

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[STINNER Victor]

STINNER Victor  added the comment:


New changeset 0f0a30f4da4b529e0f7df857b9f575b231b32758 by Victor Stinner in 
branch 'master':
bpo-34037, asyncio: add BaseEventLoop.wait_executor_on_close (GH-13786)
https://github.com/python/cpython/commit/0f0a30f4da4b529e0f7df857b9f575b231b32758


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[STINNER Victor]

STINNER Victor  added the comment:

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

commit 4ca7355901049ff9a873cf236091e6780d7a8126
Author: Antoine Pitrou 
Date:   Sun Oct 20 00:54:10 2013 +0200

Issue #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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[STINNER Victor]

STINNER Victor  added the comment:

I wrote PR 13786 to fix this issue.

--
versions: +Python 3.8 -Python 3.7

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[STINNER Victor]

STINNER Victor  added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Yury Selivanov]

Yury Selivanov  added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[STINNER Victor]

STINNER Victor  added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Guido van Rossum]

Change by Guido van Rossum :


--
nosy:  -gvanrossum

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[STINNER Victor]

Change by STINNER Victor :


--
title: test_asyncio: test_run_in_executor_cancel() leaked a dangling thread on 
AMD64 FreeBSD 10.x Shared 3.7 -> asyncio: BaseEventLoop.close() shutdowns the 
executor without waiting causing leak of dangling threads

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com