Prevent redundant simultaneous requests to remote storage in get.php

[colinmollenhour]

The current get.php mechanism for fetching files from remote storage does not fully prevent a stampede of requests for the same resource from resulting in multiple fetch operations from the remote storage. That is, the remote fetching happens before the file locking. When the database storage is rewritten to use a third-party extension like Thai_S3 this can result in significantly more bandwidth being used and files being written multiple times. A situation that is not too uncommon would be if you update the watermark parameters then every resized image suddenly must be regenerated and there is currently not a good way (that I know of) to pre-warm the cache for this scenario so we are trying to make the remote fetching as efficient as possible.

This change reduces race conditions and duplicate fetch operations by locking the file before it is fetched and only releasing the lock after it is written so that other processes will be able to read the file written by the first process even if they started before the file existed locally.

[tmotyl]

Hi @colinmollenhour
can you give more background for this change?

[colinmollenhour]

moved to PR description

[colinmollenhour]

This has been tested with ab to prove that given 20 simultaneous requests to get.php with cold local storage the result is:

Before: Up to 20 fetches from remote storage, usually all successful but not always
After: 1 fetch to remote storage and all 20 requests always successful

[Flyingmana]

@sreichel is there a reason for closing it?

[sreichel]

Sorry, just hit the wrong button.

[addison74]

A serious bug was discovered related to this commit. I was able to create as many folders as I wanted in some folders inside media directory. With a simple bash script running a curl command in a loop I can create trees of folders. A Denial of Service (DoS) can be achieve in this way. By reverting the changes to vanilla Magento this issue is gone.

I suggest removing this implementation from OpenMage for a while till you find a solution.

For more details please check this report here: #1334.

[midlan]

@Flyingmana we are not following the Magento convention that private/protected files have underscore prefix?

[addison74]

@colinmollenhour: My personal opinion is to revert this change to Magento default till you provide a final solution for the issue reported here #1334. You provided a solution here #1338 but it is still a temporary one.

Once there is a complete solution and tested you can merge all changes into the master branch.