10000 bpo-38267: Add thread timeout parameter to `loop.shutdown_default_executor()` by aeros · Pull Request #16360 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

bpo-38267: Add thread timeout parameter to loop.shutdown_default_executor() #16360

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 26 commits into from
Closed

bpo-38267: Add thread timeout parameter to loop.shutdown_default_executor() #16360

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
700cf86
Add thread timeout constant
aeros Sep 24, 2019
1e7794b
Add timeout param to `loop.shutdown_default_executor()`
aeros Sep 24, 2019
a262c45
Set timeout param default to None
aeros Sep 24, 2019
b1bb810
Update `loop.shutdown_default_executor() docstring
aeros Sep 24, 2019
8a4c30c
Move constant to runners.py
aeros Sep 24, 2019
ebc1b99
Add thread timeout for `asyncio.run()`
aeros Sep 25, 2019
5b90766
Update `asyncio.run()` docstring for `shutdown_default_executor()`
aeros Sep 25, 2019
f189f04
Update `asyncio.run()` docstring for thread timeout
aeros Sep 25, 2019
16921a1
Update `asyncio.run()` docs
aeros Sep 25, 2019
b678818
Fix whitespace in `asyncio.run()` docstring
aeros Sep 25, 2019
e332257
Update `shutdown_default_executor()` docs
aeros Sep 25, 2019
7f8428b
Patchcheck
aeros Sep 25, 2019
577b2b2
📜🤖 Added by blurb_it.
blurb-it[bot] Sep 25, 2019
b673080
Fix Misc/NEWS Sphinx role
aeros Sep 25, 2019
926de3f
Merge branch 'master' into bpo-38267-shutdown_default_executor-timeout
aeros Oct 1, 2019
f7c623c
Fix documentation for timeout parameter
aeros Oct 3, 2019
169d2d4
Fix documentation for timeout parameter
aeros Oct 3, 2019
f0dde2d
Fix asyncio.run() docs
aeros Oct 3, 2019
b7c0608
Fix loop.shutdown_default_executor() docstring
aeros Oct 3, 2019
1e95afb
Fix loop.shutdown_default_executor docs
aeros Oct 3, 2019
2b1f35d
Fix loop.shutdown_default_executor()
aeros Oct 4, 2019
e549996
Fix asyncio.run() docstring
aeros Oct 4, 2019
8a5121c
Fix runners.py comment
aeros Oct 4, 2019
4264c04
Fix warning
aeros Oct 5, 2019
e9a0f19
Fix RuntimeWarning
aeros Oct 29, 2019
d9c2e37
Fix style nits
gvanrossum Sep 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion Doc/library/asyncio-eventloop.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,13 +167,17 @@ Running and stopping the loop

.. versionadded:: 3.6

.. coroutinemethod:: loop.shutdown_default_executor()
.. coroutinemethod:: loop.shutdown_default_executor(timeout=None)

Schedule the closure of the default executor and wait for it to join all of
the threads in the :class:`ThreadPoolExecutor`. After calling this method, a
:exc:`RuntimeError` will be raised if :meth:`loop.run_in_executor` is called
while using the default executor.

The *timeout* parameter specifies the amount of time each thread will be
given to finish joining. The default value is ``None``, which means that each
thread will be given an indefinite amount of time to join.

Note that there is no need to call this function when
:func:`asyncio.run` is used.

Expand Down
4 changes: 4 additions & 0 deletions Doc/library/asyncio-task.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,10 @@ Running an asyncio Program
the end. It should be used as a main entry point for asyncio
programs, and should ideally only be called once.

Each thread within the threadpool is given a timeout duration of 5
minutes. If the thread hasn't finishing joining within that duration,
the lock is released and the thread is terminated.

.. versionadded:: 3.7

.. versionchanged:: 3.9
Expand Down
11 changes: 8 additions & 3 deletions Lib/asyncio/base_events.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -549,8 +549,13 @@ async def shutdown_asyncgens(self):
'asyncgen': agen
})

async def shutdown_default_executor(self):
"""Schedule the shutdown of the default executor."""
async def shutdown_default_executor(self, timeout=None):
"""Schedule the shutdown of the default executor.

The timeout parameter specifies the amount of time each thread will
be given to finish joining. The default value is None, which means
that each thread will be given an indefinite amount of time to join.
"""
self._executor_shutdown_called = True
if self._default_executor is None:
return
Expand All @@ -560,7 +565,7 @@ async def shutdown_default_executor(self):
try:
await future
finally:
thread.join()
thread.join(timeout)

def _do_shutdown(self, future):
try:
Expand Down
15 changes: 12 additions & 3 deletions Lib/asyncio/runners.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,16 @@
from . import tasks


# Default timeout for joining each thread in `shutdown_default_executor()`
THREAD_JOIN_TIMEOUT = 300


def run(main, *, debug=False):
"""Run a coroutine.

This function runs the passed coroutine, taking care of
managing the asyncio event loop and finalizing asynchronous
generators.
managing the asyncio event loop, finalizing asynchronous
generators, and closing the threadpool.

This function cannot be called when another asyncio event loop is
running in the same thread.
Expand All @@ -21,6 +25,10 @@ def run(main, *, debug=False):
It should be used as a main entry point for asyncio programs, and should
ideally only be called once.

Each thread within the threadpool is given a timeout duration of 5
minutes. If the thread hasn't finishing joining within that duration,
Copy link
Contributor Author
@aeros aeros Sep 25, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@1st1 While writing the documentation, I was thinking that @asvetlov's suggestion to add timeout as parameter for asyncio.run() might be beneficial. Although it would add an additional knob for users to adjust, forcing them to strictly use a 5 minute timeout duration seems a bit restrictive.

Do you think it might be useful to give users the option to adjust the timeout duration by adding a timeout parameter to asynio.run()? Since you regularly interact with daily users of asyncio, you might have a better idea of whether or not that would be useful.

Copy link
Member
@1st1 1st1 Sep 25, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that adding knobs is only justified when there's request for that from users. In this particular case it's a bit tricky.

The way I think about it is that it's easy to implement whatever timeout is necessary with the following pattern:

asyncio.run(main())

# versus

async def runner():
  try:
    return await main()
  finally:
    loop = asyncio.get_running_loop()
    await loop.shutdown_default_executor(3600)

asyncio.run(runner())

The argument for this pattern is that in more or less advanced asyncio applications there's always a "runner()" like in the above. It usually sets up signal handlers, manages global state, etc. asyncio.run() in those cases is simply providing a set of reasonable defaults on how you run your event loop and how you shut it down. And 5 minutes is a reasonable default in this case, I guess.

Long story short, I'd go with this design and learn from our users if we need to make it configurable.

This is just my opinion on this and I'd love to hear what @asvetlov thinks about it.

Copy link
Contributor Author
@aeros aeros Sep 25, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The argument for this pattern is that in more or less advanced asyncio applications there's always a "runner()" like in the above.

Ah, I actually hadn't thought as much about how relatively simple it would be for the users to create their own runner. I wonder if it would be worthwhile to document this pattern somewhere in asyncio-task.rst, for those who desire further customization.

And 5 minutes is a reasonable default in this case, I guess.

Yeah 5 minutes seems reasonable for most purposes. If it's an issue, we can easily adjust it by modifying the constant.

Long story short, I'd go with this design and learn from our users if we need to make it configurable.

Good point. It might be a better idea to leave out the parameter initially and then add it in later if the users request it. It would be easy to add the new parameter to the API for asyncio.run() if it's desired, but it's incredibly difficult to remove parameters in the future.

This is just my opinion on this and I'd love to hear what @asvetlov thinks about it.

Personally I have no issue with either of the options, I think both of them would be viable. After reading your latest response, I'm leaning more in favor of keeping the current iteration though.

The more I've been involved with API design, I've increasingly come to realize that there's frequently some degree of disconnect between what the users actually want and what the developers think the users want. It can definitely be worthwhile to start with a minimalist approach and add features based on user feedback, rather than making assumptions. Making too many assumptions can easily lead to feature bloat.

the lock is released and the thread is terminated.

Example:

async def main():
Expand All @@ -45,7 +53,8 @@ async def main():
try:
_cancel_all_tasks(loop)
loop.run_until_complete(loop.shutdown_asyncgens())
loop.run_until_complete(loop.shutdown_default_executor())
loop.run_until_complete(
loop.shutdown_default_executor(timeout=THREAD_JOIN_TIMEOUT))
finally:
events.set_event_loop(None)
loop.close()
Expand Down
1 change: 1 addition & 0 deletions Misc/NEWS.d/next/Library/2019-09-25-00-37-51.bpo-38267.X9Jb5V.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Add *timeout* parameter to :meth:`asyncio.loop.shutdown_default_executor`.
0