Merge pull request #57 from stb-tester/snapshot-lock-timeout

Linux/MacOS: Time out file locking after 10s

.github/workflows/lint.yml

@@ -11,7 +11,7 @@ jobs:
     strategy:
       matrix:
         os: [ubuntu-20.04]
-        python-version: ["2.7", "3.6"]
+        python-version: ["3.6"]
 
     runs-on: ${{ matrix.os }}
     steps:

.github/workflows/pytest.yml

@@ -14,7 +14,7 @@ jobs:
     strategy:
       matrix:
         environment: [ubuntu-20.04, macos-latest, windows-latest]
-        python-version: ["2.7", "3.5", "3.6", "3.7", "3.8", "3.10", "3.11"]
+        python-version: ["3.5", "3.6", "3.7", "3.8", "3.10", "3.11"]
         include:
           - environment: ubuntu-20.04
             setup: dbus-run-session sh -c 'env $(echo password | gnome-keyring-daemon --unlock) "$0" "$@"'

.pylintrc

@@ -8,6 +8,7 @@ reports=no
 
 [MESSAGES CONTROL]
 disable=
+  consider-using-f-string,
   import-outside-toplevel,
   invalid-name,
   locally-disabled,

stbt_rig.py

@@ -1658,7 +1658,8 @@ def push_git_snapshot(self, branch, interactive=True):
         # otherwise github can error with "rejected", so use file locking:
         lockfilename = self._git(
             ["rev-parse", "--git-path", "stbt-rig-snapshot.lock"]).strip()
-        with open(lockfilename, "w") as lock, file_lock(lock.fileno()):
+
+        with flock(lockfilename):
             commit_sha, run_sha = self.take_snapshot()
             options = ['--force']
             if not logger.isEnabledFor(logging.DEBUG):
@@ -1673,20 +1674,69 @@ def push_git_snapshot(self, branch, interactive=True):
             return run_sha
 
 
+@contextmanager
+def flock(filename):
+    with open(filename, "a+b") as f:
+        f.seek(0)
+        failed_attempting_lock = True
+        try:
+            with file_lock(f.fileno()):
+                failed_attempting_lock = False
+                bytes_written = f.write(b"%20d" % os.getpid())
+                f.flush()
+                f.truncate(bytes_written)
+                yield
+        except (OSError, IOError) as e:
+            if failed_attempting_lock:
+                msg = "Failed to lock %r: %s" % (filename, e)
+                if platform.system() != "Windows":
+                    try:
+                        pid = to_native_str(f.read(20).strip())
+                        msg += ".  File is currently locked by pid %s" % pid
+                    except Exception:  # pylint:disable=broad-except
+                        logger.warning(
+                            "Failed to read pid from %r", filename,
+                            exc_info=True)
+                die(msg)
+            else:
+                raise
+
+
 if platform.system() == "Windows":
     @contextmanager
     def file_lock(fileno):
+        # Arbitrarily chosen position in the file, sufficiently far from the
+        # beginning that it won't overlap with the pid:
+        LOCK_POS = 20
+
         import msvcrt  # pylint: disable=import-error
+        # Seek to specific position and lock 1 byte.  We don't want to lock the
+        # beginning of the file because that would prevent other processes from
+        # reading the pid from it:
+        pos = os.lseek(fileno, LOCK_POS, os.SEEK_SET)
         msvcrt.locking(fileno, msvcrt.LK_LOCK, 1)
+        os.lseek(fileno, pos, os.SEEK_SET)
         try:
             yield
         finally:
+            pos = os.lseek(fileno, LOCK_POS, os.SEEK_SET)
             msvcrt.locking(fileno, msvcrt.LK_UNLCK, 1)
+            os.lseek(fileno, pos, os.SEEK_SET)
 else:
     @contextmanager
     def file_lock(fileno):
         import fcntl
-        fcntl.flock(fileno, fcntl.LOCK_EX)
+        # Makes locking on UNIX match the behaviour on Windows where we give up
+        # on locking after 10s:
+        for n in range(10):
+            try:
+                fcntl.flock(fileno, fcntl.LOCK_EX | fcntl.LOCK_NB)
+                break
+            except IOError as e:
+                if e.errno in [errno.EAGAIN, errno.EWOULDBLOCK] and n < 9:
+                    time.sleep(1)
+                else:
+                    raise
         try:
             yield
         finally:

test_stbt_rig.py

@@ -341,6 +341,29 @@ def proc():
             t.join()
 
 
+def test_file_lock_slow(tmpdir):
+    env = os.environ.copy()
+    env["PYTHONPATH"] = _find_file('.')
+
+    cmd = dedent("""\
+        import time
+        import stbt_rig
+        with stbt_rig.flock("lockfile"):
+            time.sleep(20)
+        """)
+    p = subprocess.Popen([python, "-c", cmd], cwd=str(tmpdir), env=env)
+    time.sleep(0.5)
+    p2 = subprocess.Popen(
+        [python, "-c", cmd], stderr=subprocess.PIPE, cwd=str(tmpdir), env=env)
+    assert p2.wait(15) == 1
+
+    errmsg = p2.stderr.read().decode().strip()
+    assert "Failed to lock" in errmsg
+    if platform.system() != "Windows":
+        assert errmsg.endswith("File is currently locked by pid %i" % p.pid)
+    p.kill()
+
+
 def test_modify_config():
     def test(cfg):
         if sys.version_info[0] == 2: