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.
Comment | File | Size | Author |
---|---|---|---|
#11 | less_race_condition.patch | 967 bytes | pcoucke |
#8 | no_less_race.patch | 2.26 KB | chx |
#8 | no_less_race_interdiff.patch | 909 bytes | chx |
#7 | no_less_race.patch | 2.15 KB | chx |
#1 | less_no_race.diff | 2.12 KB | chx |
Comments
Comment #1
chx CreditAttribution: chx commentedFixed D6 cron using REQUEST_TIME -- it should be time().
Comment #2
chx CreditAttribution: chx commentedWe 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.
Comment #3
Jeremy CreditAttribution: Jeremy commentedWhy do you use mkdir() directly, instead of file_check_directory() in D6, and drupal_mkdir() or file_prepare_directory() in D7?
Comment #4
Jeremy CreditAttribution: Jeremy commentedIn 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?
Comment #5
chx CreditAttribution: chx commentedIf 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.
Comment #6
chx CreditAttribution: chx commentedI will tomorrow fix the wrong order of variable_set vs mkdir. This calls for a helper function.
Comment #7
chx CreditAttribution: chx commentedRerolled D6 only.
Comment #8
chx CreditAttribution: chx commentedCron was making this sub-optimal because it was deleting every directory causing a rebuild every 15 minutes needed or not.
Comment #9
corey.aufang CreditAttribution: corey.aufang commentedA most annoying issue.
Your method looks solid, but is there any reason that you're using DirectoryIterator rather than file_scan_directory for D6?
Comment #10
corey.aufang CreditAttribution: corey.aufang commentedI'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:
Comment #11
pcoucke CreditAttribution: pcoucke commentedI 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.
Comment #12
corey.aufang CreditAttribution: corey.aufang commentedThis should be resolved in 7.x-2.5.
Comment #13.0
(not verified) CreditAttribution: commentedadded a note on the directory names