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
Open-Wine-Components · Dec 29, 2024 · c30db72 · c30db72
1 parent 9849c74
commit c30db72
Show file tree

Hide file tree

Showing 14 changed files with 115 additions and 38 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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."""