Add check if childs are still alive and shutdown instead of hanging #1177
Conversation
There was a problem hiding this comment.
@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.
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.
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.