Merge pull request #430 from imbo/issue-426

Prevent race conditions in image transformation cache

library/Imbo/EventListener/ImageTransformationCache.php

@@ -13,6 +13,7 @@
 use Imbo\EventManager\EventInterface,
     Imbo\Http\Request\Request,
     Imbo\Model\Image,
+    Imbo\Exception\StorageException,
     Imbo\Exception\InvalidArgumentException,
     Symfony\Component\HttpFoundation\ResponseHeaderBag,
     RecursiveDirectoryIterator,
@@ -38,12 +39,19 @@
  */
 class ImageTransformationCache implements ListenerInterface {
     /**
-     * Root path where the temp. images can be stored
+     * Root path where the cached images can be stored
      *
      * @var string
      */
     private $path;
 
+    /**
+     * Whether or not this request hit a cached version
+     *
+     * @var boolean
+     */
+    private $cacheHit = false;
+
     /**
      * Class constructor
      *
@@ -120,6 +128,9 @@ public function loadFromCache(EventInterface $event) {
                 // Stop other listeners on this event
                 $event->stopPropagation();
 
+                // Mark this as a cache hit to prevent us from re-writing the result
+                $this->cacheHit = true;
+
                 return;
             } else {
                 // Invalid data in the cache, delete the file
@@ -141,8 +152,8 @@ public function storeInCache(EventInterface $event) {
         $response = $event->getResponse();
         $model = $response->getModel();
 
-        if (!$model instanceof Image) {
-            // Only store images in the cache
+        if (!$model instanceof Image || $this->cacheHit) {
+            // Only store images in the cache, and don't try to rewrite on cache hit
             return;
         }
 
@@ -163,8 +174,24 @@ public function storeInCache(EventInterface $event) {
         //
         // "What?! Did you forget to is_dir()-guard it?" - Mats Lindh
         if (is_dir($dir) || @mkdir($dir, 0775, true) || is_dir($dir)) {
-            if (file_put_contents($path. '.tmp', $data)) {
-                rename($path. '.tmp', $path);
+            $tmpPath = $path. '.tmp';
+
+            // If in the middle of a cache write operation, fall back
+            if (file_exists($tmpPath) || file_exists($path)) {
+                return;
+            }
+
+            // Write the transformed image to a temporary location
+            if (file_put_contents($tmpPath, $data)) {
+                // Move the transformed image to the correct destination
+
+                // We have to silence this in case race-conditions lead to source not existing,
+                // in which case it'll give a warning (we'd use try/catch here in case of PHP7)
+                if (@rename($path. '.tmp', $path) === false && !file_exists($path)) {
+                    throw new StorageException(
+                        'An error occured while moving transformed image to cache'
+                    );
+                }
             }
         }
     }

tests/phpunit/ImboUnitTest/EventListener/ImageTransformationCacheTest.php

@@ -107,14 +107,14 @@ public function testChangesTheImageInstanceOnCacheHit() {
         $this->request->expects($this->any())->method('getUser')->will($this->returnValue($this->user));
         $this->request->expects($this->any())->method('getImageIdentifier')->will($this->returnValue($this->imageIdentifier));
         $this->request->expects($this->any())->method('getExtension')->will($this->returnValue('png'));
-        $this->requestHeaders->expects($this->once())
+        $this->requestHeaders->expects($this->any())
                              ->method('get')
                              ->with('Accept', '*/*')
                              ->will($this->returnValue(
                                  'text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8'
                              ));
 
-        $this->query->expects($this->once())->method('get')->with('t')->will($this->returnValue(['thumbnail']));
+        $this->query->expects($this->any())->method('get')->with('t')->will($this->returnValue(['thumbnail']));
 
         $this->response->expects($this->once())->method('setModel')->with($imageFromCache)->will($this->returnSelf());
         $this->event->expects($this->once())->method('stopPropagation');
@@ -129,6 +129,8 @@ public function testChangesTheImageInstanceOnCacheHit() {
         $this->listener->loadFromCache($this->event);
 
         $this->assertInstanceOf('Symfony\Component\HttpFoundation\ResponseHeaderBag', $this->response->headers);
+
+        return $this->listener;
     }
 
     /**
@@ -213,6 +215,31 @@ public function testStoresImageInCache() {
         $this->assertEquals($this->responseHeaders, $data['headers']);
     }
 
+    /**
+     * @covers Imbo\EventListener\ImageTransformationCache::storeInCache
+     * @covers Imbo\EventListener\ImageTransformationCache::getCacheDir
+     * @covers Imbo\EventListener\ImageTransformationCache::getCacheKey
+     * @covers Imbo\EventListener\ImageTransformationCache::getCacheFilePath
+     */
+    public function testDoesNotStoreIfCachedVersionAlreadyExists() {
+        // Reusing the same logic as this test
+        $this->testChangesTheImageInstanceOnCacheHit();
+
+        $this->response->expects($this->once())->method('getModel')->will($this->returnValue($this->getMock('Imbo\Model\Image')));
+
+        // Overwrite cached file
+        $dir = 'vfs://cacheDir/u/s/e/user/7/b/f/7bf2e67f09de203da740a86cd37bbe8d/b/c/6';
+        $file = 'bc6ffe312a5741a5705afe8639c08835';
+        $fullPath = $dir . '/' . $file;
+
+        file_put_contents($fullPath, 'foobar');
+
+        // Since we hit a cached version earlier, we shouldn't overwrite the cached file
+        $this->listener->storeInCache($this->event);
+
+        $this->assertEquals('foobar', file_get_contents($fullPath));
+    }
+
     /**
      * @covers Imbo\EventListener\ImageTransformationCache::deleteFromCache
      * @covers Imbo\EventListener\ImageTransformationCache::getCacheDir