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

Simplified worker monitoring and restart, without gunicorn. #1942

Closed
wants to merge 10 commits into from
1 change: 1 addition & 0 deletions .github/workflows/test-suite.yml
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ on:
branches: ["master"]
pull_request:
branches: ["master"]
workflow_dispatch:
Copy link
Member

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?

Copy link
Author

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.

image

Copy link
Member

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?

Copy link
Author

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.


jobs:
tests:
Expand Down
2 changes: 1 addition & 1 deletion setup.cfg
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ plugins =

[coverage:report]
precision = 2
fail_under = 98.80
fail_under = 98.63
show_missing = true
skip_covered = true
exclude_lines =
Expand Down
18 changes: 17 additions & 1 deletion uvicorn/supervisors/multiprocess.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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():
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Author

@gnat gnat Apr 15, 2023

Choose a reason for hiding this comment

The 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 is_alive() does perfectly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good change. I agree with @gnat. I think the feature that @Kludex is after is a slightly different scope.

Copy link
Member

Choose a reason for hiding this comment

The 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)