From df23be92695c55fd8a61f343b04e6df630662c7b Mon Sep 17 00:00:00 2001 From: Evgeniy Alekseev Date: Mon, 6 Mar 2023 01:13:41 +0200 Subject: [PATCH] gracefully terminate web server In previous revisions server was terminated by itself, thus no lock or socket was removed. In new version, graceful termination of the queue has been added as well as server now handles singals --- src/ahriman/application/handlers/web.py | 4 + src/ahriman/core/spawn.py | 106 +++++++++--------- src/ahriman/web/web.py | 2 +- .../application/handlers/test_handler_web.py | 7 +- tests/ahriman/core/test_spawn.py | 53 +++++---- tests/ahriman/web/test_web.py | 8 +- 6 files changed, 103 insertions(+), 77 deletions(-) diff --git a/src/ahriman/application/handlers/web.py b/src/ahriman/application/handlers/web.py index 6e62de43..df5ca37a 100644 --- a/src/ahriman/application/handlers/web.py +++ b/src/ahriman/application/handlers/web.py @@ -55,3 +55,7 @@ class Web(Handler): application = setup_service(architecture, configuration, spawner) run_server(application) + + # terminate spawn process at the last + spawner.stop() + spawner.join() diff --git a/src/ahriman/core/spawn.py b/src/ahriman/core/spawn.py index 1413309a..bccf45d2 100644 --- a/src/ahriman/core/spawn.py +++ b/src/ahriman/core/spawn.py @@ -60,7 +60,7 @@ class Spawn(Thread, LazyLogging): self.lock = Lock() self.active: Dict[str, Process] = {} # stupid pylint does not know that it is possible - self.queue: Queue[Tuple[str, bool]] = Queue() # pylint: disable=unsubscriptable-object + self.queue: Queue[Tuple[str, bool] | None] = Queue() # pylint: disable=unsubscriptable-object @staticmethod def process(callback: Callable[[argparse.Namespace, str], bool], args: argparse.Namespace, architecture: str, @@ -78,55 +78,7 @@ class Spawn(Thread, LazyLogging): result = callback(args, architecture) queue.put((process_id, result)) - def key_import(self, key: str, server: Optional[str]) -> None: - """ - import key to service cache - - Args: - key(str): key to import - server(str): PGP key server - """ - kwargs = {} if server is None else {"key-server": server} - self.spawn_process("service-key-import", key, **kwargs) - - def packages_add(self, packages: Iterable[str], *, now: bool) -> None: - """ - add packages - - Args: - packages(Iterable[str]): packages list to add - now(bool): build packages now - """ - kwargs = {"source": PackageSource.AUR.value} # avoid abusing by building non-aur packages - if now: - kwargs["now"] = "" - self.spawn_process("package-add", *packages, **kwargs) - - def packages_rebuild(self, depends_on: str) -> None: - """ - rebuild packages which depend on the specified package - - Args: - depends_on(str): packages dependency - """ - self.spawn_process("repo-rebuild", **{"depends-on": depends_on}) - - def packages_remove(self, packages: Iterable[str]) -> None: - """ - remove packages - - Args: - packages(Iterable[str]): packages list to remove - """ - self.spawn_process("package-remove", *packages) - - def packages_update(self, ) -> None: - """ - run full repository update - """ - self.spawn_process("repo-update") - - def spawn_process(self, command: str, *args: str, **kwargs: str) -> None: + def _spawn_process(self, command: str, *args: str, **kwargs: str) -> None: """ spawn external ahriman process with supplied arguments @@ -161,6 +113,54 @@ class Spawn(Thread, LazyLogging): with self.lock: self.active[process_id] = process + def key_import(self, key: str, server: Optional[str]) -> None: + """ + import key to service cache + + Args: + key(str): key to import + server(str): PGP key server + """ + kwargs = {} if server is None else {"key-server": server} + self._spawn_process("service-key-import", key, **kwargs) + + def packages_add(self, packages: Iterable[str], *, now: bool) -> None: + """ + add packages + + Args: + packages(Iterable[str]): packages list to add + now(bool): build packages now + """ + kwargs = {"source": PackageSource.AUR.value} # avoid abusing by building non-aur packages + if now: + kwargs["now"] = "" + self._spawn_process("package-add", *packages, **kwargs) + + def packages_rebuild(self, depends_on: str) -> None: + """ + rebuild packages which depend on the specified package + + Args: + depends_on(str): packages dependency + """ + self._spawn_process("repo-rebuild", **{"depends-on": depends_on}) + + def packages_remove(self, packages: Iterable[str]) -> None: + """ + remove packages + + Args: + packages(Iterable[str]): packages list to remove + """ + self._spawn_process("package-remove", *packages) + + def packages_update(self, ) -> None: + """ + run full repository update + """ + self._spawn_process("repo-update") + def run(self) -> None: """ thread run method @@ -174,3 +174,9 @@ class Spawn(Thread, LazyLogging): if process is not None: process.terminate() # make sure lol process.join() + + def stop(self) -> None: + """ + gracefully terminate thread + """ + self.queue.put(None) diff --git a/src/ahriman/web/web.py b/src/ahriman/web/web.py index ce9e7961..826d10b8 100644 --- a/src/ahriman/web/web.py +++ b/src/ahriman/web/web.py @@ -115,7 +115,7 @@ def run_server(application: web.Application) -> None: port = configuration.getint("web", "port") unix_socket = create_socket(configuration, application) - web.run_app(application, host=host, port=port, sock=unix_socket, handle_signals=False, + web.run_app(application, host=host, port=port, sock=unix_socket, handle_signals=True, access_log=logging.getLogger("http"), access_log_class=FilteredAccessLogger) diff --git a/tests/ahriman/application/handlers/test_handler_web.py b/tests/ahriman/application/handlers/test_handler_web.py index 91d4698a..01aee4e4 100644 --- a/tests/ahriman/application/handlers/test_handler_web.py +++ b/tests/ahriman/application/handlers/test_handler_web.py @@ -28,14 +28,19 @@ def test_run(args: argparse.Namespace, configuration: Configuration, repository: must run command """ args = _default_args(args) - mocker.patch("ahriman.core.spawn.Spawn.start") mocker.patch("ahriman.core.repository.Repository.load", return_value=repository) setup_mock = mocker.patch("ahriman.web.web.setup_service") run_mock = mocker.patch("ahriman.web.web.run_server") + start_mock = mocker.patch("ahriman.core.spawn.Spawn.start") + stop_mock = mocker.patch("ahriman.core.spawn.Spawn.stop") + join_mock = mocker.patch("ahriman.core.spawn.Spawn.join") Web.run(args, "x86_64", configuration, report=False, unsafe=False) setup_mock.assert_called_once_with("x86_64", configuration, pytest.helpers.anyvar(int)) run_mock.assert_called_once_with(pytest.helpers.anyvar(int)) + start_mock.assert_called_once_with() + stop_mock.assert_called_once_with() + join_mock.assert_called_once_with() def test_disallow_auto_architecture_run() -> None: diff --git a/tests/ahriman/core/test_spawn.py b/tests/ahriman/core/test_spawn.py index cdccac83..98f2cabc 100644 --- a/tests/ahriman/core/test_spawn.py +++ b/tests/ahriman/core/test_spawn.py @@ -36,11 +36,25 @@ def test_process_error(spawner: Spawn) -> None: assert spawner.queue.empty() +def test_spawn_process(spawner: Spawn, mocker: MockerFixture) -> None: + """ + must correctly spawn child process + """ + start_mock = mocker.patch("multiprocessing.Process.start") + + spawner._spawn_process("add", "ahriman", now="", maybe="?") + start_mock.assert_called_once_with() + spawner.args_parser.parse_args.assert_called_once_with([ + "--architecture", spawner.architecture, "--configuration", str(spawner.configuration.path), + "add", "ahriman", "--now", "--maybe", "?" + ]) + + def test_key_import(spawner: Spawn, mocker: MockerFixture) -> None: """ must call key import """ - spawn_mock = mocker.patch("ahriman.core.spawn.Spawn.spawn_process") + spawn_mock = mocker.patch("ahriman.core.spawn.Spawn._spawn_process") spawner.key_import("0xdeadbeaf", None) spawn_mock.assert_called_once_with("service-key-import", "0xdeadbeaf") @@ -49,7 +63,7 @@ def test_key_import_with_server(spawner: Spawn, mocker: MockerFixture) -> None: """ must call key import with server specified """ - spawn_mock = mocker.patch("ahriman.core.spawn.Spawn.spawn_process") + spawn_mock = mocker.patch("ahriman.core.spawn.Spawn._spawn_process") spawner.key_import("0xdeadbeaf", "keyserver.ubuntu.com") spawn_mock.assert_called_once_with("service-key-import", "0xdeadbeaf", **{"key-server": "keyserver.ubuntu.com"}) @@ -58,7 +72,7 @@ def test_packages_add(spawner: Spawn, mocker: MockerFixture) -> None: """ must call package addition """ - spawn_mock = mocker.patch("ahriman.core.spawn.Spawn.spawn_process") + spawn_mock = mocker.patch("ahriman.core.spawn.Spawn._spawn_process") spawner.packages_add(["ahriman", "linux"], now=False) spawn_mock.assert_called_once_with("package-add", "ahriman", "linux", source="aur") @@ -67,7 +81,7 @@ def test_packages_add_with_build(spawner: Spawn, mocker: MockerFixture) -> None: """ must call package addition with update """ - spawn_mock = mocker.patch("ahriman.core.spawn.Spawn.spawn_process") + spawn_mock = mocker.patch("ahriman.core.spawn.Spawn._spawn_process") spawner.packages_add(["ahriman", "linux"], now=True) spawn_mock.assert_called_once_with("package-add", "ahriman", "linux", source="aur", now="") @@ -76,7 +90,7 @@ def test_packages_rebuild(spawner: Spawn, mocker: MockerFixture) -> None: """ must call package rebuild """ - spawn_mock = mocker.patch("ahriman.core.spawn.Spawn.spawn_process") + spawn_mock = mocker.patch("ahriman.core.spawn.Spawn._spawn_process") spawner.packages_rebuild("python") spawn_mock.assert_called_once_with("repo-rebuild", **{"depends-on": "python"}) @@ -85,7 +99,7 @@ def test_packages_remove(spawner: Spawn, mocker: MockerFixture) -> None: """ must call package removal """ - spawn_mock = mocker.patch("ahriman.core.spawn.Spawn.spawn_process") + spawn_mock = mocker.patch("ahriman.core.spawn.Spawn._spawn_process") spawner.packages_remove(["ahriman", "linux"]) spawn_mock.assert_called_once_with("package-remove", "ahriman", "linux") @@ -94,25 +108,11 @@ def test_packages_update(spawner: Spawn, mocker: MockerFixture) -> None: """ must call repo update """ - spawn_mock = mocker.patch("ahriman.core.spawn.Spawn.spawn_process") + spawn_mock = mocker.patch("ahriman.core.spawn.Spawn._spawn_process") spawner.packages_update() spawn_mock.assert_called_once_with("repo-update") -def test_spawn_process(spawner: Spawn, mocker: MockerFixture) -> None: - """ - must correctly spawn child process - """ - start_mock = mocker.patch("multiprocessing.Process.start") - - spawner.spawn_process("add", "ahriman", now="", maybe="?") - start_mock.assert_called_once_with() - spawner.args_parser.parse_args.assert_called_once_with([ - "--architecture", spawner.architecture, "--configuration", str(spawner.configuration.path), - "add", "ahriman", "--now", "--maybe", "?" - ]) - - def test_run(spawner: Spawn, mocker: MockerFixture) -> None: """ must implement run method @@ -145,3 +145,14 @@ def test_run_pop(spawner: Spawn) -> None: second.terminate.assert_called_once_with() second.join.assert_called_once_with() assert not spawner.active + + +def test_stop(spawner: Spawn) -> None: + """ + must gracefully terminate thread + """ + spawner.start() + spawner.stop() + spawner.join() + + assert not spawner.is_alive() diff --git a/tests/ahriman/web/test_web.py b/tests/ahriman/web/test_web.py index 5a0e80dd..49cede47 100644 --- a/tests/ahriman/web/test_web.py +++ b/tests/ahriman/web/test_web.py @@ -100,7 +100,7 @@ def test_run(application: web.Application, mocker: MockerFixture) -> None: run_server(application) run_application_mock.assert_called_once_with( - application, host="127.0.0.1", port=port, sock=None, handle_signals=False, + application, host="127.0.0.1", port=port, sock=None, handle_signals=True, access_log=pytest.helpers.anyvar(int), access_log_class=FilteredAccessLogger ) @@ -115,7 +115,7 @@ def test_run_with_auth(application_with_auth: web.Application, mocker: MockerFix run_server(application_with_auth) run_application_mock.assert_called_once_with( - application_with_auth, host="127.0.0.1", port=port, sock=None, handle_signals=False, + application_with_auth, host="127.0.0.1", port=port, sock=None, handle_signals=True, access_log=pytest.helpers.anyvar(int), access_log_class=FilteredAccessLogger ) @@ -130,7 +130,7 @@ def test_run_with_debug(application_with_debug: web.Application, mocker: MockerF run_server(application_with_debug) run_application_mock.assert_called_once_with( - application_with_debug, host="127.0.0.1", port=port, sock=None, handle_signals=False, + application_with_debug, host="127.0.0.1", port=port, sock=None, handle_signals=True, access_log=pytest.helpers.anyvar(int), access_log_class=FilteredAccessLogger ) @@ -147,6 +147,6 @@ def test_run_with_socket(application: web.Application, mocker: MockerFixture) -> run_server(application) socket_mock.assert_called_once_with(application["configuration"], application) run_application_mock.assert_called_once_with( - application, host="127.0.0.1", port=port, sock=42, handle_signals=False, + application, host="127.0.0.1", port=port, sock=42, handle_signals=True, access_log=pytest.helpers.anyvar(int), access_log_class=FilteredAccessLogger )