[perf-OpenMage#601] Prevent redundant simultaneous requests to remote…

… storage in get.php
edannenberg · Aug 24, 2020 · e6ae319 · e6ae319
1 parent 7603294
commit e6ae319
Show file tree

Hide file tree

Showing 3 changed files with 102 additions and 19 deletions.
diff --git a/app/code/core/Mage/Core/Model/File/Storage/File.php b/app/code/core/Mage/Core/Model/File/Storage/File.php
@@ -275,4 +275,22 @@ public function saveFile($file, $overwrite = true)
 
         return false;
     }
+
+    /**
+     * @param $filePath
+     * @return bool
+     */
+    public function lockCreateFile($filePath)
+    {
+        return $this->getResource()->lockCreateFile($filePath);
+    }
+
+    /**
+     * @param $filePath
+     */
+    public function removeLockedFile($filePath)
+    {
+        $this->getResource()->removeLockedFile($filePath);
+    }
+
 }
diff --git a/app/code/core/Mage/Core/Model/Resource/File/Storage/File.php b/app/code/core/Mage/Core/Model/Resource/File/Storage/File.php
@@ -47,6 +47,9 @@ class Mage_Core_Model_Resource_File_Storage_File
      */
     protected $_ignoredFiles;
 
+    /** @var resource */
+    protected $filePointer;
+
     /**
      * Files at storage
      *
@@ -200,14 +203,22 @@ public function saveFile($filePath, $content, $overwrite = false)
         }
 
         $fullPath = $path . DS . $filename;
-        if (!file_exists($fullPath) || $overwrite) {
+        if ($this->filePointer || !file_exists($fullPath) || $overwrite) {
+            // If we already opened the file using lockCreateFile method
+            if ($this->filePointer) {
+                $fp = $this->filePointer;
+                $this->filePointer = NULL;
+                if (@fwrite($fp, $content) !== false && @fflush($fp) && @flock($fp, LOCK_UN) && @fclose($fp)) {
+                    return true;
+                }
+            }
             // If overwrite is not required then return if file could not be locked (assume it is being written by another process)
             // Exception is only thrown if file was opened but could not be written.
-            if (!$overwrite) {
+            else if (!$overwrite) {
                 if (!($fp = @fopen($fullPath, 'x'))) {
                     return false;
                 }
-                if (@fwrite($fp, $content) !== false && @fclose($fp)) {
+                if (@fwrite($fp, $content) !== false && @fflush($fp) && @fclose($fp)) {
                     return true;
                 }
             }
@@ -221,4 +232,58 @@ public function saveFile($filePath, $content, $overwrite = false)
 
         return false;
     }
+
+    /**
+     * Create a new file already locked by this process and save the handle for later writing by saveFile method.
+     *
+     * @param $filePath
+     * @return bool
+     */
+    public function lockCreateFile($filePath)
+    {
+        $filename = basename($filePath);
+        $path = $this->getMediaBaseDirectory() . DS . str_replace('/', DS ,dirname($filePath));
+
+        if (!is_dir($path)) {
+            @mkdir($path, 0777, true);
+        }
+
+        $fullPath = $path . DS . $filename;
+
+        // Get exclusive lock on new or existing file
+        if ($fp = @fopen($fullPath, 'c')) {
+            @flock($fp, LOCK_EX);
+            @fseek($fp, 0, SEEK_END);
+            if (@ftell($fp) === 0) { // If the file is empty we can write to it
+                $this->filePointer = $fp;
+                return true;
+            } else { // Otherwise we should not write to it
+                @flock($fp, LOCK_UN);
+                @fclose($fp);
+                return false;
+            }
+        }
+
+        return false;
+    }
+
+    /**
+     * Unlock, close and remove a locked file (in case the file could not be read from remote storage)
+     *
+     * @param $filePath
+     */
+    public function removeLockedFile($filePath)
+    {
+        $filename = basename($filePath);
+        $path = $this->getMediaBaseDirectory() . DS . str_replace('/', DS ,dirname($filePath));
+        $fullPath = $path . DS . $filename;
+        if ($this->filePointer) {
+            $fp = $this->filePointer;
+            $this->filePointer = NULL;
+            @flock($fp, LOCK_UN);
+            @fclose($fp);
+        }
+        @unlink($fullPath);
+    }
+
 }
diff --git a/get.php b/get.php
@@ -142,24 +142,24 @@
     sendNotFoundPage();
 }
 
-$databaseFileStorage = Mage::getModel('core/file_storage_database');
+$localStorage = Mage::getModel('core/file_storage_file');
+$remoteStorage = Mage::getModel('core/file_storage_database');
 try {
-    $databaseFileStorage->loadByFilename($relativeFilename);
-} catch (Exception $e) {
-}
-if ($databaseFileStorage->getId()) {
-    try {
-        if (Mage::getModel('core/file_storage_file')->saveFile($databaseFileStorage, false) === false) {
-            // False return value means file was not overwritten. However, it may not be ready
-            // to be read yet so wait for shared lock to ensure file is not partially read.
-            if ($fp = fopen($filePath, 'r')) {
-                flock($fp, LOCK_SH) && flock($fp, LOCK_UN) && fclose($fp);
-            }
+    if ($localStorage->lockCreateFile($relativeFilename)) {
+        try {
+            $remoteStorage->loadByFilename($relativeFilename);
+        } catch (Exception $e) {
+            // Ignore errors
+        }
+        if ($remoteStorage->getId()) {
+            $localStorage->saveFile($remoteStorage, false);
+        } else {
+            $localStorage->removeLockedFile($relativeFilename);
         }
-        sendFile($filePath);
-    } catch (Exception $e) {
-        Mage::logException($e);
     }
+    sendFile($filePath);
+} catch (Exception $e) {
+    Mage::logException($e);
 }
 
 sendNotFoundPage();
@@ -200,7 +200,7 @@ function checkResource($resource, array $allowedResources)
  */
 function sendFile($file)
 {
-    if (file_exists($file) || is_readable($file)) {
+    if (is_readable($file) && filesize($file) > 0) {
         $transfer = new Varien_File_Transfer_Adapter_Http();
         $transfer->send($file);
         exit;