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.

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

Jump to bottom

fix: make sure always execute the shutdown logic #2510

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

fix: make sure always execute the shutdown logic #2510

wants to merge 4 commits into from
+7 −6

Conversation

xianml
Copy link

@xianml xianml commented Nov 13, 2024 •
edited
Loading

make sure always execute the shutdown logic, even if server received a signal before entering main_loop

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 3 times, most recently from c999e6e to 05b1ec2 Compare November 26, 2024 02:13
@xianml
Copy link
Author

xianml commented Dec 16, 2024

@Kludex Could you please help review and approve?

@xianml
Copy link
Author

xianml commented Dec 25, 2024

@florimondmanca Hello. sorry to loop you in. Not very sure if you are the right person for reviewing this PR. Just saw you have ever touched this part of the code. Could you please review this one?

@xianml
Copy link
Author

xianml commented Jan 7, 2025

hi @Kludex Happy new year!!
btw, any comments?

@Kludex
Copy link
Member

Kludex commented Jan 7, 2025

Please create a discussion with a MRE. As I said before, this goes against the ASGI specification.

@xianml
Copy link
Author

xianml commented Jan 13, 2025

Please create a discussion with a MRE. As I said before, this goes against the ASGI specification.

hello @Kludex Could you please elaborate which rules of ASGI specification will against?

@xianml xianml changed the title fix: make sure always execute the shutdown logic, even if server rece… fix: make sure always execute the shutdown logic Jan 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@xianml @Kludex