Lock creation of CacheDir config

When creating a new CacheDir, the config file is created in
exclusive mode, but there's a timing window before the json dump
to the file completes when another thread could read the config
because it exists - but get a JSONDecodeError because it hasn't
finished writing yet.  Add locking so the readers will have to
wait until the writer is done.

Fixes #4489

Signed-off-by: Mats Wichmann <mats@linux.com>

5 files changed, 16 insertions, 10 deletions

diff --git a/CHANGES.txt b/CHANGES.txt
index f1e9117..33677ea 100644
--- a/CHANGES.txt
+++ b/CHANGES.txt
@@ -104,6 +104,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
diff --git a/RELEASE.txt b/RELEASE.txt
index b339c44..8c948f9 100644
--- a/RELEASE.txt
+++ b/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
diff --git a/SCons/CacheDir.py b/SCons/CacheDir.py
index 7ac2049..0174793 100644
--- a/SCons/CacheDir.py
+++ b/SCons/CacheDir.py
@@ -34,7 +34,7 @@ import uuid
 import SCons.Action
 import SCons.Errors
 import SCons.Warnings
-import SCons
+import SCons.Util
 
 cache_enabled = True
 cache_debug = False
@@ -169,15 +169,16 @@ class CacheDir:
         """
         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 @@ class CacheDir:
                     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)
 
diff --git a/SCons/Tool/MSCommon/common.py b/SCons/Tool/MSCommon/common.py
index 2b8c67b..af3afb5 100644
--- a/SCons/Tool/MSCommon/common.py
+++ b/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.
diff --git a/SCons/Util/filelock.py b/SCons/Util/filelock.py
index 48a2a39..8ebf388 100644
--- a/SCons/Util/filelock.py
+++ b/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?