From 0e8958a7150cb9d26a9369397d6dc594e2b2f688 Mon Sep 17 00:00:00 2001 From: Colin Mollenhour Date: Mon, 24 Oct 2016 23:00:52 -0400 Subject: [PATCH] [impr-#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. --- .../Core/Helper/File/Storage/Database.php | 5 ++- .../Mage/Core/Model/File/Storage/Database.php | 3 ++ .../Mage/Core/Model/File/Storage/File.php | 3 ++ .../Model/Resource/File/Storage/Database.php | 14 ++++----- .../Core/Model/Resource/File/Storage/File.php | 31 +++++++++++-------- get.php | 30 +++++++++--------- lib/Varien/File/Transfer/Adapter/Http.php | 13 ++------ 7 files changed, 52 insertions(+), 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 index a879adfc090c..e98a0dfd09c4 100644 --- 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 index 719130e4e7a9..3b5c0800ab02 100644 --- 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 diff --git a/app/code/core/Mage/Core/Model/File/Storage/File.php b/app/code/core/Mage/Core/Model/File/Storage/File.php index 57eb0e060956..4a1924d78bbd 100644 --- 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 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 index c6b32a501731..042c3c08d991 100644 --- 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 index e70740ee80f0..7728b14f5f7d 100644 --- 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 index 2748598a2125..42ff8f469fbe 100644 --- 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 index 34861ba48262..bd8b471a716b 100644 --- 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); } /**