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

Support --before-graceful-exit #2242

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

abersheeran
Copy link
Member

@abersheeran abersheeran commented Feb 1, 2024

Summary

About #1936

The PR will solved https://stackoverflow.com/questions/58133694/graceful-shutdown-of-uvicorn-starlette-app-with-websockets

Checklist

  • I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • I've updated the documentation accordingly.

@abersheeran abersheeran enabled auto-merge (squash) February 1, 2024 07:15
@Kludex Kludex disabled auto-merge February 1, 2024 07:24
@abersheeran abersheeran requested a review from a team February 7, 2024 07:14
@marcinsulikowski
Copy link
Contributor

This PR adds a quite useful feature. One of the applications where I am using uvicorn has clients that use long polling to get notifications from the server using plain HTTP 1.1 and every time the application is stopped, it logs errors like this one:

2024-02-05 17:13:18,634 INFO Waiting for connections to close. (CTRL+C to force quit)
2024-02-05 17:13:23,635 ERROR Cancel 1 running task(s), timeout graceful shutdown exceeded
2024-02-05 17:13:23,638 INFO Waiting for application shutdown.
2024-02-05 17:13:23,652 ERROR Exception in ASGI application
Traceback (most recent call last):
  File "anyio/streams/memory.py", line 98, in receive
  File "anyio/streams/memory.py", line 93, in receive_nowait
anyio.WouldBlock

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "uvicorn/protocols/http/h11_impl.py", line 408, in run_asgi
  File "uvicorn/middleware/proxy_headers.py", line 84, in __call__
  File "fastapi/applications.py", line 1106, in __call__
  (...)
  File "anyio/streams/memory.py", line 106, in receive
  File "anyio/_backends/_asyncio.py", line 1778, in wait
  File "asyncio/locks.py", line 212, in wait
asyncio.exceptions.CancelledError: Task cancelled, timeout graceful shutdown exceeded

The hook added by this PR would make it possible to avoid such errors without any tricks like patching uvicorn's internal methods.

@adriangb
Copy link
Member

adriangb commented Feb 10, 2024

Could you use https://www.uvicorn.org/deployment/#running-programmatically?

never mind I have had misunderstood the problem

uvicorn/config.py Outdated Show resolved Hide resolved
@Kludex Kludex added the hold Don't merge yet label Feb 10, 2024
@abersheeran
Copy link
Member Author

@Kludex Do you have any other opinions? I noticed that you added a "hold" tag.

@Kludex
Copy link
Member

Kludex commented Feb 10, 2024

Yes, we didn't discuss this properly.

Give me a chance to review the previous issues, and discussions, please.

@abersheeran
Copy link
Member Author

If there is no problem, can this PR be merged? It solves a problem that has been around for years.

@Kludex
Copy link
Member

Kludex commented Feb 21, 2024

I would like to take a look first.

Comment on lines +274 to +277
if callable(self.config.before_graceful_exit_hook):
f = self.config.before_graceful_exit_hook()
if inspect.isawaitable(f):
await f

Choose a reason for hiding this comment

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

Is it possible to place this code prior to the point where new connections are no longer accepted (#Stop accepting new connections)?

This could help in resolving the matter we have mentioned in #2257. Additionally, since the topic at hand is related to "before graceful exit," it would be good to execute the code beforehand, before the acceptance of any further connections is stopped.

WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

I have no objection to this, for me these two positions are the same.

Choose a reason for hiding this comment

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

@adriangb @Kludex , what are you thoughts on this?

@abersheeran
Copy link
Member Author

ping @Kludex

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hold Don't merge yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants