While doing front-end performance auditing (
LPS-83840) I discovered that some CSS wasn't being minified ( ). Investigation revealed that this is due to the async processing behavior added in bd8c6ac26a5 ( LPS-94543 ). LPS-85135
Specifically, when a CSS file is served through the AggregateFilter and it doesn't already have a cached copy, we immediately return the unminified content, while asynchronously minifying and then persisting to disk.
The idea is that the next time we hit the AggregateFilter, we'll use the cached content from disk instead, but in practice, the CacheFilter prevents us from hitting the AggregateFilter again; the CacheFilter already cached the unminified content the first time around.
To evict the poisoned cache content: restart tomcat, which causes CacheFilter to forget the unminified resource, and the next time you request it, AggregateFilter will read it off disk.
To see this in action:
- Clean out all files from "tomcat/work/Catalina/localhost/ROOT/aggregate"
- Go to pretty much any page in the control panel and look at what gets requested in the browser network pane: for example, look at the request for "/o/admin-theme/css/main.css" and see that it is not minified.
- Look in "tomcat/work/Catalina/localhost/ROOT/aggregate" and see that we actually did write a minified version of that file with a filename like "http__o_admin-theme_css_main.css7FjjBbmTK6mS22B3MUEQXHTsR28dQ84Co
- Refresh the page and see that CacheFilter continues to serve up the unminified content because AggregateFilter is not being hit.
- Restart tomcat to force CacheFilter to drop its cache.
- Request the page again, and now see that it does serve up the minified CSS read from the disk.
- We can either remove the async processing in AggregateFilter because it is fundamentally in conflict with CacheFilter.
- We can decide that the cache poisoning is acceptable because it will "fix itself" whenever the server restarts or the CacheFilter is otherwise invalidated.
- We can teach AggregateFilter to set a `Cache-Control: no-cache` header on any HTTP response that is returned provisionally while async processing is occurring in AggregateFilter.
Option 3 is the most "sophisticated" solution, but it adds complexity and arguably makes AggregateFilter a victim of leaky abstraction because it has to compensate for the behavior of another class with which it has an implicit relationship.
Option 1 is probably the best because it is the simplest, and speeding up the first request is probably not worth it if it means that we end up hurting the performance of subsequent requests. Furthermore, we don't have to worry about any browser getting unminified content and caching it for a long time due to our use of long "Expires" headers.
Option 2 doesn't seem to have much to recommend it.
Assigning to Shuyang Zhou to get your opinion on the best way forward here.