From b43df8e094cf727edb28334f97fe90633ea7649e Mon Sep 17 00:00:00 2001 From: Evgenii Alekseev Date: Mon, 17 Jun 2024 12:00:49 +0300 Subject: [PATCH] improve lock mechanisms --- package/archlinux/ahriman.tmpfiles | 3 +- src/ahriman/application/ahriman.py | 13 +- src/ahriman/application/lock.py | 121 ++++++++++---- tests/ahriman/application/test_ahriman.py | 3 +- tests/ahriman/application/test_lock.py | 187 +++++++++++++++------- 5 files changed, 225 insertions(+), 102 deletions(-) diff --git a/package/archlinux/ahriman.tmpfiles b/package/archlinux/ahriman.tmpfiles index 6a1b6bad..1929a150 100644 --- a/package/archlinux/ahriman.tmpfiles +++ b/package/archlinux/ahriman.tmpfiles @@ -1 +1,2 @@ -d /var/lib/ahriman 0755 ahriman ahriman \ No newline at end of file +d /var/lib/ahriman 0755 ahriman ahriman +d /run/ahriman 0755 ahriman ahriman \ No newline at end of file diff --git a/src/ahriman/application/ahriman.py b/src/ahriman/application/ahriman.py index 4713fdf8..510d63db 100644 --- a/src/ahriman/application/ahriman.py +++ b/src/ahriman/application/ahriman.py @@ -19,7 +19,6 @@ # # pylint: disable=too-many-lines import argparse -import tempfile from pathlib import Path from typing import TypeVar @@ -73,8 +72,7 @@ def _parser() -> argparse.ArgumentParser: parser.add_argument("-c", "--configuration", help="configuration path", type=Path, default=Path("/") / "etc" / "ahriman.ini") parser.add_argument("--force", help="force run, remove file lock", action="store_true") - parser.add_argument("-l", "--lock", help="lock file", type=Path, - default=Path(tempfile.gettempdir()) / "ahriman.lock") + parser.add_argument("-l", "--lock", help="lock file", type=Path, default=Path("ahriman.pid")) parser.add_argument("--log-handler", help="explicit log handler specification. If none set, the handler will be " "guessed from environment", type=LogHandler, choices=enum_values(LogHandler)) @@ -628,8 +626,7 @@ def _set_repo_daemon_parser(root: SubParserAction) -> argparse.ArgumentParser: parser.add_argument("-y", "--refresh", help="download fresh package databases from the mirror before actions, " "-yy to force refresh even if up to date", action="count", default=False) - parser.set_defaults(handler=handlers.Daemon, exit_code=False, - lock=Path(tempfile.gettempdir()) / "ahriman-daemon.lock", package=[]) + parser.set_defaults(handler=handlers.Daemon, exit_code=False, lock=Path("ahriman-daemon.pid"), package=[]) return parser @@ -880,7 +877,7 @@ def _set_service_clean_parser(root: SubParserAction) -> argparse.ArgumentParser: action=argparse.BooleanOptionalAction, default=False) parser.add_argument("--pacman", help="clear directory with pacman local database cache", action=argparse.BooleanOptionalAction, default=False) - parser.set_defaults(handler=handlers.Clean, quiet=True, unsafe=True) + parser.set_defaults(handler=handlers.Clean, lock=None, quiet=True, unsafe=True) return parser @@ -1139,8 +1136,8 @@ def _set_web_parser(root: SubParserAction) -> argparse.ArgumentParser: argparse.ArgumentParser: created argument parser """ parser = root.add_parser("web", help="web server", description="start web server", formatter_class=_formatter) - parser.set_defaults(handler=handlers.Web, architecture="", lock=Path(tempfile.gettempdir()) / "ahriman-web.lock", - report=False, repository="", parser=_parser) + parser.set_defaults(handler=handlers.Web, architecture="", lock=Path("ahriman-web.pid"), report=False, + repository="", parser=_parser) return parser diff --git a/src/ahriman/application/lock.py b/src/ahriman/application/lock.py index 58b0cdc3..cd6fc214 100644 --- a/src/ahriman/application/lock.py +++ b/src/ahriman/application/lock.py @@ -18,7 +18,10 @@ # along with this program. If not, see . # import argparse +import fcntl +import os +from io import TextIOWrapper from pathlib import Path from types import TracebackType from typing import Literal, Self @@ -36,7 +39,7 @@ from ahriman.models.waiter import Waiter class Lock(LazyLogging): """ - wrapper for application lock file + wrapper for application lock file. Credits for idea to https://github.com/bmhatfield/python-pidfile.git Attributes: force(bool): remove lock file on start if any @@ -70,8 +73,13 @@ class Lock(LazyLogging): repository_id(RepositoryId): repository unique identifier configuration(Configuration): configuration instance """ - self.path: Path | None = \ - args.lock.with_stem(f"{args.lock.stem}_{repository_id.id}") if args.lock is not None else None + self.path: Path | None = None + if args.lock is not None: + self.path = args.lock.with_stem(f"{args.lock.stem}_{repository_id.id}") + if not self.path.is_absolute(): + # prepend full path to the lock file + self.path = Path("/") / "run" / "ahriman" / self.path + self._pid_file: TextIOWrapper | None = None self.force: bool = args.force self.unsafe: bool = args.unsafe @@ -80,6 +88,65 @@ class Lock(LazyLogging): self.paths = configuration.repository_paths self.reporter = Client.load(repository_id, configuration, report=args.report) + @staticmethod + def perform_lock(fd: int) -> bool: + """ + perform file lock + + Args: + fd(int): file descriptor: + + Returns: + bool: True in case if file is locked and False otherwise + """ + try: + fcntl.flock(fd, fcntl.LOCK_EX | fcntl.LOCK_NB) + except OSError: + return False + + return True + + def _open(self) -> None: + """ + create lock file + """ + if self.path is None: + return + self._pid_file = self.path.open("a+") + + def _watch(self) -> None: + """ + watch until lock disappear + """ + # there are reasons why we are not using inotify here. First of all, if we would use it, it would bring to + # race conditions because multiple processes will be notified at the same time. Secondly, it is good library, + # but platform-specific, and we only need to check if file exists + if self._pid_file is None: + return + + waiter = Waiter(self.wait_timeout) + waiter.wait(self.perform_lock, self._pid_file.fileno()) + + def _write(self) -> None: + """ + write pid to the lock file + + Raises: + DuplicateRunError: if it cannot lock PID file + """ + if self._pid_file is None: + return + if not self.perform_lock(self._pid_file.fileno()): + raise DuplicateRunError + + self._pid_file.seek(0) # reset bytes + self._pid_file.truncate() + + self._pid_file.write(str(os.getpid())) # write current pid + self._pid_file.flush() # flush data to disk + + self._pid_file.seek(0) # reset position again + def check_user(self) -> None: """ check if current user is actually owner of ahriman root @@ -100,46 +167,33 @@ class Lock(LazyLogging): """ remove lock file """ - if self.path is None: - return - self.path.unlink(missing_ok=True) + if self._pid_file is not None: # close file descriptor + try: + self._pid_file.close() + except IOError: + pass # suppress any IO errors occur + if self.path is not None: # remove lock file + self.path.unlink(missing_ok=True) - def create(self) -> None: + def lock(self) -> None: """ - create lock file - - Raises: - DuplicateRunError: if lock exists and no force flag supplied + create pid file """ - if self.path is None: - return - try: - self.path.touch(exist_ok=self.force) - except FileExistsError: - raise DuplicateRunError from None - - def watch(self) -> None: - """ - watch until lock disappear - """ - # there are reasons why we are not using inotify here. First of all, if we would use it, it would bring to - # race conditions because multiple processes will be notified in the same time. Secondly, it is good library, - # but platform-specific, and we only need to check if file exists - if self.path is None: - return - - waiter = Waiter(self.wait_timeout) - waiter.wait(self.path.is_file) + if self.force: # remove lock if force flag is set + self.clear() + self._open() + self._watch() + self._write() def __enter__(self) -> Self: """ default workflow is the following: #. Check user UID - #. Check if there is lock file #. Check web status watcher status + #. Open lock file #. Wait for lock file to be free - #. Create lock file and directory tree + #. Write current PID to the lock file #. Report to status page if enabled Returns: @@ -147,8 +201,7 @@ class Lock(LazyLogging): """ self.check_user() self.check_version() - self.watch() - self.create() + self.lock() self.reporter.status_update(BuildStatusEnum.Building) return self diff --git a/tests/ahriman/application/test_ahriman.py b/tests/ahriman/application/test_ahriman.py index d4121150..06acc232 100644 --- a/tests/ahriman/application/test_ahriman.py +++ b/tests/ahriman/application/test_ahriman.py @@ -1097,9 +1097,10 @@ def test_subparsers_repo_update_option_refresh(parser: argparse.ArgumentParser) def test_subparsers_service_clean(parser: argparse.ArgumentParser) -> None: """ - service-clean command must imply quiet and unsafe + service-clean command must imply lock, quiet and unsafe """ args = parser.parse_args(["service-clean"]) + assert args.lock is None assert args.quiet assert args.unsafe diff --git a/tests/ahriman/application/test_lock.py b/tests/ahriman/application/test_lock.py index 1a0b3b03..dafca3ad 100644 --- a/tests/ahriman/application/test_lock.py +++ b/tests/ahriman/application/test_lock.py @@ -1,10 +1,12 @@ import argparse +import fcntl +import os import pytest -import tempfile from pathlib import Path from pytest_mock import MockerFixture -from unittest.mock import call as MockCall +from tempfile import NamedTemporaryFile +from unittest.mock import MagicMock, call as MockCall from ahriman import __version__ from ahriman.application.lock import Lock @@ -22,14 +24,102 @@ def test_path(args: argparse.Namespace, configuration: Configuration) -> None: assert Lock(args, repository_id, configuration).path is None - args.lock = Path("/run/ahriman.lock") - assert Lock(args, repository_id, configuration).path == Path("/run/ahriman_x86_64-aur-clone.lock") + args.lock = Path("/run/ahriman.pid") + assert Lock(args, repository_id, configuration).path == Path("/run/ahriman_x86_64-aur-clone.pid") + + args.lock = Path("ahriman.pid") + assert Lock(args, repository_id, configuration).path == Path("/run/ahriman/ahriman_x86_64-aur-clone.pid") with pytest.raises(ValueError): args.lock = Path("/") assert Lock(args, repository_id, configuration).path # special case +def test_perform_lock(mocker: MockerFixture) -> None: + """ + must lock file with fcntl + """ + flock_mock = mocker.patch("fcntl.flock") + assert Lock.perform_lock(1) + flock_mock.assert_called_once_with(1, fcntl.LOCK_EX | fcntl.LOCK_NB) + + +def test_perform_lock_exception(mocker: MockerFixture) -> None: + """ + must return False on OSError + """ + mocker.patch("fcntl.flock", side_effect=OSError) + assert not Lock.perform_lock(1) + + +def test_open(lock: Lock, mocker: MockerFixture) -> None: + """ + must open file + """ + open_mock = mocker.patch("pathlib.Path.open") + lock.path = Path("ahriman.pid") + + lock._open() + open_mock.assert_called_once_with("a+") + + +def test_open_skip(lock: Lock, mocker: MockerFixture) -> None: + """ + must skip file opening if path is not set + """ + open_mock = mocker.patch("pathlib.Path.open") + lock._open() + open_mock.assert_not_called() + + +def test_watch(lock: Lock, mocker: MockerFixture) -> None: + """ + must check if lock file exists + """ + lock._pid_file = MagicMock() + lock._pid_file.fileno.return_value = 1 + wait_mock = mocker.patch("ahriman.models.waiter.Waiter.wait") + + lock._watch() + wait_mock.assert_called_once_with(lock.perform_lock, 1) + + +def test_watch_skip(lock: Lock, mocker: MockerFixture) -> None: + """ + must skip watch on empty path + """ + mocker.patch("ahriman.application.lock.Lock.perform_lock", return_value=True) + lock._watch() + + +def test_write(lock: Lock) -> None: + """ + must write PID to lock file + """ + with NamedTemporaryFile("a+") as pid_file: + lock._pid_file = pid_file + lock._write() + + assert int(lock._pid_file.readline()) == os.getpid() + + +def test_write_skip(lock: Lock) -> None: + """ + must skip write to file if no path set + """ + lock._write() + + +def test_write_locked(lock: Lock, mocker: MockerFixture) -> None: + """ + must raise DuplicateRunError if cannot lock file + """ + mocker.patch("ahriman.application.lock.Lock.perform_lock", return_value=False) + with pytest.raises(DuplicateRunError): + lock._pid_file = MagicMock() + lock._write() + + def test_check_user(lock: Lock, mocker: MockerFixture) -> None: """ must check user correctly @@ -88,7 +178,7 @@ def test_clear(lock: Lock) -> None: """ must remove lock file """ - lock.path = Path(tempfile.gettempdir()) / "ahriman-test.lock" + lock.path = Path("ahriman-test.pid") lock.path.touch() lock.clear() @@ -99,7 +189,7 @@ def test_clear_missing(lock: Lock) -> None: """ must not fail on lock removal if file is missing """ - lock.path = Path(tempfile.gettempdir()) / "ahriman-test.lock" + lock.path = Path("ahriman-test.pid") lock.clear() @@ -112,67 +202,52 @@ def test_clear_skip(lock: Lock, mocker: MockerFixture) -> None: unlink_mock.assert_not_called() -def test_create(lock: Lock) -> None: +def test_clear_close(lock: Lock) -> None: """ - must create lock + must close pid file if opened """ - lock.path = Path(tempfile.gettempdir()) / "ahriman-test.lock" - - lock.create() - assert lock.path.is_file() - lock.path.unlink() + close_mock = lock._pid_file = MagicMock() + lock.clear() + close_mock.close.assert_called_once_with() -def test_create_exception(lock: Lock) -> None: +def test_clear_close_exception(lock: Lock) -> None: """ - must raise exception if file already exists + must suppress IO exception on file closure """ - lock.path = Path(tempfile.gettempdir()) / "ahriman-test.lock" - lock.path.touch() - - with pytest.raises(DuplicateRunError): - lock.create() - lock.path.unlink() + close_mock = lock._pid_file = MagicMock() + close_mock.close.side_effect = IOError() + lock.clear() -def test_create_skip(lock: Lock, mocker: MockerFixture) -> None: +def test_lock(lock: Lock, mocker: MockerFixture) -> None: """ - must skip creating if no file set + must perform lock correctly """ - touch_mock = mocker.patch("pathlib.Path.touch") - lock.create() - touch_mock.assert_not_called() + clear_mock = mocker.patch("ahriman.application.lock.Lock.clear") + open_mock = mocker.patch("ahriman.application.lock.Lock._open") + watch_mock = mocker.patch("ahriman.application.lock.Lock._watch") + write_mock = mocker.patch("ahriman.application.lock.Lock._write") + + lock.lock() + clear_mock.assert_not_called() + open_mock.assert_called_once_with() + watch_mock.assert_called_once_with() + write_mock.assert_called_once_with() -def test_create_unsafe(lock: Lock) -> None: +def test_lock_clear(lock: Lock, mocker: MockerFixture) -> None: """ - must not raise exception if force flag set + must clear lock file before lock if force flag is set """ + mocker.patch("ahriman.application.lock.Lock._open") + mocker.patch("ahriman.application.lock.Lock._watch") + mocker.patch("ahriman.application.lock.Lock._write") + clear_mock = mocker.patch("ahriman.application.lock.Lock.clear") lock.force = True - lock.path = Path(tempfile.gettempdir()) / "ahriman-test.lock" - lock.path.touch() - lock.create() - lock.path.unlink() - - -def test_watch(lock: Lock, mocker: MockerFixture) -> None: - """ - must check if lock file exists - """ - wait_mock = mocker.patch("ahriman.models.waiter.Waiter.wait") - lock.path = Path(tempfile.gettempdir()) / "ahriman-test.lock" - - lock.watch() - wait_mock.assert_called_once_with(lock.path.is_file) - - -def test_watch_skip(lock: Lock, mocker: MockerFixture) -> None: - """ - must skip watch on empty path - """ - mocker.patch("pathlib.Path.is_file", return_value=True) - lock.watch() + lock.lock() + clear_mock.assert_called_once_with() def test_enter(lock: Lock, mocker: MockerFixture) -> None: @@ -181,18 +256,14 @@ def test_enter(lock: Lock, mocker: MockerFixture) -> None: """ check_user_mock = mocker.patch("ahriman.application.lock.Lock.check_user") check_version_mock = mocker.patch("ahriman.application.lock.Lock.check_version") - watch_mock = mocker.patch("ahriman.application.lock.Lock.watch") - clear_mock = mocker.patch("ahriman.application.lock.Lock.clear") - create_mock = mocker.patch("ahriman.application.lock.Lock.create") + lock_mock = mocker.patch("ahriman.application.lock.Lock.lock") update_status_mock = mocker.patch("ahriman.core.status.Client.status_update") with lock: pass check_user_mock.assert_called_once_with() - clear_mock.assert_called_once_with() - create_mock.assert_called_once_with() check_version_mock.assert_called_once_with() - watch_mock.assert_called_once_with() + lock_mock.assert_called_once_with() update_status_mock.assert_has_calls([MockCall(BuildStatusEnum.Building), MockCall(BuildStatusEnum.Success)]) @@ -202,7 +273,7 @@ def test_exit_with_exception(lock: Lock, mocker: MockerFixture) -> None: """ mocker.patch("ahriman.application.lock.Lock.check_user") mocker.patch("ahriman.application.lock.Lock.clear") - mocker.patch("ahriman.application.lock.Lock.create") + mocker.patch("ahriman.application.lock.Lock.lock") update_status_mock = mocker.patch("ahriman.core.status.Client.status_update") with pytest.raises(ValueError):