Skip to content

gh-111358: Fix timeout behaviour in BaseEventLoop.shutdown_default_executor #115622

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.

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

Merged
merged 3 commits into from
Feb 19, 2024

Conversation

ordinary-jamie
Copy link
Contributor

@ordinary-jamie ordinary-jamie commented Feb 18, 2024

Fixes the timeout behaviour in asyncio.BaseEventLoop.shutdown_default_executor to ensure that the coroutine correctly times-out when awaiting the shutdown thread.

If the timeout is reaching, the thread is left to run, and is not joined (since waiting for the thread to join in this case would exceed the timeout). The Executor is shutdown without waiting.

Minor updates to _do_shutdown to ensure future isn't cancelled. This is necessary since the thread can still run and attempt to set the future result/exception

@gvanrossum
Copy link
Member

@ordinary-jamie Can you look into the test failures? They all look related to shutdown_default_executor().

@gvanrossum gvanrossum added the needs backport to 3.12 only security fixes label Feb 18, 2024
@gvanrossum
Copy link
Member

This can cleanly be backported to 3.12, but not to 3.11 -- that doesn't have the timeout argument.

Either way, while this PR fixes the timeout logic, it doesn't really help the OP of gh-111358 -- it'll still wait 300 seconds for the shutdown upon receiving the first ^C, so it still won't print anything right away, and it'll take three ^C to exit.

But I'll note that this isn't entirely asyncio's fault -- if you use an executor to sleep 1000 seconds and print the result, it'll take two ^C to exit. (Admittedly, if you don't wait for the future returned by submit() it only takes one ^C.) In either case, the final traceback printed is from threading._shutdown() handling atexit calls, trying to quit the idle threads in all executors. Note that this traceback is being logged during shutdown (it says "Exception ignored in ...") -- it is not printed by the regular exception handling if __main__ is interrupted.

Also, I might as well ping @kumaraditya303, to see if he agrees with our analysis. We made the mistake in gh-97561 (which he cleaned up and committed but didn't write) not to add a test. :-(

@ordinary-jamie
Copy link
Contributor Author

@ordinary-jamie Can you look into the test failures?

Sorry! Didn't notice these yesterday. Took a look at it appears our issue it because the Lib/test/ssltests.py script flags -Werror

def run_regrtests(*extra_args):
print(ssl.OPENSSL_VERSION)
args = [
sys.executable,
'-Werror', '-bb', # turn warnings into exceptions

Pushed a fix to temporarily fix the warning filter for this case

Comment on lines 245 to 246
with warnings.catch_warnings(record=True) as w:
warnings.simplefilter("default")
Copy link
Member

Choose a reason for hiding this comment

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

I see. It seems that in tests, using catch_warnings() requires simplefilter(), otherwise the test fails when using -Werror. Is there any particular reason you're not using assertWarns() though? It takes care of this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No reason (besides me not knowing about that method, sorry). Updated!

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

Now you know. :-)

Looks good, I'll merge.

@gvanrossum gvanrossum enabled auto-merge (squash) February 19, 2024 00:00
@gvanrossum gvanrossum merged commit 53d5e67 into python:main Feb 19, 2024
@miss-islington-app
Copy link

Thanks @ordinary-jamie for the PR, and @gvanrossum for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Feb 19, 2024
…ult_executor (pythonGH-115622)

(cherry picked from commit 53d5e67)

Co-authored-by: Jamie Phan <[email protected]>
@bedevere-app
Copy link

bedevere-app bot commented Feb 19, 2024

GH-115641 is a backport of this pull request to the 3.12 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.12 only security fixes label Feb 19, 2024
gvanrossum pushed a commit that referenced this pull request Feb 19, 2024
…ault_executor (GH-115622) (#115641)

(cherry picked from commit 53d5e67)

Co-authored-by: Jamie Phan <[email protected]>
woodruffw pushed a commit to woodruffw-forks/cpython that referenced this pull request Mar 4, 2024
diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024
LukasWoodtli pushed a commit to LukasWoodtli/cpython that referenced this pull request Jan 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants