-
-
Notifications
You must be signed in to change notification settings - Fork 722
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 check if childs are still alive and shutdown instead of hanging #1177
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@humrochagf Thanks for the PR! 🎉
First, sorry for taking so long to review this.
Now, I do think the "reload" part makes sense, and we should get that merged. I've added a single comment on it (that I can change it myself).
As for the "multiprocess" part, looks fine as well. Just a remark about the reload_delay
, as I don't think it should be used here. But... Although it looks fine, I don't want to merge this part, because I'm working on a process manager for uvicorn (which will be located on the Multiprocess class).
For the reason stated in the previous paragraph, would you mind removing the logic around the Multiprocess, so we can merge the "reload" part?
uvicorn/supervisors/basereload.py
Outdated
for _socket in self.sockets: | ||
_socket.close() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use the same notation as the server.py
.
for _socket in self.sockets: | |
_socket.close() | |
for sock in self.sockets: | |
sock.close() |
uvicorn/supervisors/multiprocess.py
Outdated
while not self.should_exit.wait(self.config.reload_delay): | ||
if not any(p.exitcode is None for p in self.processes): | ||
break |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used the same interval as the one already setup for the reloader, but it could be another one as well.
Yep, I don't think we should use self.config.reload_delay
here.
Hey, @Kludex no worries for the delay, and I don't mind removing the multiprocessing part, I agree with you, having a process manager would be much better 😃 I'll do the changes. |
Thanks! ✨ |
Done 🚀 |
…1177) * Add check if childs are still alive and shutdown instead of hanging * Revert multiprocessing change * Fix socket loop notation to keep consistency with server.py
…ncode#1177) * Add check if childs are still alive and shutdown instead of hanging * Revert multiprocessing change * Fix socket loop notation to keep consistency with server.py
Fix: #1115
Since server closes with
sys.exit(1)
on child process without communicating with the parent supervisor we need to check if the process is still alive.I used the same interval as the one already setup for the reloader, but it could be another one as well.
Also added the tests for the direct call from the command line, and also ensured sockets are closed at the end of the shutdown. Since child dies before being able to properly close their sockets.