fix: Data race in file cache (#366)

ankitpokhrel · Nov 18, 2021 · c48bc87 · c48bc87
1 parent fc134e9
commit c48bc87
Show file tree

Hide file tree

Showing 2 changed files with 56 additions and 25 deletions.
diff --git a/src/Cache/FileStore.php b/src/Cache/FileStore.php
@@ -8,6 +8,9 @@
 
 class FileStore extends AbstractCache
 {
+    /** @var int */
+    public const LOCK_NONE = 0;
+
     /** @var string */
     protected $cacheDir;
 
@@ -125,47 +128,61 @@ public function get(string $key, bool $withExpired = false)
     }
 
     /**
-     * Get contents of a file with shared access.
+     * @param string        $path
+     * @param int           $type
+     * @param callable|null $cb
      *
-     * @param string $path
-     *
-     * @return string
+     * @return mixed
      */
-    public function sharedGet(string $path): string
+    protected function lock(string $path, int $type = LOCK_SH, callable $cb = null)
     {
-        $contents = '';
-        $handle   = @fopen($path, File::READ_BINARY);
+        $out    = false;
+        $handle = @fopen($path, File::READ_BINARY);
 
         if (false === $handle) {
-            return $contents;
+            return $out;
         }
 
         try {
-            if (flock($handle, LOCK_SH)) {
+            if (flock($handle, $type)) {
                 clearstatcache(true, $path);
 
-                $contents = fread($handle, filesize($path) ?: 1);
-
-                flock($handle, LOCK_UN);
+                $out = $cb($handle);
             }
         } finally {
+            flock($handle, LOCK_UN);
             fclose($handle);
         }
 
-        return $contents;
+        return $out;
+    }
+
+    /**
+     * Get contents of a file with shared access.
+     *
+     * @param string $path
+     *
+     * @return string
+     */
+    public function sharedGet(string $path): string
+    {
+        return $this->lock($path, LOCK_SH, function ($handle) use ($path) {
+            return fread($handle, filesize($path) ?: 1);
+        });
     }
 
     /**
      * Write the contents of a file with exclusive lock.
      *
      * @param string $path
      * @param string $contents
+     * @param int    $lock
      *
      * @return int|false
      */
-    public function put(string $path, string $contents)
+    public function put(string $path, string $contents, int $lock = LOCK_EX)
     {
-        return file_put_contents($path, $contents, LOCK_EX);
+        return file_put_contents($path, $contents, $lock);
     }
 
     /**
@@ -180,15 +197,18 @@ public function set(string $key, $value)
             $this->createCacheFile();
         }
 
-        $contents = json_decode($this->sharedGet($cacheFile), true) ?? [];
+        return $this->lock($cacheFile, LOCK_EX, function ($handle) use ($cacheKey, $cacheFile, $value) {
+            $contents = fread($handle, filesize($cacheFile) ?: 1) ?? [];
+            $contents = json_decode($contents, true) ?? [];
 
-        if ( ! empty($contents[$cacheKey]) && \is_array($value)) {
-            $contents[$cacheKey] = $value + $contents[$cacheKey];
-        } else {
-            $contents[$cacheKey] = $value;
-        }
+            if ( ! empty($contents[$cacheKey]) && \is_array($value)) {
+                $contents[$cacheKey] = $value + $contents[$cacheKey];
+            } else {
+                $contents[$cacheKey] = $value;
+            }
 
-        return $this->put($cacheFile, json_encode($contents));
+            return $this->put($cacheFile, json_encode($contents), self::LOCK_NONE);
+        });
     }
 
     /**

diff --git a/tests/Cache/FileStoreTest.php b/tests/Cache/FileStoreTest.php
@@ -290,9 +290,9 @@ public function it_returns_false_if_cache_file_doesnt_exist(): void
      * @test
      *
      * @covers ::set
-     * @covers ::get
+     * @covers ::put
+     * @covers ::lock
      * @covers ::getCacheContents
-     * @covers ::sharedGet
      */
     public function it_gets_all_cache_contents(): void
     {
@@ -398,6 +398,7 @@ public function it_gets_actual_cache_key(): void
      * @test
      *
      * @covers ::put
+     * @covers ::lock
      * @covers ::sharedGet
      */
     public function it_gets_data_with_shared_lock(): void
@@ -436,11 +437,21 @@ public function it_gets_data_with_shared_lock(): void
     /**
      * @test
      *
+     * @covers ::lock
      * @covers ::sharedGet
      */
-    public function it_gets_empty_contents_for_invalid_file_in_shared_get(): void
+    public function it_gets_contents_in_shared_get(): void
     {
         $this->assertEmpty($this->fileStore->sharedGet(__DIR__ . '/.tmp/invalid.file'));
+
+        $file  = __DIR__ . '/../.tmp/shared.txt';
+        $text  = 'lorem ipsum';
+
+        file_put_contents($file, $text);
+
+        $this->assertEquals($text, $this->fileStore->sharedGet($file));
+
+        @unlink($file);
     }
 
     /**