Problem/Motivation
The module currently relies upon the time() function to track when various caches are cleared. The time() function tracks events to the nearest second. The problem is that there can be thousands of operations within an individual second, so a second becomes an extremely imprecise value to work from. The current system leads to issues like #2444221: cache_lifetime should not affect cache_clear_all(), whereby it's impossible to discern between cache_set() being called before or after a cache_clear_all().
Proposed resolution
Change all time-based logic to use microtime() instead of time().
Remaining tasks
Write a patch, test it.
User interface changes
None.
API changes
None.
Data model changes
Additional variables are kept to track the microtime of various events in addition to the timestamp.
| Comment | File | Size | Author |
|---|---|---|---|
| #25 | memcache-n2543030-25.patch | 10.01 KB | mrinalini9 |
| #22 | memcache-n2543030-22.patch | 10.09 KB | damienmckenna |
Comments
Comment #1
damienmckennaComment #2
damienmckennaFood for thought. This adds some reliance upon $cache->created_microtime. It could probably use a little tidying up too.
Comment #3
damienmckennaWrapped some of the logic (if() statements) to make the lines easier to follow.
Comment #4
damienmckennaImprovements to make the same logic improvements from the cache_flush logic also available on the other scenarios, and added comments explaining the logic.
Comment #5
damienmckennaSome sample code that can be added to a PHP script and then loaded via the webpage to confirm the before & after behavior:
Comment #6
damienmckennaWhen the code above is ran against the current -dev release, the following is the results:
After the patch is applied the output looks like this:
Comment #7
damienmckennaFYI you'll need to clear all sessions after applying the patch because the $_SESSION['cache_flush'] data structure changes.
Comment #8
damienmckennaWorking on the tests - they're not all passing yet, so there's some work to be done.
Comment #9
damienmckennaOne step closer. I noticed that the MemCacheRealWorldCase tests were failing because the 'language' table wasn't found, so I added 'locale' to the list of enabled modules.
Comment #10
damienmckennaDown to three fails in MemCacheClearCase and one in MemcacheLockFunctionalTest, while MemCacheGetMultipleUnitTest and MemCacheRealWorldCase pass all tests.
FYI the failure in MemcacheLockFunctionalTest is the old "Bootstrap failed in lockInit(), lock_acquire() is not available." problem.
Comment #11
damienmckennaHere's what "drush test-run MemCacheClearCase" has to say for itself:
Comment #12
damienmckennaComment #13
damienmckennaSome minor code formatting cleanup for the tests. Still just have the four failures.
Comment #14
damienmckennaI left in some debug print() statements in one of the tests, sorry about that.
Comment #15
damienmckennaReverted the unnecessary changes to the memcache6.test file.
Comment #16
pieterdcIssue #2335727: Setting $cache->created with msec precision borks page caching. changed the created timestamp to a second-precision version to align with the Drupal core database cache backend class, but:
- it uses to REQUEST_TIME (like it did pre-2011) which holds the start time of the request instead of the current time (like it did from 2011 to 2014)
- didn't adjust the time checks in MemCacheDrupal::valid() accordingly
That causes for example a cache entry set after a cache clear to be considered invalid if retrieved in the same request. That's a false cache miss.
Luckily this issue helps with that.
It's a pity there are so many test failures on the 7.x-1.x branch because that makes it hard to verify if any patch breaks existing functionality. Being worked on in #1863996: Fix all tests. But running CacheClearTest on my local, with the patch, I went down from 3 to 2 failures.
If we leave out the code cleanup (that adheres coding standards and improves readability) we're left with a patch that's easier to review.
If we then leave out the pieces from the general test fixing issue patch, the changes of this issue are even more obvious: see patch attached.
Comment #17
pieterdcWe ran a whole bunch of integration tests on our Drupal distribution and discovered failures were introduced by applying the patch from comment #15.
Mostly module installations that assign permissions to roles in a hook_enable() implementation. Clearing some caches did the trick to get the newly created permissions known even before the module installation was completed.
But with the aforementioned patch, we sometimes get stale (cache) data making the permissions unknown again.
So, the patch needs more work.
Comment #18
damienmckenna@PieterDC: are you talking about the problem with permissions exported via Features, e.g. #1549608: Cannot import exported Panelizer permissions using Features/defaultconfig if handler cache is stale?
Comment #19
pieterdcDamien, our permissions are not exported through Features, but yeah, we face the same PDOException. I created an internal low priority follow-up ticket to have a look at it. Thanks for the hint!
Comment #20
jeremy commented@PieterDC, did you ever track down the regressions? I'm interested in seeing this patch committed, if you can provide any more detail on how to duplicate the regressions that would be helpful.
Comment #21
pieterdc@Jeremy, I never got to tracking down the regressions.
Comment #22
damienmckennaJust to get a baseline of where we are, this is a reroll of the raw changes, it doesn't include the tests.
Comment #23
damienmckennaWas automated testing disabled for the 7.x-1.x branch?
Comment #24
jeremy commentedpatch doesn't apply
Comment #25
mrinalini9 commentedRerolled patch #22 for 7.x-1.x branch as it failed to apply, please review.
Comment #26
jeremy commentedThanks! I'll test this out once 7.x-1.7 is released (ideally to be part of 7.x-1.8).
Comment #27
badrange commentedHi @jeremy - looks like the patch didn't make it to 1.8, is there a chance for it to be released in 1.9? If this fix would improve caching it would save lots of cpu cycles and electricity..
Comment #28
moshe weitzman commentedLooks to me like this is done in latest version.
Comment #29
japerryClosing as Drupal 7 is no longer supported.