E66F 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

Changes from 1 commit
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 10000
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
Prev Previous commit
Next Next commit
Fix whitespace in asyncio.run() docstring
  • Loading branch information
@aeros
aeros committed Sep 25, 2019
commit b6788188f31215d8d40f3c550fec6679073c8ec5
2 changes: 1 addition & 1 deletion Lib/asyncio/runners.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ def run(main, *, debug=False):
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,
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:
Expand Down
0