Merge pull request #4790 from mwichmann/test/filelock

Add some basic unit tests for file locking

Author: bdbaddog

CHANGES.txt

@@ -13,11 +13,13 @@ NOTE: Since SCons 4.9.0, Python 3.7.0 or above is required.
 RELEASE  VERSION/DATE TO BE FILLED IN LATER
 
   From John Doe:
-
-        - Whatever John Doe did.
+    - Whatever John Doe did.
 
   From William Deegan:
-        - Fix SCons Docbook schema to work with lxml > 5
+    - Fix SCons Docbook schema to work with lxml > 5
+
+  From Mats Wichmann:
+    - Introduce some unit tests for the file locking utility routines
 
 
 RELEASE 4.10.1 - Sun, 16 Nov 2025 10:51:57 -0700

RELEASE.txt

@@ -61,6 +61,8 @@ DEVELOPMENT
 
 - List visible changes in the way SCons is developed
 
+- Introduce some unit tests for the file locking utility routines
+
 Thanks to the following contributors listed below for their contributions to this release.
 ==========================================================================================
 .. code-block:: text

SCons/Util/filelock.py

@@ -8,6 +8,12 @@
 which acquires a lock (or at least, permission) on entry and
 releases it on exit.
 
+The model is to prevent concurrency problems if there are multiple
+SCons processes running which may try to read/write the same
+externally shared resource, such as a cache file. They don't
+have to be the same machine in the case of a network share,
+so we can't use OS-specific locking schemes like fcntl/msvcrt.
+
 Usage::
 
     from SCons.Util.filelock import FileLock
@@ -19,13 +25,16 @@
 
 # TODO: things to consider.
 #   Is raising an exception the right thing for failing to get lock?
-#   Is a filesystem lockfile scheme sufficient for our needs?
-#   - or is it better to put locks on the actual file (fcntl/windows-based)?
-#   ... Is that even viable in the case of a remote (network) file?
+#   Is this scheme sufficient for our needs? Or do we need the extra
+#     level of creating a "claim file" first and then trying to link
+#     that to the lockfile? Bonus: a claim file can contain details, like
+#     how long before another process is allowed to break the lock,
+#     plus identifying the creator.
 #   Is this safe enough? Or do we risk dangling lockfiles?
 #   Permission issues in case of multi-user. This *should* be okay,
 #     the cache usually goes in user's homedir, plus you already have
-#     enough rights for the lockfile if the dir lets you create the cache.
+#     enough rights for the lockfile if the dir lets you create the cache,
+#     but manual permission fiddling may be needed for network shares.
 #   Need a forced break-lock method?
 #   The lock attributes could probably be made opaque. Showed one visible
 #     in the example above, but not sure the benefit of that.
@@ -37,36 +46,46 @@
 
 
 class SConsLockFailure(Exception):
-    """Lock failure exception."""
+    """Generic lock failure exception."""
+
+class AlreadyLockedError(SConsLockFailure):
+    """Non-blocking lock attempt on already locked object."""
+
+class LockTimeoutError(SConsLockFailure):
+    """Blocking lock attempt timed out."""
 
 
 class FileLock:
-    """Lock a file using a lockfile.
+    """Lock a resource using a lockfile.
 
-    Basic locking for when multiple processes may hit an externally
-    shared resource that cannot depend on locking within a single SCons
-    process. SCons does not have a lot of those, but caches come to mind.
+    Basic locking for when multiple processes may hit an externally shared
+    resource (i.e. a file) that cannot depend on locking within a single
+    SCons process. SCons does not have a lot of those, but caches come to
+    mind.  Note that the resource named by *file* does not need to exist.
 
-    Cross-platform safe, does not use any OS-specific features.  Provides
+    Cross-platform safe, does not use any OS-specific features.  Is not
+    safe from outside interference - a "locked" resource can be messed
+    with by anyone who doesn't pay attention to this protocol.  Provides
     context manager support, or can be called with :meth:`acquire_lock`
     and :meth:`release_lock`.
 
     Lock can be a write lock, which is held until released, or a read
-    lock, which releases immediately upon aquisition - we want to not
-    read a file which somebody else may be writing, but not create the
-    writers starvation problem of the classic readers/writers lock.
+    lock, which releases immediately upon acquisition - we want to prevent
+    reading a file while somebody else may be writing to it, but once we
+    get that permission, we don't want to block others from reading it
+    too, or asking for a write lock on it.
 
     TODO: Should default timeout be None (non-blocking), or 0 (block forever),
-       or some arbitrary number?
+       or some arbitrary number? For now it's non-blocking.
 
     Arguments:
        file: name of file to lock. Only used to build the lockfile name.
-       timeout: optional time (sec) to give up trying.
-          If ``None``, quit now if we failed to get the lock (non-blocking).
+       timeout: optional time (sec) after which to give up trying.
+          If ``None``, the lock attempt will be non-blocking (the default).
           If 0, block forever (well, a long time).
        delay: optional delay between tries [default 0.05s]
-       writer: if True, obtain the lock for safe writing. If False (default),
-          just wait till the lock is available, give it back right away.
+       writer: if true, obtain and hold the lock for safe writing.
+          If false (the default), release the lock right away.
 
     Raises:
         SConsLockFailure: if the operation "timed out", including the
