use shared read/write lock in ReflectionCache to prevent race conditions (#160)

Co-authored-by: Markus Staab <[email protected]>

Author: staabm

src/QueryReflection/ReflectionCache.php

@@ -28,9 +28,26 @@ final class ReflectionCache
      */
     private $changes = [];
 
+    /**
+     * @var resource
+     */
+    private static $lockHandle;
+
     private function __construct(string $cacheFile)
     {
         $this->cacheFile = $cacheFile;
+
+        if (null === self::$lockHandle) {
+            // prevent parallel phpstan-worker-process from writing into the cache file at the same time
+            // XXX we use a single system-wide lock file, which might get problematic if multiple users run phpstan on the same machine at the same time
+            $lockFile = sys_get_temp_dir().'/staabm-phpstan-dba-cache.lock';
+            $lockHandle = fopen($lockFile, 'w+');
+            if (false === $lockHandle) {
+                throw new DbaException(sprintf('Could not open lock file "%s".', $this->cacheFile));
+            }
+
+            self::$lockHandle = $lockHandle;
+        }
     }
 
     public static function create(string $cacheFile): self
@@ -41,7 +58,7 @@ public static function create(string $cacheFile): self
     public static function load(string $cacheFile): self
     {
         $reflectionCache = new self($cacheFile);
-        $cachedRecords = $reflectionCache->readCache();
+        $cachedRecords = $reflectionCache->readCache(true);
         if (null !== $cachedRecords) {
             $reflectionCache->records = $cachedRecords;
         }
@@ -52,15 +69,24 @@ public static function load(string $cacheFile): self
     /**
      * @return array<string, array{error?: ?Error, result?: array<QueryReflector::FETCH_TYPE*, ?Type>}>|null
      */
-    private function readCache(): ?array
+    private function readCache(bool $useReadLock): ?array
     {
         if (!is_file($this->cacheFile)) {
             if (false === file_put_contents($this->cacheFile, '')) {
                 throw new DbaException(sprintf('Cache file "%s" is not readable and creating a new one did not succeed.', $this->cacheFile));
             }
         }
-        clearstatcache(true, $this->cacheFile);
-        $cache = require $this->cacheFile;
+
+        try {
+            if ($useReadLock) {
+                flock(self::$lockHandle, \LOCK_SH);
+            }
+            $cache = require $this->cacheFile;
+        } finally {
+            if ($useReadLock) {
+                flock(self::$lockHandle, \LOCK_UN);
+            }
+        }
 
         if (\is_array($cache) && \array_key_exists('schemaVersion', $cache) && self::SCHEMA_VERSION === $cache['schemaVersion']) {
             return $cache['records'];
@@ -75,47 +101,40 @@ public function persist(): void
             return;
         }
 
-        // prevent parallel phpstan-worker-process from writing into the cache file at the same time
-        // XXX we use a single system-wide lock file, which might get problematic if multiple users run phpstan on the same machine at the same time
-        $lockFile = sys_get_temp_dir().'/staabm-phpstan-dba-cache.lock';
-        $lockHandle = fopen($lockFile, 'w+');
-        if (false === $lockHandle) {
-            throw new DbaException(sprintf('Could not open cache file "%s" for writing', $this->cacheFile));
-        }
-        flock($lockHandle, LOCK_EX);
+        try {
+            flock(self::$lockHandle, LOCK_EX);
 
-        // freshly read the cache as it might have changed in the meantime
-        $cachedRecords = $this->readCache();
+            // freshly read the cache as it might have changed in the meantime
+            $cachedRecords = $this->readCache(false);
 
-        $handle = fopen($this->cacheFile, 'w+');
-        if (false === $handle) {
-            throw new DbaException(sprintf('Could not open cache file "%s" for writing', $this->cacheFile));
-        }
+            // re-apply all changes to the current cache-state
+            if (null === $cachedRecords) {
+                $newRecords = $this->changes;
+            } else {
+                $newRecords = array_merge($cachedRecords, $this->changes);
+            }
 
-        // re-apply all changes to the current cache-state
-        if (null === $cachedRecords) {
-            $newRecords = $this->changes;
-        } else {
-            $newRecords = array_merge($cachedRecords, $this->changes);
-        }
+            // sort records to prevent unnecessary cache invalidation caused by different order of queries
+            uksort($newRecords, function ($queryA, $queryB) {
+                return $queryA <=> $queryB;
+            });
 
-        // sort records to prevent unnecessary cache invalidation caused by different order of queries
-        uksort($newRecords, function ($queryA, $queryB) {
-            return $queryA <=> $queryB;
-        });
+            $cacheContent = '<?php return '.var_export([
+                    'schemaVersion' => self::SCHEMA_VERSION,
+                    'records' => $newRecords,
+                ], true).';';
 
-        $cacheContent = '<?php return '.var_export([
-                'schemaVersion' => self::SCHEMA_VERSION,
-                'records' => $newRecords,
-            ], true).';';
+            if (false === file_put_contents($this->cacheFile, $cacheContent)) {
+                throw new DbaException(sprintf('Unable to write cache file "%s"', $this->cacheFile));
+            }
 
-        if (false === fwrite($handle, $cacheContent)) {
-            throw new DbaException(sprintf('Unable to write cache file "%s"', $this->cacheFile));
+            clearstatcache(true, $this->cacheFile);
+            if (\function_exists('opcache_invalidate')) {
+                opcache_invalidate($this->cacheFile, true);
+            }
+        } finally {
+            flock(self::$lockHandle, \LOCK_UN);
         }
-
-        fclose($handle);
-        // will free the lock implictly
-        fclose($lockHandle);
     }
 
     public function hasValidationError(string $queryString): bool