arcanis/ahriman
1
0
Fork 0
mirror of https://github.com/arcan1s/ahriman.git synced 2025-04-24 07:17:17 +00:00
Code Issues Projects Releases Wiki Activity

improve lock mechanisms

Browse Source
This commit is contained in:
Evgenii Alekseev 2024-06-17 12:00:49 +03:00
parent 985307a89e
commit b43df8e094
5 changed files with 225 additions and 102 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
package/archlinux/ahriman.tmpfiles
View File

@ -1 +1,2 @@
d /var/lib/ahriman 0755 ahriman ahriman
d /var/lib/ahriman 0755 ahriman ahriman
d /run/ahriman 0755 ahriman ahriman

13
src/ahriman/application/ahriman.py
View File

@ -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

121
src/ahriman/application/lock.py
View File

@ -18,7 +18,10 @@
# along with this program. If not, see <http://www.gnu.org/licenses/>.
#
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

3
tests/ahriman/application/test_ahriman.py
View File

@ -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

187
tests/ahriman/application/test_lock.py
View File

@ -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):