Merge pull request SCons#4490 from mwichmann/maint/cachedir_lock

Lock creation of CacheDir config

Author: bdbaddog

CHANGES.txt

@@ -108,6 +108,7 @@ RELEASE  VERSION/DATE TO BE FILLED IN LATER
       DeprecatedMissingSConscriptWarning) add two warnings to manpage
       (cache-cleanup-error, future-reserved-variable), improve unittest, tweak
       Sphinx build.
+    - Add locking around creation of CacheDir config file. Fixes #4489.
 
 
 RELEASE 4.6.0 -  Sun, 19 Nov 2023 17:22:20 -0700

RELEASE.txt

@@ -62,6 +62,7 @@ FIXES
   scanners - try harder to decode non-UTF-8 text.
 - PyPackageDir no longer fails if passed a module name which cannot be found,
   now returns None.
+- Add locking around creation of CacheDir config file. Fixes issue #4489.
 
 
 IMPROVEMENTS

SCons/CacheDir.py

@@ -34,7 +34,7 @@
 import SCons.Action
 import SCons.Errors
 import SCons.Warnings
-import SCons
+import SCons.Util
 
 cache_enabled = True
 cache_debug = False
@@ -169,15 +169,16 @@ def _readconfig(self, path):
         """
         config_file = os.path.join(path, 'config')
         try:
+            # still use a try block even with exist_ok, might have other fails
             os.makedirs(path, exist_ok=True)
-        except FileExistsError:
-            pass
         except OSError:
             msg = "Failed to create cache directory " + path
             raise SCons.Errors.SConsEnvironmentError(msg)
 
         try:
-            with open(config_file, 'x') as config:
+            with SCons.Util.FileLock(config_file, timeout=5, writer=True), open(
+                config_file, "x"
+            ) as config:
                 self.config['prefix_len'] = 2
                 try:
                     json.dump(self.config, config)
@@ -186,9 +187,11 @@ def _readconfig(self, path):
                     raise SCons.Errors.SConsEnvironmentError(msg)
         except FileExistsError:
             try:
-                with open(config_file) as config:
+                with SCons.Util.FileLock(config_file, timeout=5, writer=False), open(
+                    config_file
+                ) as config:
                     self.config = json.load(config)
-            except ValueError:
+            except (ValueError, json.decoder.JSONDecodeError):
                 msg = "Failed to read cache configuration for " + path
                 raise SCons.Errors.SConsEnvironmentError(msg)
 

SCons/Tool/MSCommon/common.py

@@ -173,7 +173,7 @@ def read_script_env_cache() -> dict:
     p = Path(CONFIG_CACHE)
     if not CONFIG_CACHE or not p.is_file():
         return envcache
-    with SCons.Util.FileLock(CONFIG_CACHE, timeout=5, writer=True), p.open('r') as f:
+    with SCons.Util.FileLock(CONFIG_CACHE, timeout=5, writer=False), p.open('r') as f:
         # Convert the list of cache entry dictionaries read from
         # json to the cache dictionary. Reconstruct the cache key
         # tuple from the key list written to json.

SCons/Util/filelock.py

@@ -42,7 +42,7 @@ class SConsLockFailure(Exception):
 class FileLock:
     """Lock a file using a lockfile.
 
-    Locking for the case where multiple processes try to hit an externally
+    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.
 
@@ -51,8 +51,9 @@ class FileLock:
     and :meth:`release_lock`.
 
     Lock can be a write lock, which is held until released, or a read
-    lock, which releases immediately upon aquisition - the interesting
-    thing being to not read a file which somebody else may be writing,
+    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.
 
     TODO: Should default timeout be None (non-blocking), or 0 (block forever),
        or some arbitrary number?