[impr-OpenMage#127] Improvements to file storage

* saveFileToFilesystem returns void instead of bool in some cases. * fileExists loads entire file from database just to return a bool. * Use of Varien_Io_File seems pointless; deleting file before saving is also pointless. * fix multi-process concurrency issues in get.php. * use more efficient readfile() method to send files * avoid file-writing stampede or incomplete reads in get.php. * remove pointless use of fgets.
edannenberg · Feb 28, 2018 · f9d6366 · f9d6366
1 parent 698e2de
commit f9d6366
Show file tree

Hide file tree

Showing 7 changed files with 52 additions and 47 deletions.
diff --git a/app/code/core/Mage/Core/Helper/File/Storage/Database.php b/app/code/core/Mage/Core/Helper/File/Storage/Database.php
@@ -197,7 +197,8 @@ public function getUniqueFilename($directory, $filename)
      * @param string $filename
      * @return bool|int
      */
-    public function saveFileToFilesystem($filename) {
+    public function saveFileToFilesystem($filename)
+    {
         if ($this->checkDbUsage()) {
             /** @var $file Mage_Core_Model_File_Storage_Database */
             $file = Mage::getModel('core/file_storage_database')
@@ -208,6 +209,8 @@ public function saveFileToFilesystem($filename) {
 
             return $this->getStorageFileModel()->saveFile($file, true);
         }
+
+        return false;
     }
 
     /**

diff --git a/app/code/core/Mage/Core/Model/File/Storage/Database.php b/app/code/core/Mage/Core/Model/File/Storage/Database.php
@@ -28,6 +28,9 @@
 /**
  * File storage database model class
  *
+ * @method Mage_Core_Model_Resource_File_Storage_Database _getResource()
+ * @method Mage_Core_Model_Resource_File_Storage_Database getResource()
+ *
  * @category    Mage
  * @package     Mage_Core
  * @author      Magento Core Team <core@magentocommerce.com>

diff --git a/app/code/core/Mage/Core/Model/File/Storage/File.php b/app/code/core/Mage/Core/Model/File/Storage/File.php
@@ -28,6 +28,9 @@
 /**
  * Abstract model class
  *
+ * @method Mage_Core_Model_Resource_File_Storage_File _getResource()
+ * @method Mage_Core_Model_Resource_File_Storage_File getResource()
+ *
  * @category    Mage
  * @package     Mage_Core
  * @author      Magento Core Team <core@magentocommerce.com>

diff --git a/app/code/core/Mage/Core/Model/Resource/File/Storage/Database.php b/app/code/core/Mage/Core/Model/Resource/File/Storage/Database.php
@@ -126,7 +126,7 @@ protected function _decodeAllFilesContent($rows)
      * @param  Mage_Core_Model_File_Storage_Database $object
      * @param  string $filename
      * @param  string $path
-     * @return Mage_Core_Model_Mysql4_File_Storage_Database
+     * @return Mage_Core_Model_Resource_File_Storage_Database
      */
     public function loadByFilename(Mage_Core_Model_File_Storage_Database $object, $filename, $path)
     {
@@ -150,7 +150,7 @@ public function loadByFilename(Mage_Core_Model_File_Storage_Database $object, $f
     /**
      * Clear files in storage
      *
-     * @return Mage_Core_Model_Mysql4_File_Storage_Database
+     * @return Mage_Core_Model_Resource_File_Storage_Database
      */
     public function clearFiles()
     {
@@ -186,8 +186,8 @@ public function getFiles($offset = 0, $count = 100)
     /**
      * Save file to storage
      *
-     * @param  Mage_Core_Model_File_Storage_Database|array $object
-     * @return Mage_Core_Model_Mysql4_File_Storage_Database
+     * @param array|Mage_Core_Model_File_Storage_Database $file
+     * @return Mage_Core_Model_Resource_File_Storage_Database
      */
     public function saveFile($file)
     {
@@ -215,7 +215,7 @@ public function saveFile($file)
      * @param  string $oldPath
      * @param  string $newFilename
      * @param  string $newPath
-     * @return Mage_Core_Model_Mysql4_File_Storage_Database
+     * @return Mage_Core_Model_Resource_File_Storage_Database
      */
     public function renameFile($oldFilename, $oldPath, $newFilename, $newPath)
     {
@@ -237,7 +237,7 @@ public function renameFile($oldFilename, $oldPath, $newFilename, $newPath)
      * @param  string $oldPath
      * @param  string $newFilename
      * @param  string $newPath
-     * @return Mage_Core_Model_Mysql4_File_Storage_Database
+     * @return Mage_Core_Model_Resource_File_Storage_Database
      */
     public function copyFile($oldFilename, $oldPath, $newFilename, $newPath)
     {
@@ -277,7 +277,7 @@ public function fileExists($filename, $path)
         $adapter = $this->_getReadAdapter();
 
         $select = $adapter->select()
-            ->from(array('e' => $this->getMainTable()))
+            ->from(array('e' => $this->getMainTable()), 'file_id')
             ->where('filename = ?', $filename)
             ->where($adapter->prepareSqlCondition('directory', array('seq' => $path)))
             ->limit(1);

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
@@ -51,6 +51,7 @@ class Mage_Core_Model_Resource_File_Storage_File
      * Files at storage
      *
      * @var array
+     * @return string
      */
     public function getMediaBaseDirectory()
     {
@@ -186,28 +187,32 @@ public function saveDir($dir)
      * @param  string $filePath
      * @param  string $content
      * @param  bool $overwrite
-     * @return bool
+     * @return bool true if file written, otherwise false
+     * @throws Mage_Core_Exception
      */
     public function saveFile($filePath, $content, $overwrite = false)
     {
         $filename = basename($filePath);
         $path = $this->getMediaBaseDirectory() . DS . str_replace('/', DS ,dirname($filePath));
 
-        if (!file_exists($path) || !is_dir($path)) {
+        if (!is_dir($path)) {
             @mkdir($path, 0777, true);
         }
 
-        $ioFile = new Varien_Io_File();
-        $ioFile->cd($path);
-
-        if (!$ioFile->fileExists($filename) || ($overwrite && $ioFile->rm($filename))) {
-            $ioFile->streamOpen($filename);
-            $ioFile->streamLock(true);
-            $result = $ioFile->streamWrite($content);
-            $ioFile->streamUnlock();
-            $ioFile->streamClose();
-
-            if ($result !== false) {
+        $fullPath = $path . DS . $filename;
+        if (!file_exists($fullPath) || $overwrite) {
+            // 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) {
+                if (!($fp = @fopen($fullPath, 'x'))) {
+                    return false;
+                }
+                if (@fwrite($fp, $content) !== false && @fclose($fp)) {
+                    return true;
+                }
+            }
+            // If overwrite is required, throw exception on failure to write file
+            else if (@file_put_contents($fullPath, $content, LOCK_EX) !== false) {
                 return true;
             }
 

diff --git a/get.php b/get.php
@@ -142,27 +142,26 @@
     sendNotFoundPage();
 }
 
+$databaseFileStorage = Mage::getModel('core/file_storage_database');
 try {
-    $databaseFileSotrage = Mage::getModel('core/file_storage_database');
-    $databaseFileSotrage->loadByFilename($relativeFilename);
+    $databaseFileStorage->loadByFilename($relativeFilename);
 } catch (Exception $e) {
 }
-if ($databaseFileSotrage->getId()) {
-    $directory = dirname($filePath);
-    if (!is_dir($directory)) {
-        mkdir($directory, 0777, true);
-    }
-
-    $fp = fopen($filePath, 'w');
-    if (flock($fp, LOCK_EX | LOCK_NB)) {
-        ftruncate($fp, 0);
-        fwrite($fp, $databaseFileSotrage->getContent());
+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);
+            }
+        }
+        sendFile($filePath);
+    } catch (Exception $e) {
+        Mage::logException($e);
     }
-    flock($fp, LOCK_UN);
-    fclose($fp);
 }
 
-sendFile($filePath);
 sendNotFoundPage();
 
 /**
@@ -193,6 +192,7 @@ function checkResource($resource, array $allowedResources)
         sendNotFoundPage();
     }
 }
+
 /**
  * Send file to browser
  *

diff --git a/lib/Varien/File/Transfer/Adapter/Http.php b/lib/Varien/File/Transfer/Adapter/Http.php
@@ -80,7 +80,7 @@ class Varien_File_Transfer_Adapter_Http
      * Send the file to the client (Download)
      *
      * @param  string|array $options Options for the file(s) to send
-     * @return void
+     * @throws Exception
      */
     public function send($options = null)
     {
@@ -105,16 +105,7 @@ public function send($options = null)
 
         $response->sendHeaders();
 
-        $handle = fopen($filepath, 'r');
-        if ($handle) {
-            while (($buffer = fgets($handle, 4096)) !== false) {
-                echo $buffer;
-            }
-            if (!feof($handle)) {
-                throw new Exception("Error: unexpected fgets() fail.");
-            }
-            fclose($handle);
-        }
+        readfile($filepath);
     }
 
     /**