Concurrency problem with variable caching leading to cache inconsistency

This is one of the race conditions that Drupal it is suffering from. I'm not sure there is any perfect solution.

This would solve the problem, but I fear the cost of this is huge (locking both the whole cache table and the variables table???). Could you show some performance analysis?

Bugs like this one should be corrected first in the current HEAD version (7.x), than back-ported.

I suggested a patch implementing soft locking for that type of operation in #251792.

We have lock API in core now but I don't know that this solved in d7. I do think it is solved in latest pressflow for d6.

FYI, here is the Pressflow work in this area: http://bazaar.launchpad.net/~pressflow/pressflow/6/annotate/head:/includ.... note variable_init() locking and variable_cache_rebuild() has deferred db write

I just discovered a problem with the PF patch for this issue under very heavy load: in such a situation, if a slow (or even failing) page is the one holding the variable regeneration lock, the attempt to lock for cache generation can cause every page to wait for the full duration until rebuild success of up to 30s in case of forced lock release, causing requests to pile up and overall throughput to decrease massively until one request finally succeeds in acquiring the lock and building the cache entry at the expiration of the delay.

I looked at the equivalent D7 implementation and, although different - notably because is uses the specific cache_bootstrap bin instead of the default bin - it should suffer from the same problem: if the lock is taken, every page will be waiting during rebuild and, if for any reason that rebuild fails, the whole site will be crawling to a halt for 30 seconds.

The current logic is like this (abstracting the maintenance case):

Pressflow:

if (cache_get succeeds) {
  $variables = cached data;
}
else {
  if (lock_aquire(somekey, 30 seconds)) {
    build $variables from db_query
    cache_set($variables)
    lock_release
  }
  else {
    lock_wait
    $variables = variable_init unless recursion too deep or array() otherwise
  }
  
return $variables;
}

Drupal 7 (swapped the "if" branches for easier comparison):

if (cache_get succeeds) {
  $variables = cached data;
}
else {
  if (lock_aquire(somekey, 1 second)) {
    build $variables from db query
    cache_set($variables)
    lock_release
  }
  else {
    lock_wait
    $variables = variable_initialize without recursion limit
  }
return $variables;
}

It seems that since the point here is to build a *cache* and not a stable reference, if would make better sense not to try to build the cache if the lock can not be taken, and just proceed from there by querying the DB, just like normally happens on any case of cache unavailability. It could go like this:

if (cache_get succeeds) {
  $variables = cached data;
}
else {
  build $variables from db query
  if (lock = lock_aquire(somekey, 1 second)) {
    cache_set($variables)
    lock_release
  }    
}
return $variables;
}

That way, if the lock is not acquired, the cache is not initialized with the possibly invalid data concurrency-wise (as to the topic of the thread), and all page requests can still be served from the DB while the cache is being built, only incurring the extra cost of the db query (liable to be in the mysql cache quite often anyway), instead of waiting for the cache reconstruction, which can be long if the page holding the lock has some problem like being aborted of even just being slow because it happens perchance to be a heavy one.

Operations modifying the variables and clearing the variable cache for this, (vset/vdel) are not affected, be they immediate as in D7 or registered for shutdown as in PF.

(on a related note, not sure why D7 performs no check on recursion depth ?)

I fail to appreciate how an issue worked on since 2008 can be a duplicate of one opened in 11/2010, but nevermind.