-
-
Notifications
You must be signed in to change notification settings - Fork 756
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
Simplified worker monitoring and restart, without gunicorn. #1942
Changes from all commits
365bc8e
5db1e47
1672371
dbf1eb7
750d809
a47cfb8
dcadc64
fd73701
e4ffbae
e62c96e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,7 @@ on: | |
branches: ["master"] | ||
pull_request: | ||
branches: ["master"] | ||
workflow_dispatch: | ||
|
||
jobs: | ||
tests: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,7 @@ | |
import os | ||
import signal | ||
import threading | ||
import time | ||
from multiprocessing.context import SpawnProcess | ||
from socket import socket | ||
from types import FrameType | ||
|
@@ -42,7 +43,7 @@ def signal_handler(self, sig: int, frame: Optional[FrameType]) -> None: | |
|
||
def run(self) -> None: | ||
self.startup() | ||
self.should_exit.wait() | ||
self.monitor() | ||
self.shutdown() | ||
|
||
def startup(self) -> None: | ||
|
@@ -72,3 +73,18 @@ def shutdown(self) -> None: | |
click.style(str(self.pid), fg="cyan", bold=True) | ||
) | ||
logger.info(message, extra={"color_message": color_message}) | ||
|
||
def monitor(self) -> None: | ||
while not self.should_exit.is_set(): | ||
time.sleep(0.5) | ||
if self.should_exit.is_set(): | ||
break | ||
gnat marked this conversation as resolved.
Show resolved
Hide resolved
|
||
# Restart expired workers. | ||
for process in self.processes: | ||
if not process.is_alive(): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is not reliable. We can't know if the process is stuck. It will only give the false impression of a good process manager. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've considered this but I don't think it's reasonable to expect Uvicorn to be so involved, because determining "stuck" and the handling of "stuck" could be quite different for every project: It's up to the developer to decide if/when/how to kill a "stuck" worker. Nor is it the point of this PR. We want Uvicorn to just mimic k8s here- and only become involved when the worker is dead/crashed/exited, which is what There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On the description it says: "we're eliminating an entire dependency with only a few lines.", but gunicorn does check if the process is stuck, so it's the same scope, and this is not a replacement for it. |
||
self.processes.remove(process) | ||
process_new = get_subprocess( | ||
config=self.config, target=self.target, sockets=self.sockets | ||
) | ||
process_new.start() | ||
self.processes.append(process_new) |
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.
Why did you add this?
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.
This enables dev branches to manually run the test suite.
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.
Without creating a PR you mean?
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.
Yes, that's correct. It allows for running the test suite manually on your own fork or branch prior to creating a pull request.