-
-
Notifications
You must be signed in to change notification settings - Fork 761
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
Create ProcessManager #1205
Closed
Closed
Create ProcessManager #1205
Changes from all commits
Commits
Show all changes
28 commits
Select commit
Hold shift + click to select a range
4155eba
Add deprecation warning to worker
Kludex 8445da0
Merge remote-tracking branch 'origin/master' into feat/process-manager
Kludex effb125
Create ProcessManager class
Kludex 8c5dcd0
Create ProcessManager class
Kludex 84e9d8e
Spawn workers
d4e872c
merge
Kludex fe26320
More implemented
Kludex eaa7014
Add PR reference to worker deprecation warning
Kludex 6b0d6d6
Fix lint
Kludex 2c63961
Exiting loop successfully
Kludex 1f998e6
Server shutdown with the right code
Kludex 5f2df64
Able to restart workers
Kludex 88ac47d
Fix alive condition
Kludex 518075d
Add graceful timeout and handle_sigquit
Kludex 3711cb6
Remove wrong process removal
Kludex b789f89
Remove print statements
Kludex 9c7b643
Remove Multiprocess
Kludex 467fbf4
Fix tests
Kludex f8a592e
Fix mypy
Kludex 6807907
Remove SIGHUP - Windows doesn't support
Kludex b42b159
ProcessManager available only to Unix systems
Kludex b666e13
Remove unused files
Kludex 0474cb7
Remove unused import
Kludex 9a54880
Fix linter
Kludex 79e2fb4
Run ProcessManager only on Unix
Kludex 678ac82
Fix tests
Kludex 348f001
Import only Multiprocess
Kludex a4377a1
Fix multiprocess test
Kludex File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,157 @@ | ||
import enum | ||
import logging | ||
import multiprocessing as mp | ||
import os | ||
import queue | ||
import signal | ||
import sys | ||
import time | ||
from multiprocessing.context import SpawnProcess | ||
from socket import socket | ||
from types import FrameType | ||
from typing import Callable, List, Optional | ||
|
||
from uvicorn.config import Config | ||
from uvicorn.subprocess import get_subprocess | ||
|
||
if sys.version_info >= (3, 8): | ||
from typing import Protocol | ||
else: | ||
from typing_extensions import Protocol | ||
|
||
logger = logging.getLogger("uvicorn.error") | ||
|
||
|
||
class Target(Protocol): | ||
def __call__(self, sockets: Optional[List[socket]] = None) -> None: | ||
... | ||
|
||
|
||
class ExitCode(enum.IntEnum): | ||
OK = 0 | ||
DEFAULT_FAILURE = 1 | ||
STARTUP_FAILURE = 3 | ||
|
||
|
||
class ProcessManager: | ||
# Complete list of signals can be found on: | ||
# http://manpages.ubuntu.com/manpages/bionic/man7/signal.7.html | ||
SIGNALS = { | ||
getattr(signal, f"SIG{sig.upper()}"): sig | ||
for sig in ( | ||
"abrt", # Abort signal from abort(3) | ||
"hup", # Hangup signal generated by terminal close. | ||
"quit", # Quit signal generated by terminal close. | ||
"int", # Interrupt signal generated by Ctrl+C | ||
"term", # Termination signal | ||
"winch", # Window size change signal | ||
"chld", # Child process terminated, stopped, or continued | ||
) | ||
} | ||
|
||
# TODO(Marcelo): This should be converted into a CLI option. | ||
GRACEFUL_TIMEOUT = 30 | ||
|
||
def __init__(self, config: Config, target: Target, sockets: List[socket]) -> None: | ||
self.config = config | ||
self.target = target | ||
self.sockets = sockets | ||
self.processes: List[SpawnProcess] = [] | ||
self.sig_queue = mp.Queue() | ||
|
||
def run(self) -> None: | ||
self.start() | ||
|
||
try: | ||
self.spawn_processes() | ||
|
||
while True: | ||
try: | ||
sig = self.sig_queue.get(timeout=0.25) | ||
except queue.Empty: | ||
self.reap_processes() | ||
self.spawn_processes() | ||
continue | ||
|
||
if sig not in self.SIGNALS.keys(): | ||
logger.info("Ignoring unknown signal: %d", sig) | ||
continue | ||
|
||
handler = self.signal_handler(sig) | ||
if handler is None: | ||
logger.info("Unhandled signal: %s", self.SIGNALS.get(sig)) | ||
continue | ||
|
||
handler() | ||
except StopIteration: | ||
self.halt() | ||
except Exception: | ||
logger.info("Unhandled exception in main loop", exc_info=True) | ||
self.stop(signal.SIGTERM) | ||
self.halt(ExitCode.DEFAULT_FAILURE) | ||
|
||
def start(self) -> None: | ||
self.pid = os.getpid() | ||
logger.info("Started manager process [%d]", self.pid) | ||
self.init_signals() | ||
|
||
def spawn_processes(self) -> None: | ||
for _ in range(self.config.workers - len(self.processes)): | ||
self.spawn_process() | ||
|
||
def spawn_process(self) -> None: | ||
process = get_subprocess(self.config, target=self.target, sockets=self.sockets) | ||
process.start() | ||
self.processes.append(process) | ||
|
||
def init_signals(self) -> None: | ||
for s in self.SIGNALS.keys(): | ||
signal.signal(s, self._signal) | ||
signal.signal(signal.SIGCHLD, self.handle_chld) | ||
|
||
def _signal(self, sig: signal.Signals, frame: FrameType) -> None: | ||
self.sig_queue.put(sig) | ||
|
||
def handle_int(self) -> None: | ||
self.stop(signal.SIGINT) | ||
raise StopIteration | ||
|
||
def handle_term(self) -> None: | ||
self.stop(signal.SIGTERM) | ||
raise StopIteration | ||
|
||
def handle_chld(self, sig: signal.Signals, frame: FrameType) -> None: | ||
self.reap_processes() | ||
|
||
def signal_handler(self, sig: signal.Signals) -> Optional[Callable[..., None]]: | ||
sig_name = self.SIGNALS.get(sig) | ||
return getattr(self, f"handle_{sig_name}", None) | ||
|
||
def reap_processes(self) -> None: | ||
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. I need to further check if the |
||
self.processes.remove(process) | ||
|
||
def halt(self, exit_code: int = ExitCode.OK) -> None: | ||
logger.info("Stopping parent process [%d]", self.pid) | ||
sys.exit(exit_code) | ||
|
||
def kill_processes(self, sig: signal.Signals) -> None: | ||
for process in self.processes: | ||
self.kill_process(process, sig) | ||
|
||
def kill_process(self, process: SpawnProcess, sig: signal.Signals) -> None: | ||
os.kill(process.pid, sig) | ||
|
||
def wait_timeout(self) -> None: | ||
limit = time.time() + self.GRACEFUL_TIMEOUT | ||
while self.processes and time.time() < limit: | ||
time.sleep(0.1) | ||
|
||
def stop(self, sig: signal.Signals) -> None: | ||
self.kill_processes(sig) | ||
self.wait_timeout() | ||
self.kill_processes(signal.SIGKILL) | ||
|
||
for sock in self.sockets: | ||
sock.close() |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
If there are plans to support Windows, I would be happy to help. (I read the code carefully, it is not difficult to support Windows)
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.
My problem is with the signals not supported by Windows, what do you expect to do on that part? I just added the minimal signal handlers, but further work should not work.
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.
We can use process.terminate() replace SIGINT. Other signals are transmitted as they are.
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.
In fact, a large number of other signals cannot be triggered on Windows. We can give Windows users a near-identical development experience as long as we can handle the signals raised by Ctrl-C in a unified manner.
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 was not referring to
SIGINT
,SIGINT
andSIGTERM
are not an issue, and I don't see any issue of callingprocess.terminate()
onSIGINT
. I mean, I don't think there's an issue on that part for windows users.The issue right now is theThe issue is with theinit_signals()
method, that adds a handler for signals that Windows doesn't have. If we just add a conditional to createSIGNALS
attribute according to the OS, then it will work.SIGCHLD
, that Windows doesn't support.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.
https://github.com/Kludex/uvicorn/blob/a4377a14e3360fa487c506b49e8c5537fb0057a1/uvicorn/supervisors/manager.py#L71-L74
Maybe don't need SIGCHLD in Windows? The delay of 0.25 is not high.
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, I agree. It is a good idea to incorporate the reload here.
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.
#684 Sometime,
os.kill()
maybe got an error.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 see! You mean that it's redundant to have the handler as we already perform the same logic in case we don't handle the
SIGCHLD
.I need to recheck if there's further logic to be implemented on the
SIGCHLD
handler that I didn't add because it's not needed on this initial step. If I can't find anything, then yes, we can remove it.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 was not defending the idea of adding the reload here, as I think it's a posterior discussion. But I do agree with that, if we do that, we're going to have only this
ProcessManager
class, and both the Reload classes and theMultiprocess
class should be removed.