Problem with Drupal cache functions

Reviewing codes for cache functions in Drupal 5 core, I believe I have found some flaws in them.

Following is the section of cache_clear_all() in includes/cache.inc,

121: function cache_clear_all($cid = NULL, $table = NULL, $wildcard = FALSE) {
       // ... ... ...

129:   if (empty($cid)) {
130:     if (variable_get('cache_lifetime', 0)) {
131:       // We store the time in the current user's $user->cache variable which
132:       // will be saved into the sessions table by sess_write(). We then
133:       // simulate that the cache was flushed for this user by not returning
134:       // cached data that was cached before the timestamp.
135:       $user->cache = time();

137:       $cache_flush = variable_get('cache_flush', 0);
138:       if ($cache_flush == 0) {
139:         // This is the first request to clear the cache, start a timer.
140:         variable_set('cache_flush', time());
141:       }
142:       else if (time() > ($cache_flush + variable_get('cache_lifetime', 0))) {
143:         // Clear the cache for everyone, cache_flush_delay seconds have
144:         // passed since the first request to clear the cache.
145:         db_query("DELETE FROM {". $table. "} WHERE expire != %d AND expire < %d", CACHE_PERMANENT, time());
146:         variable_set('cache_flush', 0);
147:       }
148:     }
149:     else {
150:       // No minimum cache lifetime, flush all temporary cache entries now.
151:       db_query("DELETE FROM {". $table. "} WHERE expire != %d AND expire < %d", CACHE_PERMANENT, time());
152:     }
153:   }

         // ... ... ...
167: }

At line 146, variable_set('cache_flush', 0); cache_flush is set to zero. When cache_clear_all() runs next time, cache_flush is evaluated (to zero) at line 138 and the function has no chance to flush a cache table but to exit. Cache_clear_all() gets to flush cache tables once on every two runs because cache_flush switches values (zero and time()) back and forth.

This also affects cache_get(), its top portion of the code showing below,

13: function cache_get($key, $table = 'cache') {
14:   global $user;

16:   // Garbage collection necessary when enforcing a minimum cache lifetime
17:   $cache_flush = variable_get('cache_flush', 0);
18:   if ($cache_flush && ($cache_flush + variable_get('cache_lifetime', 0) <= time())) {
19:     // Reset the variable immediately to prevent a meltdown in heavy load situations.
20:     variable_set('cache_flush', 0);
21:     // Time to flush old cache data
22:     db_query("DELETE FROM {". $table ."} WHERE expire != %d AND expire <= %d", CACHE_PERMANENT, $cache_flush);
23:   }

25:   $cache = db_fetch_object(db_query("SELECT data, created, headers, expire FROM {". $table ."} WHERE cid = '%s'", $key));

      // ... ... ...
49: }

When cache_get() runs sometime after cache_clear_all() ran and set cache_flush to zero, $cache_flush is assigned zero at line 17 and fails to run "DELETE" query to clear expired caches and ends up returning expired value. This flaw makes cache_get() keep returning old cache values and failing to delete them.

I believe the code,

  variable_set('cache_flush', 0);

at line 20 and 146, should be fixed to,

  variable_set('cache_flush', time());

Furthermore,

Another problem is that 'cache_flush' variable is shared by different modes. When you run cache_clear_all() multiple times for different cache tables in succession, latter invocation would not run properly because first run would alter the value of 'cache_flush' and latter call for different cache table would not clear the cache table due to 'cache_flush' value indicating already being cleared.

My solution for this is having cache_flush variable for each cache table, such as cache_flush_cache, cache_flush_cache_page, etc.

There are some more modifications to make caching work if you look at the patch. Is seems to work okay on my site.

If you have similar caching problem, try the patch and tell us if it solves your problem.