Skip to content
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

Add --timeout-graceful-shutdown parameter #1824

Closed
wants to merge 2 commits into from

Conversation

Kludex
Copy link
Member

@Kludex Kludex commented Dec 30, 2022

Description

This PR adds the --timeout-graceful-shutdown parameter, which sets a timeout on tasks that the server is waiting to perform the shutdown lifespan event.

This timeout doesn't act on the shutdown lifespan event i.e. the shutdown event is supposed to run even in the scenario the tasks are timed out.

I need to think of a nice test for this. Ideas?

Notes

  1. The --graceful-timeout from gunicorn is conceptually different from --timeout-graceful-shutdown. The first forcefully kills the worker, and the latter cancels the tasks that are running after uvicorn said "I'm not receiving more requests, I'm just going to finish some tasks".

  2. An alternative name for this parameter could be --timeout-tasks-shutdown, or similar.

  3. The logs are horrible when a task is cancelled. Should it be fine tho? 🤷

Missing

  • Add an entry on the documentation, and mention the difference with --graceful-timeout from gunicorn.
  • Add a test case.

Copy link
Contributor

@JonasKs JonasKs left a comment

Choose a reason for hiding this comment

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

Reminds me of python-arq/arq#345 😁 I'd love to see this merged.
As for a test, I agree, not an obvious pretty comes to mind.
I think I would've went for caplog test on this one personally.

@Kludex
Copy link
Member Author

Kludex commented Jan 16, 2023

The problem on this PR is that run_asgi task gets cancelled, and there's a horrible asyncio.CancelledError message.

Also, I guess we need to cancel the tasks (?) in case TimeoutError happens. 🤔

@Kludex Kludex mentioned this pull request Mar 10, 2023
16 tasks
@Kludex
Copy link
Member Author

Kludex commented Apr 25, 2023

@Kludex Kludex closed this Apr 25, 2023
@Kludex Kludex deleted the feat/timeout-graceful-shutdown branch April 25, 2023 18:55
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.

Make force_exit configure-able from command line How do I cancel a streaming response on shutdown?
2 participants