[libc++][lit] Atomically update the persistent cache

[arichardson]

When running multiple shards in parallel, one shard might write to the cache while another one is reading this cache. Instead of updating the file in place, write to a temporary file and swap the cache file using os.replace(). This is an atomic operation and means shards will either see the old state or the new one.

[llvmbot]

@llvm/pr-subscribers-libcxx

Changes

When running multiple shards in parallel, one shard might write to the cache while another one is reading this cache. Instead of updating the file in place, write to a temporary file and swap the cache file using os.replace(). This is an atomic operation and means shards will either see the old state or the new one.

Full diff: https://github.com/llvm/llvm-project/pull/66538.diff

1 Files Affected:

• (modified) libcxx/utils/libcxx/test/dsl.py (+5-1)

diff --git a/libcxx/utils/libcxx/test/dsl.py b/libcxx/utils/libcxx/test/dsl.py
index 847cebf5962f6aa..7d4df6b01eabdae 100644
--- a/libcxx/utils/libcxx/test/dsl.py
+++ b/libcxx/utils/libcxx/test/dsl.py
@@ -69,8 +69,12 @@ def f(config, *args, **kwargs):
             if cacheKey not in cache:
                 cache[cacheKey] = function(config, *args, **kwargs)
                 # Update the persistent cache so it knows about the new key
-                with open(persistentCache, "wb") as cacheFile:
+                # We write to a temporary file and rename the result to ensure
+                # that the cahe is not corrupted when running the test suite
+                # with multiple shards.
+                with open(persistentCache + ".tmp", "wb") as cacheFile:
                     pickle.dump(cache, cacheFile)
+                os.replace(persistentCache + ".tmp", persistentCache)
             return cache[cacheKey]
 
         return f

[ldionne]

Don't we have the same problem with the creation of the .tmp file now? Don't we need to generate a unique file name instead?

[arichardson]

Hmm that's a good point, in my limited testing this was enough to avoid the race, but really it should be a per-shard suffix. Will update to use the PID.