Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[5.x] Fix static cache locking #10887

Merged
merged 6 commits into from
Oct 2, 2024
Merged

[5.x] Fix static cache locking #10887

merged 6 commits into from
Oct 2, 2024

Conversation

duncanmcclean
Copy link
Member

@duncanmcclean duncanmcclean commented Oct 2, 2024

This pull request attempts to fix a race condition with full-measure Static Caching, where if multiple requests come in for a URL at the same time, they might all try to write to the same file at the same time, causing mangled HTML, like seen in #10829.

I haven't been able to replicate the issue on my sandbox site, but I have been able to replicate it on a site @robdekort provided, using spatie/fork to perform concurrent requests.

To fix it, I've adjusted how we handle locking in the StaticCaching\Cache middleware, replicating how we do it elsewhere in the app. From my testing, it seems to fix the issue. 🤞

We made some changes to the static cache locking code in #10607. However, from memory, when I was last debugging this issue, I could reproduce it even after reverting that PR.

After some further testing, it seems like the original fix didn't actually fix the issue - I just got lucky. We've changed approach, see comment.

Fixes #10829.

@robdekort
Copy link
Contributor

Oh this is really neat, thank you Duncan!

@jasonvarga
Copy link
Member

We discovered the initial change in this PR didn't actually fix it.

After digging deeper we realized that the problem was the subsequent requests would wait for the first one to finish (the lock was working fine), but would then continue to write to cache the response. To fix it, after the lock was acquired we would try again to serve a cached version, since now there would be one.

@jasonvarga jasonvarga merged commit 346acb7 into 5.x Oct 2, 2024
19 checks passed
@jasonvarga jasonvarga deleted the fix/static-cache-locking branch October 2, 2024 15:00
@robdekort
Copy link
Contributor

Thanks guys!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Static cached pages contain broken HTML
3 participants