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

Apc file exists cache #54

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Apc file exists cache #54

wants to merge 3 commits into from

Conversation

e0ipso
Copy link

@e0ipso e0ipso commented Mar 30, 2015

Allow flushing the file_exists APC caches (and add static cache).

This will also add a test on that feature.

@fluxsauce
Copy link
Member

Hi Mateu, this looks great. However, since Pantheon relies on this repository, I'd like to get some more feedback from them before giving it a :shipit:.

Paging @joshkoenig @davidstrauss @greg-1-anderson (not 100% sure who the point person should be)

@@ -3834,6 +3834,7 @@ function _drupal_load_stylesheet($matches) {
*/
function drupal_clear_css_cache() {
variable_del('drupal_css_cache_files');
drupal_aggregated_file_flush('css');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Flushing the cache for file existence should happen only after the deletions. Otherwise, it creates a race condition where the cache gets repopulated in a concurrent request with files that will end up deleted.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. I'll move the flushing right after:

file_scan_directory('public://css', '/.*/', array('callback' => 'drupal_delete_file_if_stale'));

@e0ipso
Copy link
Author

e0ipso commented Apr 3, 2015

@davidstrauss please re-review.

@deviantintegral
Copy link

@mateu-aguilo-bosch do we need a test case that simulates an expired APC cache item by calling apc_delete(), and then clearing caches? I don't see how the simpletest covers this issue (pasted from another ticket) - but it does look to be fixed in the actual code. Or does your test already fail if we run it against master?

On a Drupal cache clear, CSS and JS that have not been modified in 30 days are removed (configurable, but I don't think we've changed this). The file_exist calls are cached for 24 hours. I think the issue is if:

  1. An asset is still valid, but hasn't been changed in 29 days.
  2. The cache is expired in APC, and the file_exists call gets cached sometime in the last day of validity.
  3. We do a cache clear after the file is 30 days old, but before the stat call has been expired in APC.
  4. Worst case, this could lead to a broken asset for just shy of 24 hours. We could only fix it by restarting php-fpm entirely.

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.

4 participants