From e6ae319ad9ae5cab24e2d6ad88dbad635776f699 Mon Sep 17 00:00:00 2001 From: Colin Mollenhour Date: Thu, 11 Jun 2020 17:01:24 -0400 Subject: [PATCH] [perf-#601] Prevent redundant simultaneous requests to remote storage in get.php --- .../Mage/Core/Model/File/Storage/File.php | 18 +++++ .../Core/Model/Resource/File/Storage/File.php | 71 ++++++++++++++++++- get.php | 32 ++++----- 3 files changed, 102 insertions(+), 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 index 4d78febd539..6483d3cca0c 100644 --- 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 index f2ea5c2b74d..275c17f6cd5 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 @@ -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 index c48cef6651d..09ea490ec55 100644 --- 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;