Problem/Motivation
When retrieving an upstream files, it's possible for multiple requests to require the same image or file concurrently. As is, a warning will be triggered and a `Location` header sent to try again.
Unfortunately, this is confusing when debugging other issues, because it looks like something is wrong with the download code or the underlying file system. As well, it's possible for the upstream system to start denying requests (or even all of them!) if it thinks it's being attacked.
Since Stage File Proxy is nearly always used on locals and development environments, holding a PHP process open with a lock for an additional second has few downsides and avoids triggering file races at all.
Steps to reproduce
Fetch an image that needs to be downloaded from the origin with multiple processes concurrently.
Proposed resolution
Unfortunately \Drupal\stage_file_proxy\FetchManagerInterface declares a constructor in it's interface, which makes it hard to inject additional services. As such, we'll need to:
- Deprecate
\Drupal\stage_file_proxy\FetchManagerInterfaceand create a new interface, along with an underlying class. FetchManager can decorate the new class. - Deprecate
\Drupal\stage_file_proxy\EventSubscriber\ProxySubscriberso we can change it's protected $manager property. - Write new classes that support injecting the lock service.
| Comment | File | Size | Author |
|---|---|---|---|
| #7 | 3282542-7-re-roll-lock.patch | 23.65 KB | edmoreta |
Issue fork stage_file_proxy-3282542
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
deviantintegral commentedI've pushed to the issue fork. This change combined with #3282524: Fix writeFile() returning TRUE even if a file has no size fixes using Stage File Proxy with visual regression tools.
Comment #4
deviantintegral commentedComment #7
edmoreta commentedI've re-rolled the patch so it doesn't fail when applying the patch to the latest `8.x-1.x` changes (I'm using stage_file_proxy v1.5).
Comment #8
smustgrave commented2.0.x is already out so deprecation needs to be updated
Comment #9
smustgrave commentedAnd there are test failures.
Comment #10
yesct commentedI tested out the new 8.x-1.x patch from #7 and it really seemed to improve the loading of images, when I am building a lot of previews of a site (all previews use stage_file_proxy), and my visual diffs are showing a lot more loaded images.
Comment #11
deviantintegral commentedI've updated the MR with new versions for the deprecation notices. Next, tests!
Comment #12
smustgrave commentedJust wanted to point out https://www.drupal.org/project/stage_file_proxy/issues/2928564 in case they were related.
Comment #15
deviantintegral commentedComment #17
smustgrave commentedMerged in the changes
Also opened #3334239: 2.0.3 Release to make sure we get a bunch of testing before doing a release.
Thanks for the work!