@@ -97,8 +116,10 @@ def __init__(
         self.writer = writer
 
     def acquire_lock(self) -> None:
-        """Acquire the lock, if possible.
+        """Acquire the lock.
 
+        Blocks until the lock is acquired, unless the lock object
+        was initialized to not block.
         If the lock is in use, check again every *delay* seconds.
         Continue until lock acquired or *timeout* expires.
         """
@@ -108,12 +129,12 @@ def acquire_lock(self) -> None:
                 self.lock = os.open(self.lockfile, os.O_CREAT|os.O_EXCL|os.O_RDWR)
             except (FileExistsError, PermissionError) as exc:
                 if self.timeout is None:
-                    raise SConsLockFailure(
+                    raise AlreadyLockedError(
                         f"Could not acquire lock on {self.file!r}"
                     ) from exc
                 if (time.perf_counter() - start_time) > self.timeout:
-                    raise SConsLockFailure(
-                        f"Timeout waiting for lock on {self.file!r}."
+                    raise LockTimeoutError(
+                        f"Timeout waiting for lock on {self.file!r} for {self.timeout} seconds."
                     ) from exc
                 time.sleep(self.delay)
             else:
@@ -131,21 +152,30 @@ def release_lock(self) -> None:
 
     def __enter__(self) -> FileLock:
         """Context manager entry: acquire lock if not holding."""
-        if not self.lock:
+        if self.lock is None:
             self.acquire_lock()
         return self
 
     def __exit__(self, exc_type, exc_value, exc_tb) -> None:
         """Context manager exit: release lock if holding."""
-        if self.lock:
+        if self.lock is not None:
             self.release_lock()
 
     def __repr__(self) -> str:
         """Nicer display if someone repr's the lock class."""
         return (
-            f"{self.__class__.__name__}("
+            f"<{self.__class__.__name__}("
             f"file={self.file!r}, "
-            f"timeout={self.timeout!r}, "
-            f"delay={self.delay!r}, "
-            f"writer={self.writer!r})"
+            f"type={'writer, ' if self.writer else 'reader, '}"
+            f"lock={'unlocked' if self.lock is None else 'locked'}, "
+            f"timeout={self.timeout}, "
+            f"delay={self.delay}, "
+            f"pid={os.getpid()}) "
+            f"at 0x{id(self):x}>"
         )
+
+# Local Variables:
+# tab-width:4
+# indent-tabs-mode:nil
+# End:
+# vim: set expandtab tabstop=4 shiftwidth=4:

SCons/Util/filelockTests.py

@@ -0,0 +1,104 @@
+# MIT License
+#
+# Copyright The SCons Foundation
+#
+# Permission is hereby granted, free of charge, to any person obtaining
+# a copy of this software and associated documentation files (the
+# "Software"), to deal in the Software without restriction, including
+# without limitation the rights to use, copy, modify, merge, publish,
+# distribute, sublicense, and/or sell copies of the Software, and to
+# permit persons to whom the Software is furnished to do so, subject to
+# the following conditions:
+#
+# The above copyright notice and this permission notice shall be included
+# in all copies or substantial portions of the Software.
+#
+# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY
+# KIND, EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE
+# WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+# NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE
+# LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION
+# OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION
+# WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
+
+"""File locking tests.
+
+TODO: Unlocking an unlocked file ("works" - no exception raised)
+TODO: Test reader locks
+TODO: Create another process to better simulate lock contention
+"""
+
+from __future__ import annotations
+
+import os
+import unittest
+from contextlib import suppress
+
+import TestCmd
+
+from SCons.Util import filelock
+
+
+class TestFileLock(unittest.TestCase):
+    def setUp(self):
+        self.test = TestCmd.TestCmd(workdir='')
+        self.file = self.test.workpath('test.txt')
+        self.lockfile = self.test.workpath('test.txt.lock')
+        self.lock = None
+
+    def tearDown(self):
+        self.lock.release_lock()
+        # in case we made phony lock, clean that up too
+        with suppress(FileNotFoundError):
+            os.unlink(self.lockfile)
+
+    def _fakelock(self) -> int:
+        """Create a fake lockfile to simulate a busy lock."""
+        return os.open(self.lockfile, os.O_CREAT|os.O_EXCL|os.O_RDWR)
+
+    def test_context_manager_lock(self):
+        """Test acquiring a file lock, context manager."""
+        self.lock = filelock.FileLock(self.file, writer=True)
+        self.assertFalse(os.path.exists(self.lockfile))
+        with self.lock:
+            self.assertTrue(os.path.exists(self.lockfile))
+        self.assertFalse(os.path.exists(self.lockfile))
+
+    def test_acquire_release_lock(self):
+        """Test acquiring a file lock, method."""
+        self.lock = filelock.FileLock(self.file, writer=True)
+        self.lock.acquire_lock()
+        self.assertTrue(os.path.exists(self.lockfile))
+        self.lock.release_lock()
+        self.assertFalse(os.path.exists(self.lockfile))
+
+    # Implementation does not raise on release when not holding lock: skip
+    # def test_exception_raised_when_lock_not_held(self):
+    #     self.lock = filelock.FileLock(self.file, writer=True)
+    #     with self.assertRaises(filelock.SConsLockFailure):
+    #         self.lock.release_lock()
+
+    def test_nonblocking_lockfail(self):
+        """Test non-blocking acquiring a busy file lock."""
+        self.lock = filelock.FileLock(self.file, writer=True)
+        fd = self._fakelock()
+        with self.lock and self.assertRaises(filelock.AlreadyLockedError):
+            self.lock.acquire_lock()
+        os.close(fd)
+
+    def test_timeout_lockfail(self):
+        """Test blocking acquiring a busy file lock with timeout."""
+        self.lock = filelock.FileLock(self.file, writer=True, timeout=1)
+        fd = self._fakelock()
+        with self.lock and self.assertRaises(filelock.LockTimeoutError):
+            self.lock.acquire_lock()
+        os.close(fd)
+
+if __name__ == "__main__":
+    unittest.main()
+
+# Local Variables:
+# tab-width:4
+# indent-tabs-mode:nil
+# End:
+# vim: set expandtab tabstop=4 shiftwidth=4: