Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improvements to file storage. #127

Merged
merged 4 commits into from
Mar 1, 2017

Conversation

colinmollenhour
Copy link
Member

@colinmollenhour colinmollenhour commented Oct 25, 2016

  • 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

…esource.

Updated get.php to not overwrite, but rather wait for shared lock before reading to
avoid file-writing stampede or incomplete reads.
@colinmollenhour
Copy link
Member Author

Thanks for the review guys, I have pushed a new commit to address issues you raised. I agree about the return value so reverted it back but also made some new changes. I believe this keeps 100% compatibility but the improvements to get.php should improve stability a lot.

Explanation:
Let's assume we have a cold local cache for a given file (e.g. watermark parameters changing invalidates old resized images or a merged js/css file was updated or you simply launched a new instance with an empty media directory). If a few requests come in at the same time for the same file each one will try to read the file from central storage into the local cache. In many cases disks are so fast that race conditions are not a problem but with high traffic there can be two major issues:

  1. Multiple processes try to write at the same time. While EX locking was already used before, this just ensures that they don't write at the same time but it does not prevent multiple writes to the same file. This is a waste of disk utilization and slows the response times. Only the first one really needs to write the file.
  2. File reads (mime type, filesize, readfile) are not using locking so the file could be changing while it is being read. I've experienced this before even without get.php where js and css files get partially read and unfortunately one error becomes thousands when the file gets cached in a proxy.

Also just spotted a completely pointless block of code which should have simply been "readfile()". Reading files line by line just to echo each line is frankly retarded; imagine an XML sitemap with 100k lines. Leaving proper buffering up to the php devs is definitely the way to go.

@colinmollenhour
Copy link
Member Author

Before you mention the use of the error suppression operator let me explain. :)

Magento registers an error handler so if an error is raised Magento throws an exception or logs an error. Varien_Io_File also uses this operator so I was trying to remain consistent with it. Also, I did not want to modify the use of translated strings to add details to exception messages. Lastly, sometimes an error is expected or at least inconsequential so there is no need to litter logs with unimportant warnings.

@Flyingmana
Copy link
Contributor

besides the true/false Iam ok with it now

@colinmollenhour colinmollenhour added the review needed Problem should be verified label Oct 25, 2016
@cieslix
Copy link
Contributor

cieslix commented Mar 1, 2017

+1
Code looks much better then previous.
I've tested manipulation on files with storage file/database.
Coping/deleting/syncing works perfect.

@colinmollenhour colinmollenhour merged commit a1e42bc into OpenMage:1.9.3.0 Mar 1, 2017
@tmotyl
Copy link
Contributor

tmotyl commented Mar 2, 2017

@colinmollenhour shouldn't it be also merged to 1.9.3.x ?

@colinmollenhour colinmollenhour deleted the file-storage branch March 4, 2017 16:58
edannenberg pushed a commit to edannenberg/magento-lts that referenced this pull request Dec 6, 2017
* 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 pushed a commit to edannenberg/magento-lts that referenced this pull request Dec 6, 2017
* 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.
@sreichel sreichel added enhancement review needed Problem should be verified and removed review needed Problem should be verified labels Jan 11, 2018
edannenberg pushed a commit to edannenberg/magento-lts that referenced this pull request Feb 28, 2018
* 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 pushed a commit to edannenberg/magento-lts that referenced this pull request Mar 20, 2018
* 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 pushed a commit to edannenberg/magento-lts that referenced this pull request Jul 17, 2018
* 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 pushed a commit to edannenberg/magento-lts that referenced this pull request Sep 19, 2018
* 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 pushed a commit to edannenberg/magento-lts that referenced this pull request Feb 14, 2019
* 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 pushed a commit to edannenberg/magento-lts that referenced this pull request Apr 1, 2019
* 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 pushed a commit to edannenberg/magento-lts that referenced this pull request Aug 22, 2019
* 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 pushed a commit to edannenberg/magento-lts that referenced this pull request Oct 25, 2019
* 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 pushed a commit to edannenberg/magento-lts that referenced this pull request Aug 20, 2020
* 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement review needed Problem should be verified
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants