Problem/Motivation

It appears #1014086: Stampedes and cold cache performance issues with css/js aggregation breaks compatibility with s3fs. While this new method appears to be a boost for Drupal performance relies on css no longer being generated with the page request and instead uses an asynchronous call to generate css.

Drupal Core assumes that the link for the async request will be the same as the storage location (getExternalUrl()) which is not possible with s3fs and other streamWrapeprs that store files outside of local storage.

Steps to reproduce

Drupal 10.1 dev build with s3fs installed and public:// takeover enabled.

Proposed resolution

  • Add detection of CSS/JS object existence to S3fsStream::getExternalUrl() and modify url generation accordingly.
  • Decorate or Replace Drupal\Core\Asset\CssCollectionRenderer and Drupal\Core\Asset\JsCollectionRenderer to include cache metadata in the render array so that we may purge pages that have links to css that has not been generated yet.
  • Replace Drupal\system\Controller\CssAssetController and Drupal\system\Controller\JsAssetController as they would also be subject to the same type of vulnerability as addressed in #3298703: Core ImageStyleDownloadControler allow DoS for s3fs.. Additionally we also need to call a cache purge on pages that have the link to css generation so the pages regenerate with the link to the bucket.

Some sites may want to consider adopting #3081756: Provide a module for serving agregated CSS and JSS from local public filesystem instead of S3 when option "use_s3_for_public" is used and rely solely on local css/js objects,

Remaining tasks

  • Suggest Drupal Core make this change opt-in until D11 as it appears to break backwards compatibility with external streamWrappers in a minor release.
  • Encourage Core to use bubbleMetadata in CssCollectionRender and JsCollectionRender to avoid code duplication in s3fs.
  • Investigate if Drupal\Core\Asset\CssCollectionOptimizerLazy and Drupal\Core\Asset\JsCollectionOptimizerLazy require replacement, especially around the calls to generating file url's.
  • Investigate if an OutboundRouteProcessor can and should be added provide cache tags for other locations that generate Url objects.

User interface changes

None

API changes

TBD

Data model changes

TBD

Comments

cmlara created an issue. See original summary.

catch’s picture

catch
he/him
Primary language English
commented

I think the ideal change here would be to also get #3027639: Make css/js optimized assets path configurable into 10.1.x, so that asset aggregates are out of s3fs entirely when the public:// takeover is in place.

I've been using that patch on sites with s3fs public:// takeover for several months since it resolves performance issues with the existing behaviour too.

cmlara’s picture

cmlara
he/him
commented
Status: Active » Postponed
Related issues: +#3027639: Make css/js optimized assets path configurable

#3027639: Make css/js optimized assets path configurable looks like a good solution.

Postponing while we see what happens there.

cmlara’s picture

cmlara
he/him
commented
Status: Postponed » Closed (outdated)

With #3027639: Make css/js optimized assets path configurable committed the above requirements are no longer necessary as Drupal no longer will store assets on the public:// streamWrapper making the s3fs module no longer responsible for CSS/JS storage.

Testing against Drupal 10.1.x dev branch and s3fs 8.x-3.1 pages load, assets are stored on the Drupal instance.

As noted above, the new core change also handles a previous s3fs feature request of not storing css/js assets inside s3 storage.

Thank you again @catch for pointing out an issue that proved to be a reasonable compromise to avoid a re-work inside of s3fs.

Closing this out as outdated, we can track the rest of D10.1 support in #3346117: Use with Drupal 10.1 as we get closer to a release date.