If another process fliushes the cache between the time less builds and drupal_get_css runs then you will get broken CSS, possibly aggregated. Not good. To avoid this, I cache flush now simply creates a new dir and life continues there. The old dir stays around for another fifteen minutes. This is made easy by creating directories named microtime(TRUE) -- the name of the directory is its own timestamp.

Note that the D7 version is untested. The D6 version is battle tested: if you enable CSS aggregation, less devel, disable page cache and hit your page with ab -c 10 -n 50 then you can repro the race easily.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chx’s picture

FileSize
2.12 KB

Fixed D6 cron using REQUEST_TIME -- it should be time().

chx’s picture

We can wonder together whether using time() is enough or whether even microtime() is not enough and we should add getmypid().

This all depends on why do we flush the LESS cache in hook_flush_caches and that I do not know. I tested with ab in devel module but that is not the same as production.

Jeremy’s picture

Why do you use mkdir() directly, instead of file_check_directory() in D6, and drupal_mkdir() or file_prepare_directory() in D7?

Jeremy’s picture

In the cleanup function in cron, why not simply check whether or not the name is the less_dir variable (which can be loaded after scanning the directory), deleting the rest?

chx’s picture

If you delete the rest then congratulations, you have created a race condition if cron hits between the time less builds and drupal_get_css runs :) Edit: provided, of course, there is a cache flush too.

chx’s picture

Status: Needs review » Needs work

I will tomorrow fix the wrong order of variable_set vs mkdir. This calls for a helper function.

chx’s picture

Status: Needs work » Needs review
FileSize
2.15 KB

Rerolled D6 only.

chx’s picture

Cron was making this sub-optimal because it was deleting every directory causing a rebuild every 15 minutes needed or not.

corey.aufang’s picture

A most annoying issue.

Your method looks solid, but is there any reason that you're using DirectoryIterator rather than file_scan_directory for D6?

corey.aufang’s picture

I'm implementing this patch right now.

In response to #1347258-3: Race condition, the reason I use mkdir directly in D6 is that it allows for recursion folder creation, which is essential to finding a compiled file's original path.

I made a couple changes to your patch's changes:


/**
 * Helper function to create a new less dir.
 */
function _less_new_dir() {
  $less_dir = uniqid('', TRUE);
  $less_path = file_directory_path() .'/less/' . $less_dir;
  @mkdir($less_path, 0775, TRUE);
  variable_set('less_dir', $less_dir);
  return $less_dir;
}

/**
 * Implements hook_cron().
 */
function less_cron() {
  $less_dir = variable_get('less_dir', '');
  $found_files = file_scan_directory(file_directory_path() . '/less', '^.+$', array('.', '..', 'CVS', $less_dir), 0, FALSE);
  
  foreach ($found_files as $found_file) {
    _less_recursive_delete($found_file->filename);
  }
}
pcoucke’s picture

FileSize
967 bytes

I thought the problem with the patch in #8 was that the name of the compressed css file changes with each "flush caches" so I developed another workaround (see attached patch). I think it is a bit simpler because it only renames the less directory before recursive deleting this.

The css filename still changes every time though, I think it is because of the serialize() call in drupal_get_css() which uses the creation date of the file or something. This name change is not good since this means the css file can not be cached very long in a browser.

corey.aufang’s picture

Status: Needs review » Fixed

This should be resolved in 7.x-2.5.

Status: Fixed » Closed (fixed)

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

Anonymous’s picture

Issue summary: View changes

added a note on the directory names

  • Commit ffb6c1d on 7.x-2.x, 7.x-3.x, 7.x-4.x:
    Issue #1246308 by corey.aufang, pillarsdotnet: Change directory to...