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:

  1. Deprecate \Drupal\stage_file_proxy\FetchManagerInterface and create a new interface, along with an underlying class. FetchManager can decorate the new class.
  2. Deprecate \Drupal\stage_file_proxy\EventSubscriber\ProxySubscriber so we can change it's protected $manager property.
  3. Write new classes that support injecting the lock service.
CommentFileSizeAuthor
#7 3282542-7-re-roll-lock.patch23.65 KBedmoreta
Command icon 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

deviantintegral created an issue. See original summary.

deviantintegral’s picture

Assigned: deviantintegral » Unassigned
Status: Active » Needs review

I'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.

deviantintegral’s picture

Title: Add a lock around retrieving upstream images » Add a lock around retrieving upstream files
Issue summary: View changes

edmoreta’s picture

StatusFileSize
new23.65 KB

I'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).

smustgrave’s picture

Status: Needs review » Needs work

2.0.x is already out so deprecation needs to be updated

smustgrave’s picture

Version: 8.x-1.x-dev » 2.0.x-dev

And there are test failures.

yesct’s picture

I 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.

deviantintegral’s picture

I've updated the MR with new versions for the deprecation notices. Next, tests!

smustgrave’s picture

Just wanted to point out https://www.drupal.org/project/stage_file_proxy/issues/2928564 in case they were related.

deviantintegral’s picture

Status: Needs work » Needs review

smustgrave’s picture

Status: Needs review » Fixed

Merged 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!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.