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

fix: make sure always execute the shutdown logic, even if server rece… #2510

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

Conversation

xianml
Copy link

@xianml xianml commented Nov 13, 2024

make sure always execute the shutdown logic, even if server received a signal during startup
before: can not shutdown server when i try to shutdown a server during startup

image

after:
image

Summary

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.

@Kludex
Copy link
Member

Kludex commented Nov 20, 2024

This doesn't follow the ASGI specification. There are related issues, please search them on https://github.com/encode/uvicorn/issues.

@Kludex Kludex closed this Nov 20, 2024
@xianml
Copy link
Author

xianml commented Nov 21, 2024

@Kludex actually it is not the same as #1301 etc.
no matter or what, the initialization will run to complete according to ASGI specification. But what i want to change is uvicorn always need to run server.close to shutdown the server and close the socket. But currently, some of the process will not got the chance to execute the shutdown logic.

Please help review again. I am not to break the ASGI specification, just want to make sure the shutdown logic is comprehensive.

@Kludex Kludex reopened this Nov 21, 2024
@xianml xianml force-pushed the fix/shutdown branch 2 times, most recently from 753921c to 211388f Compare November 21, 2024 14:41
@xianml
Copy link
Author

xianml commented Dec 16, 2024

@Kludex Could you please help review and approve?

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