WIP another windows test

Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>

Author: jku

tests/test_updater_ng.py

@@ -362,8 +362,6 @@ def test_parallel_updaters(self) -> None:
         # Refresh many updaters in parallel many times, using the same local metadata cache.
         # This should reveal race conditions.
 
-        # windows on GitHub actions *will* fail with "[Errno 36] Resource deadlock avoided"
-        # if iterations is in the hundreds
         iterations = 50
         process_count = 10
 
@@ -396,7 +394,6 @@ def test_parallel_updaters(self) -> None:
                 errout += "Parallel Refresh script failed:"
                 errout += f"\nprocess stdout: \n{stdout.decode()}"
                 errout += f"\nprocess stderr: \n{stderr.decode()}"
-
         if errout:
             self.fail(
                 f"One or more scripts failed parallel refresh test:\n{errout}"

tox.ini

@@ -11,7 +11,7 @@ skipsdist = true
 [testenv]
 commands =
     python3 --version
-    python3 -m coverage run -m unittest
+    python3 -m coverage run -m unittest -v
     python3 -m coverage report -m --fail-under 96
 
 deps =

tuf/ngclient/updater.py

@@ -58,6 +58,7 @@
 import os
 import shutil
 import tempfile
+import time
 from pathlib import Path
 from typing import IO, TYPE_CHECKING, cast
 from urllib import parse
@@ -79,21 +80,56 @@
     # advisory file locking for posix
     import fcntl
 
-    def _lock_file(f: IO) -> None:
-        if f.writable():
+    @contextlib.contextmanager
+    def _lock_file(path: str) -> Iterator[IO]:
+        with open(path, "wb") as f:
             fcntl.lockf(f, fcntl.LOCK_EX)
+            yield f
 
 except ModuleNotFoundError:
-    # Windows file locking
+    # Windows file locking, in belt-and-suspenders-from-Temu style:
+    # Use a loop that tries to open the lockfile for 30 secs, but also
+    # use msvcrt.locking(). The latter would be the OS lock but it's
+    # mostly useless:
+    # * since open() usually just fails when another process has the file open
+    #   msvcrt.locking() never gets called when there is a lock. open()
+    #   sometimes succeeds for
+    #   multiple processes but write or flush then fails
+    # * msvcrt.locking() does not even block until file is available: it just
+    #   tries once per second in a non-blocking manner for 10 seconds. So if
+    #   another process keeps opening the file it's unlikely that we actually
+    #   get the lock
     import msvcrt
 
-    def _lock_file(f: IO) -> None:
-        # On Windows we lock a byte range and file must not be empty
-        f.write(b"\0")
-        f.flush()
-        f.seek(0)
-
-        msvcrt.locking(f.fileno(), msvcrt.LK_LOCK, 1)
+    @contextlib.contextmanager
+    def _lock_file(path: str) -> Iterator[IO]:
+        err = None
+        locked = False
+        for _ in range(100):
+            try:
+                with open(path, "wb") as f:
+                    # file must not be empty
+                    f.write(b"\0")
+                    f.flush()
+                    f.seek(0)
+                    msvcrt.locking(f.fileno(), msvcrt.LK_LOCK, 1)
+                    locked = True
+                    yield f
+                    return
+            except FileNotFoundError:
+                # could be from yield or from open() -- either way we bail
+                raise
+            except OSError as e:
+                if locked:
+                    # yield has raised, let's not continue loop
+                    raise e
+                err = e
+                logger.warning("Unsuccessful lock attempt for %s: %s", path, e)
+                time.sleep(0.3)
+
+        # raise the last failure if we never got a lock
+        if err is not None:
+            raise err
 
 
 class Updater:
@@ -153,6 +189,10 @@ def __init__(
                 f"got '{self.config.envelope_type}'"
             )
 
+        # Ensure the whole metadata directory structure exists
+        rootdir = Path(self._dir, "root_history")
+        rootdir.mkdir(exist_ok=True, parents=True)
+
         with self._lock_metadata():
             if not bootstrap:
                 # if no root was provided, use the cached non-versioned root
@@ -168,12 +208,9 @@ def __init__(
     @contextlib.contextmanager
     def _lock_metadata(self) -> Iterator[None]:
         """Context manager for locking the metadata directory."""
-        # Ensure the whole metadata directory structure exists
-        rootdir = Path(self._dir, "root_history")
-        rootdir.mkdir(exist_ok=True, parents=True)
+
         logger.debug("Getting metadata lock...")
-        with open(os.path.join(self._dir, ".lock"), "wb") as f:
-            _lock_file(f)
+        with _lock_file(os.path.join(self._dir, ".lock")):
             yield
         logger.debug("Released metadata lock")
 
@@ -336,8 +373,7 @@ def download_target(
             targetinfo.verify_length_and_hashes(target_file)
 
             target_file.seek(0)
-            with open(filepath, "wb") as destination_file:
-                _lock_file(destination_file)
+            with _lock_file(filepath) as destination_file:
                 shutil.copyfileobj(target_file, destination_file)
 
         logger.debug("Downloaded target %s", targetinfo.path)