From c30db728a3f6f963f1c66bee499c85b6bce283c3 Mon Sep 17 00:00:00 2001 From: R1kaB3rN <100738684+R1kaB3rN@users.noreply.github.com> Date: Sun, 29 Dec 2024 21:26:49 +0000 Subject: [PATCH] refactor: use custom file lock functionality (#314) * umu_util: add function to enable file locking * umu_proton: refactor file locking * umu_runtime: refactor file locking * umu_run: refactor file locking * pyproject: drop filelock dependency * requirements: drop filelock dependency * packaging: drop filelock dependency * umu_test: update tests to mock the context object * umu_runtime: don't create var in with statement * tests: add e2e test for file lock * workflows: add e2e file lock test * tests: shellcheck lint * tests: fix e2e test * tests: update file lock test * tests: update file lock test * tests: update file lock test * tests: update file lock test * tests: update file lock test * style: update name * chore: remove comment --- .github/workflows/e2e.yml | 5 +++++ packaging/deb/debian/control | 1 - packaging/deb/ubuntu/control | 1 - packaging/nix/umu-launcher.nix | 1 - packaging/rpm/umu-launcher.spec | 1 - packaging/snap/snap/snapcraft.yaml | 1 - pyproject.toml | 2 +- requirements.in | 1 - tests/test_flock.sh | 31 ++++++++++++++++++++++++++ umu/umu_proton.py | 35 +++++++++++++++--------------- umu/umu_run.py | 4 ++-- umu/umu_runtime.py | 23 ++++++++++---------- umu/umu_test.py | 30 +++++++++++++++++++++++++ umu/umu_util.py | 17 +++++++++++++++ 14 files changed, 115 insertions(+), 38 deletions(-) create mode 100644 tests/test_flock.sh diff --git a/.github/workflows/e2e.yml b/.github/workflows/e2e.yml index 6ee9bcc72..3cb74eafe 100644 --- a/.github/workflows/e2e.yml +++ b/.github/workflows/e2e.yml @@ -55,6 +55,11 @@ jobs: source .venv/bin/activate sh tests/test_proton_resume.sh rm -rf "$HOME/.local/share/umu" "$HOME/Games/umu" "$HOME/.local/share/Steam/compatibilitytools.d" "$HOME/.cache/umu" + - name: Test Filelock + run: | + source .venv/bin/activate + sh tests/test_flock.sh + rm -rf "$HOME/.local/share/umu" "$HOME/Games/umu" "$HOME/.local/share/Steam/compatibilitytools.d" "$HOME/.cache/umu" - name: Test winetricks run: | source .venv/bin/activate diff --git a/packaging/deb/debian/control b/packaging/deb/debian/control index ce02b0b79..98ab7a4a3 100644 --- a/packaging/deb/debian/control +++ b/packaging/deb/debian/control @@ -36,7 +36,6 @@ Depends: ${misc:Depends}, python3, python3-xlib (>= 0.33), - python3-filelock (>= 3.9.0), apparmor-profiles, libgl1-mesa-dri:i386, libglx-mesa0:i386 diff --git a/packaging/deb/ubuntu/control b/packaging/deb/ubuntu/control index ce02b0b79..98ab7a4a3 100644 --- a/packaging/deb/ubuntu/control +++ b/packaging/deb/ubuntu/control @@ -36,7 +36,6 @@ Depends: ${misc:Depends}, python3, python3-xlib (>= 0.33), - python3-filelock (>= 3.9.0), apparmor-profiles, libgl1-mesa-dri:i386, libglx-mesa0:i386 diff --git a/packaging/nix/umu-launcher.nix b/packaging/nix/umu-launcher.nix index 24bad46ad..da00e7878 100644 --- a/packaging/nix/umu-launcher.nix +++ b/packaging/nix/umu-launcher.nix @@ -18,7 +18,6 @@ python3Packages.buildPythonPackage { pyth1 pkgs.bubblewrap pkgs.python3Packages.xlib - pkgs.python3Packages.filelock pkgs.python3Packages.urllib3 ] ++ lib.optional truststore pkgs.python3Packages.truststore; makeFlags = [ "PYTHON_INTERPRETER=${pyth1}/bin/python" "SHELL_INTERPRETER=/run/current-system/sw/bin/bash" "DESTDIR=${placeholder "out"}" ]; diff --git a/packaging/rpm/umu-launcher.spec b/packaging/rpm/umu-launcher.spec index fd9943ee7..f9879c1b2 100644 --- a/packaging/rpm/umu-launcher.spec +++ b/packaging/rpm/umu-launcher.spec @@ -39,7 +39,6 @@ BuildRequires: python3 Requires: python Requires: python3 Requires: python3-xlib -Requires: python3-filelock %description diff --git a/packaging/snap/snap/snapcraft.yaml b/packaging/snap/snap/snapcraft.yaml index f21911682..1bafd0b8c 100644 --- a/packaging/snap/snap/snapcraft.yaml +++ b/packaging/snap/snap/snapcraft.yaml @@ -246,7 +246,6 @@ parts: plugin: nil stage-packages: - python3-xlib - - python3-filelock prime: - usr/lib/python3 diff --git a/pyproject.toml b/pyproject.toml index 0b1060a26..613632e44 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -35,7 +35,7 @@ classifiers = [ urls = { repository = "https://github.com/Open-Wine-Components/umu-launcher" } # Note: urllib3 is a vendored dependency. When using our Makefile, it will be # installed automatically. -dependencies = ["python-xlib>=0.33", "filelock>=3.9.0", "urllib3>=2.0.0,<3.0.0"] +dependencies = ["python-xlib>=0.33", "urllib3>=2.0.0,<3.0.0"] [project.optional-dependencies] # Recommended diff --git a/requirements.in b/requirements.in index 49ce05aec..edf15f17a 100644 --- a/requirements.in +++ b/requirements.in @@ -1,3 +1,2 @@ python-xlib>=0.33 -filelock>=3.15.4 urllib3>=2.0.0,<3.0.0 diff --git a/tests/test_flock.sh b/tests/test_flock.sh new file mode 100644 index 000000000..5153ac9cb --- /dev/null +++ b/tests/test_flock.sh @@ -0,0 +1,31 @@ +#!/usr/bin/env sh + +# +# Ensure umu-launcher does not download its fetched resources more than once +# when multiple processes of itself are created +# + +tmp1=$(mktemp) +tmp2=$(mktemp) + +UMU_LOG=debug GAMEID=umu-0 "$HOME/.local/bin/umu-run" wineboot -u 2> "$tmp1" & +sleep 1 +UMU_LOG=debug GAMEID=umu-0 "$HOME/.local/bin/umu-run" wineboot -u 2> "$tmp2" & +wait + +grep "exited with wait status" "$tmp1" && grep -E "exited with wait status" "$tmp2" + +# Ensure the 2nd proc didn't download the runtime +grep -E "\(latest\), please wait..." "$tmp2" +exit_code=$? +if "$exit_code" -ne 0; then + exit 1 +fi + +# Ensure the 2nd proc didn't download Proton +grep "Downloading" "$tmp2" +exit_code=$? +if "$exit_code" -ne 0; then + exit 1 +fi + diff --git a/umu/umu_proton.py b/umu/umu_proton.py index a4107549f..d604d666c 100644 --- a/umu/umu_proton.py +++ b/umu/umu_proton.py @@ -8,7 +8,6 @@ from tempfile import TemporaryDirectory from typing import Any -from filelock import FileLock from urllib3.exceptions import HTTPError from urllib3.exceptions import TimeoutError as TimeoutErrorUrllib3 from urllib3.poolmanager import PoolManager @@ -20,6 +19,7 @@ extract_tarfile, file_digest, run_zenity, + unix_flock, write_file_chunks, ) @@ -313,7 +313,7 @@ def _get_latest( proton: str # Name of the Proton version, which is either UMU-Proton or GE-Proton version: str - lock: FileLock + lockfile: str = f"{UMU_LOCAL}/compatibilitytools.d.lock" if not assets: return None @@ -335,19 +335,19 @@ def _get_latest( # Use the latest UMU/GE-Proton try: - lock = FileLock(f"{UMU_LOCAL}/compatibilitytools.d.lock") - log.debug("Acquiring file lock '%s'...", lock.lock_file) - lock.acquire() - - # Once acquiring the lock check if Proton hasn't been installed - if steam_compat.joinpath(proton).is_dir(): - raise FileExistsError - - # Download the archive to a temporary directory - _fetch_proton(env, session_caches, assets, session_pools) - - # Extract the archive then move the directory - _install_proton(tarball, session_caches, steam_compat, session_pools) + log.debug("Acquiring file lock '%s'...", lockfile) + with unix_flock(lockfile): + # Once acquiring the lock check if Proton hasn't been installed + if steam_compat.joinpath(proton).is_dir(): + raise FileExistsError + + # Download the archive to a temporary directory + _fetch_proton(env, session_caches, assets, session_pools) + + # Extract the archive then move the directory + _install_proton( + tarball, session_caches, steam_compat, session_pools + ) except ( ValueError, KeyboardInterrupt, @@ -356,11 +356,10 @@ def _get_latest( log.exception(e) return None except FileExistsError: + # Proton was installed by another proc, continue pass - finally: - log.debug("Released file lock '%s'", lock.lock_file) - lock.release() + log.debug("Released file lock '%s'", lockfile) os.environ["PROTONPATH"] = str(steam_compat.joinpath(proton)) env["PROTONPATH"] = os.environ["PROTONPATH"] log.debug("Removing: %s", tarball) diff --git a/umu/umu_run.py b/umu/umu_run.py index 700a6d9b4..4a4244a29 100755 --- a/umu/umu_run.py +++ b/umu/umu_run.py @@ -25,7 +25,6 @@ from subprocess import Popen from typing import Any -from filelock import FileLock from urllib3 import PoolManager, Retry from urllib3.exceptions import MaxRetryError, NewConnectionError from urllib3.exceptions import TimeoutError as TimeoutErrorUrllib3 @@ -54,6 +53,7 @@ get_library_paths, has_umu_setup, is_installed_verb, + unix_flock, xdisplay, ) @@ -830,7 +830,7 @@ def umu_run(args: Namespace | tuple[str, list[str]]) -> int: UMU_LOCAL.mkdir(parents=True, exist_ok=True) # Prepare the prefix - with FileLock(f"{UMU_LOCAL}/pfx.lock"): + with unix_flock(f"{UMU_LOCAL}/pfx.lock"): setup_pfx(env["WINEPREFIX"]) # Configure the environment diff --git a/umu/umu_runtime.py b/umu/umu_runtime.py index 1e3bbd38e..6e432fb67 100644 --- a/umu/umu_runtime.py +++ b/umu/umu_runtime.py @@ -15,7 +15,6 @@ from subprocess import run from tempfile import TemporaryDirectory, mkdtemp -from filelock import FileLock from urllib3.exceptions import HTTPError from urllib3.exceptions import TimeoutError as TimeoutErrorUrllib3 from urllib3.poolmanager import PoolManager @@ -28,6 +27,7 @@ file_digest, has_umu_setup, run_zenity, + unix_flock, write_file_chunks, ) @@ -476,14 +476,15 @@ def _restore_umu( session_pools: SessionPools, callback_fn: Callable[[], bool], ) -> None: - with FileLock(f"{local}/umu.lock") as lock: - log.debug("Acquired file lock '%s'...", lock.lock_file) + lockfile: str = f"{local}/umu.lock" + with unix_flock(lockfile): + log.debug("Acquired file lock '%s'...", lockfile) if callback_fn(): - log.debug("Released file lock '%s'", lock.lock_file) + log.debug("Released file lock '%s'", lockfile) log.info("%s was restored", runtime_ver[1]) return _install_umu(runtime_ver, session_pools) - log.debug("Released file lock '%s'", lock.lock_file) + log.debug("Released file lock '%s'", lockfile) def _restore_umu_platformid( @@ -547,20 +548,20 @@ def _update_umu_platform( latest: bytes = sha256(resp.data).digest() current: bytes = sha256(local.joinpath("VERSIONS.txt").read_bytes()).digest() versions: Path = local.joinpath("VERSIONS.txt") - lock: FileLock = FileLock(f"{local}/umu.lock") + lockfile: str = f"{local}/umu.lock" # Compare our version file to upstream's, updating if different if latest != current: log.info("Updating %s to latest...", variant) - log.debug("Acquiring file lock '%s'...", lock.lock_file) - with lock: - log.debug("Acquired file lock '%s'", lock.lock_file) + log.debug("Acquiring file lock '%s'...", lockfile) + with unix_flock(lockfile): + log.debug("Acquired file lock '%s'", lockfile) # Once another process acquires the lock, check if the latest # runtime has already been downloaded if latest == sha256(versions.read_bytes()).digest(): - log.debug("Released file lock '%s'", lock.lock_file) + log.debug("Released file lock '%s'", lockfile) return _install_umu(runtime_ver, session_pools) log.debug("Removing: %s", runtime) rmtree(str(runtime)) - log.debug("Released file lock '%s'", lock.lock_file) + log.debug("Released file lock '%s'", lockfile) diff --git a/umu/umu_test.py b/umu/umu_test.py index f87d53552..e1a5aa515 100644 --- a/umu/umu_test.py +++ b/umu/umu_test.py @@ -1131,9 +1131,15 @@ def test_latest_interrupt(self): files = (("", ""), (self.test_archive.name, "")) tmpdirs = (self.test_cache, self.test_cache_home) + # Mock the context manager object that creates the file lock + mock_ctx = MagicMock() + mock_ctx.__enter__ = MagicMock(return_value=None) + mock_ctx.__exit__ = MagicMock(return_value=None) + with ( patch("umu.umu_proton._fetch_proton") as mock_function, ThreadPoolExecutor(), + patch.object(umu_proton, "unix_flock", return_value=mock_ctx), ): # Mock the interrupt # We want the dir we tried to extract to be cleaned @@ -1164,6 +1170,11 @@ def test_latest_val_err(self): files = (("", ""), (self.test_archive.name, "")) tmpdirs = (self.test_cache, self.test_cache_home) + # Mock the context manager object that creates the file lock + mock_ctx = MagicMock() + mock_ctx.__enter__ = MagicMock(return_value=None) + mock_ctx.__exit__ = MagicMock(return_value=None) + self.assertTrue( self.test_archive.is_file(), "Expected test file in cache to exist", @@ -1172,6 +1183,7 @@ def test_latest_val_err(self): with ( patch("umu.umu_proton._fetch_proton") as mock_function, ThreadPoolExecutor(), + patch.object(umu_proton, "unix_flock", return_value=mock_ctx), ): # Mock the interrupt mock_function.side_effect = ValueError @@ -1195,11 +1207,17 @@ def test_latest_offline(self): files = () tmpdirs = (self.test_cache, self.test_cache_home) + # Mock the context manager object that creates the file lock + mock_ctx = MagicMock() + mock_ctx.__enter__ = MagicMock(return_value=None) + mock_ctx.__exit__ = MagicMock(return_value=None) + os.environ["PROTONPATH"] = "" with ( patch("umu.umu_proton._fetch_proton"), ThreadPoolExecutor(), + patch.object(umu_proton, "unix_flock", return_value=mock_ctx), ): result = umu_proton._get_latest( self.env, @@ -1228,6 +1246,11 @@ def test_link_umu(self): files = ((f"{latest}.sha512sum", ""), (f"{latest}.tar.gz", "")) tmpdirs = (self.test_cache, self.test_cache_home) + # Mock the context manager object that creates the file lock + mock_ctx = MagicMock() + mock_ctx.__enter__ = MagicMock(return_value=None) + mock_ctx.__exit__ = MagicMock(return_value=None) + # Mock the latest Proton in /tmp test_archive = self.test_cache.joinpath(f"{latest}.tar.gz") with tarfile.open(test_archive.as_posix(), "w:gz") as tar: @@ -1245,6 +1268,7 @@ def test_link_umu(self): with ( patch("umu.umu_proton._fetch_proton"), ThreadPoolExecutor(), + patch.object(umu_proton, "unix_flock", return_value=mock_ctx), ): result = umu_proton._get_latest( self.env, @@ -1287,6 +1311,11 @@ def test_latest_umu(self): files = ((f"{latest}.sha512sum", ""), (f"{latest}.tar.gz", "")) tmpdirs = (self.test_cache, self.test_cache_home) + # Mock the context manager object that creates the file lock + mock_ctx = MagicMock() + mock_ctx.__enter__ = MagicMock(return_value=None) + mock_ctx.__exit__ = MagicMock(return_value=None) + # Mock the latest Proton in /tmp test_archive = self.test_cache.joinpath(f"{latest}.tar.gz") with tarfile.open(test_archive.as_posix(), "w:gz") as tar: @@ -1310,6 +1339,7 @@ def test_latest_umu(self): with ( patch("umu.umu_proton._fetch_proton"), ThreadPoolExecutor() as thread_pool, + patch.object(umu_proton, "unix_flock", return_value=mock_ctx), ): result = umu_proton._get_latest( self.env, diff --git a/umu/umu_util.py b/umu/umu_util.py index 0f94c1575..35b22c017 100644 --- a/umu/umu_util.py +++ b/umu/umu_util.py @@ -2,6 +2,7 @@ import sys from contextlib import contextmanager from ctypes.util import find_library +from fcntl import LOCK_EX, LOCK_UN, flock from functools import lru_cache from hashlib import new as hashnew from io import BufferedIOBase @@ -19,6 +20,22 @@ from umu.umu_log import log +@contextmanager +def unix_flock(path: str): + """Create a file and configure it to be locking.""" + fd: int | None = None + + try: + fd = os.open(path, os.O_CREAT | os.O_WRONLY, 0o644) + # See https://man7.org/linux/man-pages/man2/flock.2.html + flock(fd, LOCK_EX) + yield fd + finally: + if fd is not None: + flock(fd, LOCK_UN) + os.close(fd) + + @lru_cache def get_libc() -> str: """Find libc.so from the user's system."""