From 89381484880d36a3c88f88aab8da08a14877ed13 Mon Sep 17 00:00:00 2001 From: Colin Mollenhour Date: Mon, 24 Oct 2016 23:00:52 -0400 Subject: [PATCH 1/4] Improvements to file storage. --- .../Mage/Core/Helper/File/Storage/Database.php | 5 ++++- .../Mage/Core/Model/File/Storage/Database.php | 3 +++ .../core/Mage/Core/Model/File/Storage/File.php | 3 +++ .../Model/Resource/File/Storage/Database.php | 14 +++++++------- .../Core/Model/Resource/File/Storage/File.php | 16 +++++----------- 5 files changed, 22 insertions(+), 19 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 42ed3d1b61a..5002b37924a 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 fc44f763ab8..6585c6c60ea 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 f6a969002d8..f6833504f8f 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 c83010ddcbe..e69e8ba44f5 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 f2bf233c57b..f0d1c1039a3 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() { @@ -193,19 +194,12 @@ 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 ( ! file_exists($path . DS . $filename) || $overwrite) { + $result = @file_put_contents($path . DS . $filename, $content, LOCK_EX); if ($result !== false) { return true; @@ -214,6 +208,6 @@ public function saveFile($filePath, $content, $overwrite = false) Mage::throwException(Mage::helper('core')->__('Unable to save file: %s', $filePath)); } - return false; + return true; } } From cb00241fe33df9093071588dc40bb575f6db340d Mon Sep 17 00:00:00 2001 From: Colin Mollenhour Date: Tue, 25 Oct 2016 12:07:53 -0400 Subject: [PATCH 2/4] Use different locking techniques for overwrite flag in file storage resource. Updated get.php to not overwrite, but rather wait for shared lock before reading to avoid file-writing stampede or incomplete reads. --- .../Core/Model/Resource/File/Storage/File.php | 21 +++++++++---- get.php | 30 +++++++++---------- 2 files changed, 31 insertions(+), 20 deletions(-) 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 f0d1c1039a3..cb06ffa0e4c 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 @@ -187,7 +187,8 @@ 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) { @@ -198,10 +199,20 @@ public function saveFile($filePath, $content, $overwrite = false) @mkdir($path, 0777, true); } - if ( ! file_exists($path . DS . $filename) || $overwrite) { - $result = @file_put_contents($path . DS . $filename, $content, LOCK_EX); - - if ($result !== false) { + $fullPath = $path . DS . $filename; + if (!file_exists($fullPath)) { + // 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 85b5a33605d..18917ce43ed 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 * From 8541ee709138cef62c3175bd280df0f509c2e559 Mon Sep 17 00:00:00 2001 From: Colin Mollenhour Date: Tue, 25 Oct 2016 12:41:27 -0400 Subject: [PATCH 3/4] Remove pointless use of fgets. --- lib/Varien/File/Transfer/Adapter/Http.php | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/lib/Varien/File/Transfer/Adapter/Http.php b/lib/Varien/File/Transfer/Adapter/Http.php index ea56fbea72d..84d2ee26ba8 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); } /** From 5da91b248069e84859784d5b483b378123b279b1 Mon Sep 17 00:00:00 2001 From: Colin Mollenhour Date: Tue, 25 Oct 2016 17:06:31 -0400 Subject: [PATCH 4/4] Revert return value to false when file already exists and overwrite is not forced. --- app/code/core/Mage/Core/Model/Resource/File/Storage/File.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 cb06ffa0e4c..33dacb75cd9 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 @@ -200,7 +200,7 @@ public function saveFile($filePath, $content, $overwrite = false) } $fullPath = $path . DS . $filename; - if (!file_exists($fullPath)) { + 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) { @@ -219,6 +219,6 @@ public function saveFile($filePath, $content, $overwrite = false) Mage::throwException(Mage::helper('core')->__('Unable to save file: %s', $filePath)); } - return true; + return false; } }