Overzealous locking in variable_initialize()

As an alternative strategy, we could also just load the whole variable table while a cache is being rebuilt.

Implementation of #2.

RE point 1: The require_once and lock_init in variable_set.
I agree that the lock_init looks weird, but more importantly introducing a require_once into variable_set may kill performance on servers that don't have the apc require_once override enabled. This will introduce _alot_ of realpath calls to a very hot path to find the true path of the lock file, no?

subscribe

subscribing

Subscribing.

I found a logic error that would make threads wait even if the lock was acquired:

+++ includes/bootstrap.inc	3 Dec 2010 16:26:05 -0000
@@ -803,12 +794,37 @@ function variable_get($name, $default = 
+  if (!$lock_acquired = lock_acquire($lock_name, 1) && $variable_write) {
+    // When we've made a change via variable_set() or variable_del(), we try
+    // harder to get the lock.

This is an operator precedence bug reminiscent of #802446: Cache stampede protection: drupal_render() and page cache, which evaluates as $lock_acquired = (lock_acquire() && $variable_write). Obviously not what is intended :)

Rerolled to fix that and a style issue: "else if" should be "elseif".

Otherwise the method and implementation look sound. This patch may have real performance consequences though, since the overhead for variable_set() is much more. But the trade-off is better for concurrent reads after a set, as the OP makes clear.

Also, from the OP:

other requests to the site while this is going on don't know anything about it - they'll always get a cache hit regardless.

The patch keeps this contract pretty well. The main improvement is cache_set() replacing a previous cache_set() with newer data, instead of removing it entirely. cache_clear_all() is left as a last resort during concurrent writes.

 function install_drupal($settings = array()) {
   global $install_state;
+
+  // Initialize the locking system.
+  require_once DRUPAL_ROOT . '/includes/lock.inc';
+  lock_initialize();

Why is this code in install_drupal()?

I think it should be in install_begin_request(), somewhere closer to where the rest of the files are loaded and the rest of the initialization is performed.

@David wouldn't putting lock_init in install_begin_request() be less friendly to systems that use install_drupal() as an API, such as drush?

@carlos8f: install_drupal() calls install_begin_request(), so everyone will get it. It's an issue of code organization, really - all the other systems are loaded/initialized from there, so why should this one be different?

Oh, it would've helped to look at the code :P

Yes, variable_initialize() moving to install_begin_request() makes a lot of sense. lock.inc as a pluggable system should be loaded just after DRUPAL_BOOTSTRAP_CONFIGURATION, ideally.

Rerolled to move lock initialization to install_begin_request().

this is very easy to reproduce with Jeremy's debug patches above.

I think you mean Jeremy's debug patches in #886488: [D7] Add stampede protection for css and js aggregation? :)

We have these three performance/stampede-protection patches left in the queue:

#802446: Cache stampede protection: drupal_render() and page cache
#886488: [D7] Add stampede protection for css and js aggregation
#973436: Overzealous locking in variable_initialize()

Although they look good from a logic and code review standpoint, I am a little nervous at the prospect of these getting committed just before release, and having very little real-life testing to indicate that they actually improve concurrency and minimize load. I emphasize real-life because it's hard to test the organic nature of traffic and concurrency in a controlled test. If we can conduct a test either on a real traffic-heavy site, or in a controlled test that can test for locking/multiple threads across multiple URLs, I would be more confident. Any ideas on how we could go about that testing?

#31: 973436-31-variable-cache-rebuild.patch queued for re-testing.

+++ includes/bootstrap.inc	5 Dec 2010 00:27:17 -0000
@@ -725,43 +725,6 @@ function drupal_get_filename($type, $nam
@@ -786,6 +749,34 @@ function variable_get($name, $default = 

@@ -786,6 +749,34 @@ function variable_get($name, $default = 
+function variable_initialize($conf = array()) {

I agree that this function should be moved closer to the corresponding functions, but why is _initialize() moved below _get()?

+++ includes/bootstrap.inc	5 Dec 2010 00:27:17 -0000
@@ -803,12 +794,37 @@ function variable_get($name, $default = 
+ * @param bool $variable_write

Missing phpDoc description.

+++ includes/bootstrap.inc	5 Dec 2010 00:27:17 -0000
@@ -803,12 +794,37 @@ function variable_get($name, $default = 
+function variable_reset_cache($variable_write = TRUE) {

Why is this added below _set()? Should be directly below _initialize().

Powered by Dreditor.

Sorry, please scratch the last of the review issues (removed it), I'm tired.

+++ includes/bootstrap.inc	15 Dec 2010 16:16:44 -0000
@@ -762,6 +753,34 @@ function variable_initialize($conf = arr
+ * @param bool $variable_write
+ *   Flag to indicate if we are resetting the cache after a write operation.

Without reading the actual code, it wasn't entirely clear to me what the effect of this parameter is.

Also, the default value should be documented, and I especially wondered why it's TRUE and not FALSE by default, since many other *_reset() functions in core solely flush a cache - they do not write the cache.

Powered by Dreditor.

Further analysis at #249185: Concurrency problem with variable caching leading to cache inconsistency, of which this issue appears to be a duplicate.

Here's a first run at a D6 port.

I was uncertain how best to apply the changes in the install_begin_request() section of #41, so they are not yet included.

Tested with basic drush vset / vget / vdel, and simple drush cron runs.

----
Edited to correct comment number reference. Changed "#42" to "#41".

The patch in #41 changes install_begin_request() to initialize the locking system before the database system is loaded. But the locking system depends on the database system, because lock_acquire() calls db_insert(). Can we guarantee that this will not result in PHP Fatal error: Call to undefined function db_insert()?

subscribe

subscribe

This will need to be patched in D8 first and then back-ported to both D7 and D6 after it is integrated into D8. Hence, moving adjusting the version and issue tags.

As I read through this issue, I was wondering whether for performance issues it might make sense to allow variable_set and variable_del functions to be called with a second format.

The second format would be a keyed associative array where the key was the variable to be set and the value would be what it should be set to. In both functions then we could use an is_array() check to silently handle strings as they are now or to set an array of values in one go. This would save the relatively expensive cache rebuild until most, if not all, of the variables are set for a module/entity/plug-in etc.

This seems like relatively low hanging fruit from performance and DX perspective. Does this idea make sense?

Meant to change the version number too!

This will need to be patched in D8 first and then back-ported to both D7 and D6 after it is integrated into D8.

I don't understand the reasoning for this. Is this Drupal policy? In my view, fixing it in the current version is a higher priority than fixing it in a future version.

It's a pretty serious bug for anyone who wants to run cron frequently with a lot of processing and can bring their server to a halt until they figure out the cause and build a custom cron.

@kentr -- Yes, this is Drupal policy. If you want to discuss the policy, please read all of the following thread first: #1050616: Figure out backport workflow from Drupal 8 to Drupal 7

@pillarsdotnet: Thanks. I will read that thread.

Independent of the backport policy, I believe this bug to be critical. Inaccurate variable values can wreak all sorts of havoc.

What if instead of a full variable cache reset each time a variable was set, we set a &drupal_static() flag and have the cache reset once the page is done rendering....

variable_set('mystuff45', 19);
variable_set('mystuf545f', 12);
variable_set('mystuff54', 50);
variable_set('mystuff534', 72);
variable_set('mystuff2', 24);
variable_del('mystuff2');
variable_set('mystuff3', 66);

Do we really need to reset the cache seven times here?

Subscribing

Tagging issues not yet using summary template.

A programmer with 20 years experience recently told me:

locking is like a toilet - only one person at a time can do a sh!t.

Patch included for pressflow-6.22.104 (may need one small change for D6).

Patch implements:
* read-modify-write concept.
* write {b,}locking for variable_{set,del}().
* variable_init() is called just before modifying $conf[].
* checks if db_insert() is defined.
* modify lock_wait() to sleep _after_ checking lock, not before, and to sleep 250 mS not 1000 mS.
* does _not_ implement read locking (that needs to be implemented at the module level for time-heavy processing, and use lock_wait() & lock_acquire() - the problem is _not_ with variable_get()).

And yes. I made a mistake in the patch! But while I was at it, I found / resolved 2 more "problems".

Updated for:

* remove function_exists() - that was not causing the warning. db_result() being called from lock_acquire() on a disappeared lock was causing the problem. "drush @site cc all" is one case to trigger the following:

'all' cache was cleared                                                                        [success]
WD php: Warning: mysqli_num_rows() expects parameter 1 to be mysqli_result, boolean given in   [error]
db_result() (line 200 of
/var/aegir/platforms/test-pressflow-6.22.104/includes/database.mysqli.inc).

* fix lock_acquire() to check db_query() with db_affected_rows(), instead of db_result().
* rewrote cache_get() since it was if-nesting hell. Unreadable core code is never good.
* cache_get() now supports checking "expire" on an individual basis, providing cache_lifetime is set (which appears to be designed as a way to globally enable never-expire caching). cache->expire was only ever used to check if CACHE_PERMANENT or not. Otherwise cache_lifetime was used as the "expire" time as part of deleting expired cache entries.

@dsobon, Pressflow patches should be submitted to https://github.com/pressflow/6. This is a D8 issue now :)

#41: 973436.variable.patch queued for re-testing.

Re-roll of #41 for D7 and D8.

The pressflow patch does not translate well to vanilla D6, so I'll leave that bit to someone else.

Let's try that D7 patch again...

Note that the Drupal 8.x patch will need to be rerolled, because the core directory structure for Drupal 8 has now changed. (For more information, see #22336: Move all core Drupal files under a /core folder to improve usability and upgrades). When the patch has been rerolled, please set the issue back to "Needs Review."

Tagging as novice for the task of rerolling the Drupal 8.x patch.

If you need help rerolling this patch, you can come to core office hours or ask in #drupal-gitsupport on IRC.

Here's a re-roll of the 8.x patch in #62 for the /core move.

Fixing whitespace issues in #65

#66: variable_set-973436-67.patch queued for re-testing.

+++ b/core/includes/install.core.incundefined
@@ -240,6 +240,9 @@ function install_begin_request(&$install_state) {
+  // Initialize the locking system.
+  require_once DRUPAL_ROOT . '/' . variable_get('lock_inc', 'includes/lock.inc');

Need to correct this for core/.

Re-roll which fixes the issue identified by @xjm in #70.

Had my "/"s in the wrong place.

Refer #58 http://drupal.org/node/973436#comment-5125648

I appreciate the work / code offered by dsobon @ #58

I have applied dsobon's code to Pressflow 6.22 - 104,
and it works for a solution to this issue. Pressflow lags somewhat on the Support
+ Google scale, so I am *grateful* to find the solution here; as will others.

As is mentioned in both these related articles;
See: http://drupal.org/node/987768 and http://drupal.org/node/973436 (this page)
this is a problem for D6/D7 and D8. (that logically means Pressflow too)

What exactly will this mean on the persistence level?
What will be stored in the database?
In D6, there is the variables table with one row per serialized variable, and then the cache table, with one row for the serialized contents of the variables table altogether. The cache thing can only be loaded all-at-once, while from the variables table we can load the variables one-at-a-time, or at least with a big SELECT and then fetched row-by-row. The same for write operations.

How are we going to change this?
Will there be a cache that caches only a part of the variables table?

(I hope the summary will tell)

@catch (#78),
so this means our only "cache" for variables in the db is still the big thing that stores all variables in one big serialized nested array?
How does this help us then? We still load this big thing on every request, in variable_initialize() ?

In variable_get():

  $cache = &drupal_static('variables');
  if (isset($cache) && isset($cache[$name])) {
    $conf[$name] = $cache[$name];
    return $conf[$name];
  }

Is that really necessary? In variable_initialize() all content of the cache object is copied into $conf, and its internals have been emptied. It should never pass the isset($cache[$name]) here; Or am I missing something?

In variable_set() (and also variable_set()):

  $cache = &drupal_static('variables');
  if (is_object($cache)) {
    // Write through to the variable cache.
    $replace = array();
    if ($cached = cache('bootstrap')->get('variables')) {
      $cached->data[$name] = $value;
      cache('bootstrap')->set('variables', $cached->data);
      $replace = $cached->data;
    }
    $cache->replaceStorage($replace);
  }
  else {
    cache('bootstrap')->delete('variables');
  }

When does the $cache variable could not be initialized? If anything is using variable_set() in pre-boostrap, it's a real software bug, it should not happen. So my guess is that this is_object() test should be removed.

To ensure a correct behavior, we could use a new function such as _variable_get_cache() instead of direct call to drupal_static() that would create on the fly the new instance instead, maybe? In all cases, the object is created at bootstrap anyway.

My guess is that a empty() method could be done over the cache object instead of using cache('something')->delete('something'): this has a huge cons: the cid and bin appears only once in the full code, where you instanciate the object itself.

We copy the variables that are known about by DrupalCacheArray into $conf (since there is still code around in contrib that directly mucks about with $GLOBALS['conf'] to tweak things during requests, so we can't actually replace that with the object safely).

I understood that, it's OK it keeps backward compat.

However DrupalCacheArray won't know about variables it hasn't seen yet, hence the fallback which will only happen on a cache miss.

I see this use cases here, tell me if I'm missing one:
I call variable_get() and I get a variable into $conf...

• ... and this variable comes from an override, so it's OK that the cache object doesn't know about it!
• ... and this variable comes from the cache object, so it has been overrided at bootstrap time thanks to variable_initialize() and it's already into $conf
• ... and this variable does not exists in either, but no need to check the $cache since it already gave all it's got.
• ... and the variable has been modified or set just before using variable_set(), but variable_set() already updated both $conf and the object.
• ... and the variable was into $conf but someone modified the global by hand: case in which we don't want to call the cache object.

So the isset() call will never go to TRUE, except if someone modified the object internals without going through variable_set(), which is a real bug out there, not in the variable system.

EDIT: Ho I understood, someone may have set a variable in a parallel thread: considering the fact that the PHP has not a long run, does it really worth the shot of reading it? Or am I still missing something in the DrupalCacheArray resolve/persist algorithm.

The DrupalCacheArray seems to suffer many problems, but I might be wrong here, see this:
It seems to be the extending class or the component who use it to have the responsability of calling the persist() method, which happens with the VariableCache::resolveCacheMiss() and VariableCache::replaceStorage() implementations: those two calls can be used in parallel threads, thus one saving its set of persisting keys as one big cache entry, the second one overrides and does the same, but they may have used totally different keys (and may have not loaded the cache from the other). This will bump to the next hits that may do exactly the same because not all variables has been saved (the second save will have overriden the first), and those after, etc... until we have luck and one and only one thread happens to run with ALL the useful variables being used.

Yes, you are right! That was were I was wrong, indeed. That's OK I guess then. Still my notes about the _set() and _del() functions remains.

OK, didn't see that bit, nice.

The patch in #92 fixes the bug from #1536262: Entering a site name when installing Drupal 8 has no effect until caches are cleared, so I closed that, and bumped this. We should add a test case here for the site title as well.

Also I think the patch in #94 was missing that new file again.

Re-rolled against HEAD. Improved some documentation.

Also adjusted the replaceStorage() method, as it didn't persist the new/replaced storage keys. When replacing the existing storage (which was persisted) I think the new/replaced storage should be fully persisted, too. Doesn't really matter for the implementation code in this patch, as it replaces with an empty array.

I'm entirely not happy about the

drupal_static_reset('variables');

that seems to be required for the batch test.

+++ b/core/includes/bootstrap.inc
@@ -874,9 +867,22 @@ function variable_set($name, $value) {
+    // Write through to the variable cache.
+    $replace = array();
+    if ($cached = cache('bootstrap')->get('variables')) {
+      $cached->data[$name] = $value;
+      cache('bootstrap')->set('variables', $cached->data);
+      $replace = $cached->data;
+    }
+    $cache->replaceStorage($replace);

Why don't we call into VariableCache::__destruct() (or a public VariableCache::set()) instead of this manual fiddling?

I'm actually not sure whether I fully understand the envisioned concept behind this patch, so here's one more version incorporating the suggestion in #98 and reverting the replaceStorage() change from #97. I don't have the impression I'm contributing something useful here, so going to step out of this issue for now.

The replaceStorage() idea in variable_initialize() really needs much more elaborate documentation on why that is done. The idea looks fairly complex and special to me and I wonder whether it is not going to result in unexpected issues down the line.

EDIT: It also looks like the lock validation in variable_initialize() on read is lost with this patch.

Moving back to a task based on discussion in #1536262: Entering a site name when installing Drupal 8 has no effect until caches are cleared.

#1816424: Performance patch: fast cache variables invalidation leads to lock_wait slowdown was marked as duplicate of this.

Maybe I get to work on that :)

@Fabianx still interested?

Does it still make any sense in the wake of CMI ?

I think the D8 equivalent task for this is #1880766: Preload configuration objects.

Updating summary.

With everyone focused on the D8 beta, issues like this have fallen to the side of the road. I'm excited by the D8 work being done in #2228261: Add a local, PhpStorage-based cache backend and #2248767: Use fast, local cache back-end (APCu, if available) for low-write caches (bootstrap, discovery, and config), but before I can even dream of those being back-ported to D7, it seems we need this issue committed first. I'm wondering if anyone has made any advancements on this front but has yet to post about it.

I'm reassigning to none since there has been no activity for two years.

A reroll of #61, which was the last D7 version of the patch.

No, sorry, this is the patch file.

Whoops, there was a syntax error in the patch. Fixed.

RTBC - Looks great to me.

I am not sure what we wanted to test here, maybe Stefan or David will know.

Not sure yet: does it cover the case outlined in https://www.drupal.org/node/249185#comment-3312162 under heavy load ? It seems it tries to, but not sure it succeeds.

Yes, that case is covered.

It tries to acquire the lock, but unless writing variables it never waits.

Seconding Fabianx's request in #115 for feedback on what tests might be useful to confirm this doesn't cause regressions. Thanks.

The patch in #113 misses the partial querying of the variable table that was added early in D8 in #987768: [PP-1] Optimize variable caching, so if you do multiple variable_sets in one page request you are going to do multiple rebuilds. This is as noted in #53, and catch addressed that concern in #54 by essentially saying that the work in #987768 was done, which it isn't in D7, right?
(I appreciate that the variable system has gone away in D8)

So, is that a real issue, or is it okay that page requests might rebuild the variable cache over and over, albeit in a non-blocking way for other requests.

#120: While what catch has written is true, there is one 'catch' to it (pun intended):

- Writes are way slower than reads usually and read-before write is generally cheap

So the only thing we save is the 'read', but we increase conflicts that way (e.g. ping-pong play of two or more threads).

Therefore I stick to this being RTBC - even if we don't use the VariableCache().

Though we might still consider to use the CacheArray (cache collector) approach for variables - for the reason that it means that only active variables have to be loaded from the cache and for several sites, variable table size is so BIG, but e.g. only 30% is actively used on most page requests.

I still think #113 is a pretty safe compromise to improve it more without radically changing it.

Please don't CacheArray the variables, even if only 30% are in use on each page, it's just too critical during runtime to setup anything that would be more complex than it currently is. Current variable handling is fine, and we don't have cache size problems with it (as long as you don't use 200 modules).

EDIT: +1 for catch

This is base system and 7.60 target and hence an issue for David to look at.

I am not seeing the need for further tests though.

It took me a while to get to this, but I did some testing now and as #120 said this really does slow down variable_set() a lot.

The difference is somewhat noticeable when submitting a form like the one on admin/config/people/accounts that does 30+ variable_set() calls. And that's with a default core install (with only around 60-70 variables in the table); I haven't tested but I wonder if the slowdown would be a lot worse on a site that has many variables?

So if the tradeoff is between slowing down an admin operation that lots of sites do vs. solving an end-user performance bottleneck that happens only on extremely high traffic sites, it is not clear to me how to weigh that tradeoff.

I got to thinking, though, why does this patch even need to make changes to variable_set() and variable_del() at all? The performance bottleneck is due to the lock_wait() during variable initialization, so why not just fix that? Then I noticed that that's exactly what Damien Tournoud's original patch in #3 did :) #4 says there would still be a "stampede" with that patch but I'm not sure what that refers to since it is designed to avoid a stampede on the cache writes... is the stampede just that many processes will try to call lock_acquire() and read directly from the {variable} table simultaneously? Isn't that pretty minor?

The patch in #113 has benefits and drawbacks, but the patch in #3 seems like it only has benefits compared to the current code in core - it gets rid of the pointless lock_wait() and that's it. So why not go with #3?

Marking #1595070: Consider removing the cache in variable_initialize() as related.

It seems the "Proposed solution" in the issue summary talks only about Drupal 8.
Could someone who followed this issue update the issue summary and add a section "Proposed solution(s) for Drupal 7" ?

I'm seeing deadlocks on the variable_reset_cache key in semaphore in with #113 applied. Going to try #3 for a while.

Lots of deadlocks on semaphore on D8.4 too, even on single web head, with as little as 3 simultaneous clients (looping Drush commands).

Hi All,

Our environment is on 7.59 with memcache_storage module (7.x-1.4), we have been frequently getting Lock wait time out errors.

Patch https://www.drupal.org/files/issues/drupal-n973436-113.patch in #113 seems already there in 7.59 and this issue is tagged to 7.60 release.

Could some one help me what else is planned on 7.60 release to fix the issue.

Below is full stack trace of error.

Error message
PDOException: Uncaught exception 'PDOException' with message 'SQLSTATE[HY000]: General error: 1205 Lock wait timeout exceeded; try restarting transaction' in /var/www/html/includes/database/database.inc:2227

Stack trace
in PDOStatement::execute called at /var/www/html/includes/database/database.inc (2227)
in DatabaseStatementBase::execute called at /var/www/html/includes/database/database.inc (697)
in DatabaseConnection::query called at /var/www/html/includes/database/query.inc (1177)
in UpdateQuery::execute called at /var/www/html/includes/database/query.inc (1643)
in MergeQuery::execute called at /var/www/html/sites/all/modules/contrib/memcache_storage/memcache_storage.inc (321)
in MemcacheStorage::variableSet called at /var/www/html/sites/all/modules/contrib/memcache_storage/memcache_storage.inc (148)
in MemcacheStorage::clear called at /var/www/html/includes/cache.inc (173)
in cache_clear_all called at /var/www/html/includes/theme.inc (354)
in drupal_theme_rebuild called at /var/www/html/includes/common.inc (7664)
in drupal_flush_all_caches called at /var/www/html/modules/system/system.admin.inc (1781)
in system_clear_cache_submit called at /var/www/html/includes/form.inc (1520)
in form_execute_handlers called at /var/www/html/includes/form.inc (904)
in drupal_process_form called at /var/www/html/includes/form.inc (386)
in drupal_build_form called at /var/www/html/includes/form.inc (131)
in drupal_get_form called at ? (?)
in call_user_func_array called at /var/www/html/includes/menu.inc (527)
in menu_execute_active_handler called at /var/www/html/index.php (21)

Bumping to 7.61. This didn't make it into 7.60.

Actually, I'm going to disagree with this patch, I don't think variable_set should rebuild the variable cache, that would mean the variable cache would be rebuild multiple times if you had to set several variables in a row.
Maybe someone else can explain how this would improve performance? Seems like it would possibly degrade performance.

After re-reading through again this entire thread, points brought up by DavidRothstein at #125 and reports by JoelPittet #128 and fgm #129 , patch #3 provides the best solution and has no reported regressions. Comment #130 is erroneous, patch 113 is NOT in 7.59.

Patch #3 works without causing deadlocks on the semaphore table.

Go with patch #3

patch #3 needed a reroll.

creds to the developer who wrote patch #3 , as this is code verbatim

***EDITTED*** see next comment

deleted my own comment for now.

Lets get this in dev asap, this issue just cost me another weekend (thanks to the patch I have some of my sunday left), I stumbled on this again for a client that had an unpatched environment, this could have been resolved 10 years ago thanks to Damien Tournauds patch, to avoid repeating myself please see see comment #135 and also #136 fixes stale variable_get cache that seems to occur when variable_get is hit within 1 second of another request pulling the same variable_get. Patch resolves stale cache even for simultaneous request.

RTBC +1

RTBC +1

Would be good to get this committed - thank-you! +1 RBTC

@FabianX seeing as you already approved this patch 4 years ago can you please slam dunk this one for us? I have mentioned this issue to
@McDruid however he is currently very busy with the db api changes revolving around MySQL 8.

#973436-115: Overzealous locking in variable_initialize()

Can this change have an opt-in/opt-out variable that could be set in $conf in settings.php?

@mcdruid, thanks for having a look, I checked and D8 /D9 does not have an opt-in/opt-out variable for settings get.

The fix for this bug ensures that the variable_get returns the correct value instead of the stale cached value. The cache for the value is rebuilt when the value is updated.

I realize that variable_get doesn't exist as it was in D7 however settings get exists in D9. The fix was hopefully brought to settings get also. With that said, D8/D9 does not have an opt-in / opt-out for stale cached settings values.

@mcdruid, (@catch, @FabianX) and others have already agreed that this is a good fix (change) to make.

@catch recommends that once we put this in we can re-open this one:
#987768: [PP-1] Optimize variable caching

as for an opt-in, I have a half dozen client sites that need this fix I would not want to have to 'opt-in' to get the fix.

The performance penalty here is very small as a tradeoff for accuracy.

Again, I've spent a lot of time debugging multiple different sites in different scenarios where I discovered this patch was needed. 2 years after I found another site that needs this fix. There's already 148 comments on this issue and very little pushback and no reported regressions.

@mcdruid, if an opt-in option is what it takes to finally get this in, then yes I can do that. Is this what you recommend? if so, let me know I'll roll a patch for this.

I haven't had a chance to discuss this with @Fabianx yet, but I am starting to think that we should consider an opt-out variable for any changes that could possibly have adverse effects on some sites.

The change in #3 / #136 does look mostly harmless to me (and @David_Rothstein's approval in #125 is reassuring).

However participation in discussion of D7 issues on d.o (or lack thereof) is such that I don't want to assume a few people saying "this works on tens of sites for me" means something is safe for the (we think) hundreds of thousands of sites that are a "silent majority". Giving those sites an escape hatch if they are negatively impacted by a change in core seems a good idea to me. Others may have a different point of view, which I'm happy to hear.

So yes, I'd like to see this change have a simple opt-out option if it doesn't make the code horribly complicated.

@mcdruid, I understand your hesitation on this as a lot of weight on your shoulders but I had another look at this patch just now (patch 136) and it seems that we're actually simplifying the code quite a bit. I just cannot see how this change (improvement imho) could cause a regression. The code we're to replace is problematic and I just cannot see a need for it.

variable_get in this case just needs to get a value, and the code change simplifies the logic for this ensuring it's not getting a stale value.

I'm hoping that @FabianX can re-affirm what he stated in 121 is also true for patch 136.

So for now I'll hold off refactoring for an 'opt-in' with the hopes that @FabianX can review.

After 10 years no one here has reported a single regression. The empirical evidence is very strong and there is extensive test coverage exists for variable_get already included in core.

Thanks,

As a quick followup, I just did a grep in the 7.x branch , there's several pages of results of variable_get being used in our automated tests. We've got good coverage , the fix here is for concurrency which not even D9 tests are currently covering so I wouldn't expect us to have to write concurrency tests for this (although it would be nice).

grep variable_get * -R| grep test

My confidence level in patch 136 is extremely high, 99.9999%

with that said, I'm hoping that @FabianX can weigh in soon.

@joseph.olstad confidence burn into flame as soon as you experience weird database concurrency problems :) I know what I'm talking about, we actually did write 2 different versions of variable_get, variable_set and variable_initialize patches for different projects, depending on how the database behave, and how the site was being used. I think there's not global win-win solution for this problem. None of those patches did work for the project they weren't written for, but all did solve real life database concurrency or performance problems.

@pounard, the last time you reviewed this issue was 9 years ago, have you reviewed the newest patch (#136) ? It is vastly simplified (improved) logic.

I am not a fan of locking with "infinite patience"; looking at this has given me flashbacks to #2406863-146: stampede protection should be flexible ("Cache rebuild lock hit" watchdog message). There are definitely cases where waiting for a lock can be worse for performance than just getting on with it (which often means rebuilding a cache).

I really like the approach in #3 (re-rolled in #136) and it looks to me like in the vast majority of cases it would be an improvement.

However, I can think of some edge cases where it may cause problems. For example (this may seem a bit convoluted / silly but hopefully illustrates the point) imagine a site is (ab)using a variable as a counter. Bad practice, but we've all seen similar / worse things I'm sure.

Without the locking to rebuild the cache I think it's possible that a site will sometimes have a thread which is in the process of incrementing that counter (but has not yet committed the change to the db), while another thread gets a cache miss, loads variables straight from the db and begins the process of incrementing the counter itself, but starting from a lower value than should be the case. Throw the "cache consistent" problem into the mix (whereby a site might be using e.g. memcache which is unaware of database transactions) and I think it's definitely plausible that this change could exacerbate - if not cause - race conditions on some sites.

So I'm inclined to say we should only make this change if we provide an opt-out for any sites on which it causes problems.

Here's a patch which I think does that in a fairly lightweight way.

I'm not convinced about the comment / explanation in default.settings.php yet, but I'm more concerned about reviewing the logic initially.

In a very unscientific test using ab while running a variable_set / cache clear in a loop to try and roughly simulate some churn in the variables / caches, the patch was very marginally faster without the opt-out set. In other words, re-introducing the locking made Drupal slower.

Would appreciate any reviews of the logic especially.

@mcdruid, funny you mention (ab)using a variable_get variable as a counter, this is precisely what I've seen clients do (frequently) (and even lots of experienced long time site builders that are still active did) and 136 is the fix for that. In fact, this is the very reason why my client needs this fix, it was easier to patch core variable_get than it was to rewrite their custom code with an autoincrementer field from the db.

And I was looking at refactoring their code but it was far too much work. I am guessing that lots of shops have done, I've patched core and put a monitoring service on to see if there were any concurrency incidents since then, my monitoring software (a cron job checks every 2 hours for bad data , if found sends me and my client an email warning), has not found a single glitch since patching.

I have a cron job monitoring the server that had the issue and since putting in patch 136 the issue has not resurfaced. Yet they still handle high loads. Prior to the patch 136 they would have concurrency issues once or twice per month where a stale value was returned, since then it has been never a stale value always the correct value. Not one single incident as long as 136 is applied (patch 3 reroll).

the concurrency test for this is have to be within 1000 milliseconds, two requests for the same variable, one request sets and gets the variable, the next sets it and gets it also, without patching the second request incorrectly gets the stale cached value of the first request.

With patch 136 (reroll of 3), the second request gets the correct value (not the stale value), an incremented value as expected.

you can run more concurrency tests by scripting additional concurrent requests that increment and get the variable to prove that patch 136 works.

so instead of 2 concurrent requests, make 10 or 20. Patch 136 will do a lot better than without.

I think if I have time I will try to set up a test case scenario for you to PROVE without a doubt that patch 136 is FAR AND AWAY superior to variable_get that is currently in core.

@joseph.olstad I have to agree that #136 seems much simpler, and avoid the terrible wait recursive loop, which is far better in my opinion, it'll work fine (much better than actual code anyway) on all sites that don't use variables for dynamic data.

ok here is the code that exposes the stale cached values when hit twice within 1000 milliseconds from two distinct requests.

function mycompany_next_case() {
  $case_no = variable_get('mycompany_next_case', 10001);
  variable_set('mycompany_next_case', $case_no + 1);

  return $case_no;
}

without patch, stale cached value is returned on second request within 1000 ms of the first request.

WITH patch 136 the correct value is returned

This is a very straight forward and simple use case, very easy to prove that core behavior is undocumented (except for here) and unexpected.

The expected and correct behavior is reliable only by applying patch 136

Without a patch you may get the correct value, other times you may not.

With patch 136 we always get the expected value.

Does this help clarify why patch 136 is needed? This is also a code simplification and an elegant one.

Unless I'm missing something, #156 does the same thing as #136 unless this is in settings.php

$conf['variable_initialize_wait_for_lock'] = TRUE;

There's the additional cost (compared to #136) of one function call in the event of a cache miss + a failure to get the lock.

The advantage is that any sites which encounter a problems because of the behaviour change have an easy way to opt out.

Have I got the logic wrong, or is the argument against the added complexity of the opt out?

I haven't tested the opt-out patch, I just cannot see a reason for opting out of this fix.
I'm on the phone with the client that was affected by this and they are wondering why this would not get fixed?

***EDIT***
This caused my client huge amounts of grief and several people were notified of the issue it led to a serious security and privacy issue as two people got the same id and the information cross contaminated. Once the issue became known, we found out this spanned several months if not years unnoticed.
***EDIT***

Its quite demoralizing to have to fix the issue several times on several servers over a span of several years and realize that you forgot to apply the patch.

@mcdruid , ya the opt-out seems like added complexity to me. I'll maybe take a stab at writing an automated test to prove the 136 fix using the code I provided in #160.

I am wondering if a simpletest just calling that function 3 or 4 times in a row will prove the failure or if it really has to be a seperate request doing so... maybe this weekend I'll run a few more tests to see if I can write a fails test and prove without any doubt the need for 136.

Tests only patch.

another one, hoping both of these fail.

add patch

tests only is also the interdiff

patch.

try again

Sticking with patch 136 , ignore 172

It's a 10 years old bug, it needs to be fixed, if @joseph.olstad patch works and proves having no side effects, I'm in favor of commiting it. There is no drupal dev in the world maintaining complex sites on the long run that never experienced problems with variables. 10 years reapplying patches every upgrade is really a maintenance burden we all want to avoid.

Just to clarify, 136 is basically a straight up reroll of Damien Tournouds patch 3 from 10 years ago, it's brilliant. I changed nothing except for rerolling it. It has stood up the test of time.

As for opt-in, opt-out, one of my clients has over 180 Drupal 7 sites and I have several other clients with lots of sites. Opting-in is a lot of tedious work in this case. Sometimes I go back to maintain sites that have been worked on by others and not correctly patched. Would prefer the one fix for all.

@mcdruid / @joseph.olstad The variable system isn't a Software Transactional Memory system. There's a giant race condition baked in that I think comments #156 to #173 that dance around. None of these patches solve that race condition, at all (and that's fine):

1. Thread A and B load variables, either from the cache or from the variables table. They both have variable: 'foo' == 123.
2. Both do some work, then thread A increments the variable and calls variable set: 'foo' = 124 in the DB
3. Thread B then completes its work and then increments the variable and sets it, but because it loaded the original value of 'foo' as 123, it also sets 'foo' = 124 in the DB.

If we're trying to count something, then ideally we'd have ended up with 'foo' == 125, but it's 124 in the DB.

db_next_id() exists if you need a source of strictly increasing integers. If you want to count something, then 'UPDATE table SET col = col + 1' is going to be thread safe on an ACID compliant DB.

@joseph.olstad I assume the sites you describe are essentially setting a variable on a pretty high % of page requests, and are 'high traffic', i.e. more than one current request. In that case, running stock Drupal core, there would likely be a decent number of page requests waiting for the lock to be available, and then as soon as it is, one of them discovers that the cache needs rebuilding and acquires the lock and rebuilds the cache. But then from what you describe it's often the case that that request might invalidate the cache. Then one of the threads waiting for the lock will eventually get it, and rebuild the cache.
As acquiring the lock isn't a FIFO queue, it's simply random, it's entirely possible that a number of the threads will keep waiting on the lock essentially forever.

The patch in #136 gets rid of your threads waiting around, which is indeed a very good thing, but unless your code is doing some magic on the variables table there's still the race condition there, no?

@mcdruid I assume that in #156 what you're doing is essentially de-risking the release in the case that there's some site using variables in a weird way, you can drop something in the release notes and tell people to read them and that'll sort it. In the same way that we hope this fix is going to fix @joseph.olstad's problem, it might cause someone else problems. The variable system is such a fundamental thing that changing the loading behavior could make for weird edge cases where it breaks. I applaud the effort to provide an easy 'opt out' for people who for whatever reason, do not want this change.
Maybe someone is running some system where selecting and unseriazling all those variables has a significantly higher cost than waiting and getting them out of cache and so maybe they'd opt-out of the 'just carry on approach' as that would actually be worse for them. These things are hard to predict!

So, as you asked in #156: the logic seems sounds to me, it is essentially the patch in #136 by default, and the existing behavior with the opt out enabled. The additional function call is basically negligible I'd say, since it really is a single call to another function that then accesses some global memory. If adding that sort of thing is a significant performance penalty, then your Drupal site would have a much bigger problems to solve!

@joseph.olstad I assume you'd be happy with either of #136 or #156 being commited, maybe now that I've given a possible reason someone might want to opt out?

@Steven Jones thanks for a great summary.

Your description of my approach aiming to "de-risk the release" is spot on; my hope was that #156 is the same as #136 by default but with an opt-out for any sites that want to keep the existing behaviour (for whatever reason). Thanks for your review of the logic.

I was once asked to provide examples of "something inconceivable that might happen" and had to point out that by definition it's pretty hard to conceive of something inconceivable ;) As you mention, edge cases where this change may make things worse might be hard to imagine, but I am uncomfortable forcing it on all D7 sites without providing an opt-out.

Are we RTBC on #156?

Also, the patch(es) we're now considering don't really reflect the issue title here or the current IS, which I'll try to address if/when this is committed.

Ok on 156

I think 156 is overly cautious but it is an improvement.

In all of my 10 years on Drupal extensively with over 250 different apps on drupal including one for an international public govt celebrity starts with "J" running high volume spikes on a clustered mysql backend I have very rarely maybe once or twice in 10 years seen a lock issue and it was caused by scripting or very poor implementation unrelated to the variable table that we were able to resolve.

variable get should rebuild cache when set, it's logical to me in terms of caching. similar to expiring a page cache when a node is updated.

With that said I appreciate the 156 opt in which config is better than patching.

I'm ok with either 136 or 156.

Thanks for your time on this.

So I think we are RTBC on #156 with the caveat that you might want to improve the comment on commit?

Great, thank you.

Yes, any suggested tweak to the comments for default.settings.php welcome - I'll have a think.

@mcdruid, I just re-read 156, this is perfect thanks!

so if I understand the patch correctly you're providing an option to use the previous logic but the default will be to use the new logic.

great! looking forward to this.

While I think I understand the purpose of the patch I am finding the comment in the default.settings.php a bit unclear.

+/**
+ * Opt out of variable_initialize() locking optimization.
+ *
+ * After lengthy discussion in https://www.drupal.org/node/973436 a change was
+ * made in variable_initialize() in order to avoid excessive waiting under
+ * certain conditions. Set this variable to TRUE in order to opt out of this
+ * optimization and revert to the original behaviour.
+ */

Isn't the purpose of the patch to use a lock to prevent arriving at the wrong result and the concern is that under certain conditions it may result in excessive waits in which case one may want to opt out of this optimization?

If this is the case the wording may need to be changed to something like:

+ * After lengthy discussion in https://www.drupal.org/node/973436 a change was
+ * made in variable_initialize() that includes locking to ensure the correct
+ * result is returned. This may result in excessive waiting in certain
+ * conditions. To avoid excessive waiting set this variable to TRUE to opt out
+ * of this optimization and revert to the original behaviour.
+ */

I just spent 30 minutes looking at this patch again closely and figuring out why it works so well (136).

I think its important to mention the crux of this fix and why we don't need an opt-out (but I'm not against it I just think it's not necessary).

The crux is this, with the fix, the cache_set function (a write to the db) is ONLY called if a lock is successfully made, notice the 1 second lock, this is consistent with what I observed on unpatched systems getting stale values if two requests within 1000 milliseconds, so with the new logic there won't be a stampede ever and always correct uncached value on a cache miss due to the fact that only the successful 1 second lock does the write to cache. If someone else (another request) has a lock then there will not be a cache_set the uncached result is instead retrieved directly from the db (a read).

with the patch 136/156 this cache miss logic is only invoked when the cache is missed (patch 156 slightly tougher to read and understand but looks ok). If the cache is missed without a lock there's a read straight from the db, something that Drupal does all the time anyway. If a lock is successfully made, a write to the cache is performed.

Voilà les raisons que je n'ai aucun souci avec ce patch!

as for the wording of the settings.default.php the original wording from 156 is sufficient. We don't know if anyone will actually need this but if we (in the unlikely event) get support requests about this we can quickly point to the option that allows this to be opted-out.

@izmeez no :)

At this very moment Drupal core does use a lock, but this lock is only to prevent multiple threads doing the exact same work: to take the variables from the variable table and put them into the cache system.
Having them as one giant lump in the cache system 'improves performance by 20% when serving cached pages' according to a comment in variable_initialize.

The patch in #156 changes that so that if the variables are not in the cache system, a single thread still does the work and pops them all into the cache system, but while that thread is doing that, all of the other threads are not prevented from continuing, they simply read the variables from the table (possibly incurring a 20% performance penalty.)

However, what patch #156 also allows is configuration so that someone can opt-out of that behavior, if for example as mentioned in #177:

someone is running some system where selecting and unseriazling all those variables has a significantly higher cost than waiting and getting them out of cache

Then maybe they'd actually be looking at a huge performance issue. For example, maybe their cache system is a very, very fast but their backend storage database is very, very slow and can't handle lots and lots of connections. In that case, they would almost certainly want to preserve the current behavior and block other threads rather than overloading their backend database.

What they are opt-ing out of is the 'carry on with selecting the variables from the database table directly in the event of a cache miss and another thread is going set the cache in a moment' behavior.
Drupal will still be using locks to ensure that only one thread bothers to do the cache set/write.

@joseph.olstad I think that the variable system is probably best thought of as an 'eventually consistent' system. It's riddled with race conditions, and you may or may not get some depending on the state of transactions during the page request and configured cache backends etc. I think we're on the same page, but I'll just say again that the variables system not a Software Transactional Memory system nor does it use locks to ensure that things like 'incrementing' a variable are guaranteed to work as you may expect. If you need those things then #177 mentions some alternatives. No amount of fiddling with the patches in #136 or #156 is going to change that.

@mcdruid How about:

After lengthy discussion in https://www.drupal.org/node/973436 a change was made in variable_initialize() in order to avoid excessive waiting under certain conditions. More specifically: if a variable is changed or the variable cache otherwise cleared, multiple concurrent request threads would have coordinated using a lock so that a single thread would rebuild the variable cache and then all the other threads block/wait until that cache entry was available. This was changed so that by default the other threads would read the variables directly from the variables table too while the cache was being rebuilt.

On most sites this change should have very little negative impact, as variables are changed relatively infrequently and the additional cost of multiple threads briefly doing the same work should be small. However, depending on the exact site configuration and database and cache components used there's a small possibility that this additional cost would be high enough to outweigh the benefits of allowing multiple threads to continue in parallel.

Set this variable to TRUE in order to opt out of this optimization removing the coordinated waiting and revert to the original behaviour.

Maybe it would be clearer to actually make it an opt-in to the previous 'blocking' behaviour?

The opt out is to read the stale value from the locked thread rather than read the correct current value from the variables table directly. Both are reads, the value in the variable table is correct it is the cached value that sometimes ends up stale.

Both 136 and unpatched core correctly prevent stampeding cache table writes only the thread which acquires the 1 second lock is able to write to cache.

What we are fixing is to bypass cache and read the correct value with db query. There is no risk of a stampede in either case because neither approach results in more writes.

I am not sure why someone would want to opt out of a table read in this case but that is their prerogative.

The use case I outlined is using variable set increment value and get to get the new count. variable_set is working it is variable_get that we are patching.

The issue summary is somewhat misleading.

This is a critical bug for some and not circumventable easily.

So let's classify as a bug.

The bug is endless rebuilding of the variables cache.

I am okay with opt-in (as it fixes a bug) and this is approved after issue summary update.

There are three different sites:

a) Using DB purely -> not an issue as the write incurred by the "lock()" is worse performance than the "select()" anyway prompting some to remove the lock all-together (and for those site that indeed would be best)

b) Using Memcache / redis and high performance -> Hopefully have done their homework to have as less variable_set calls as possible, it could stampede for them in theory, but there are other things loaded from the DB (like roles) that hopefully they should cope well. Worst case they know what they are doing and can opt-out again.

c) Abusing the variable system with high traffic -> Lot's of variable_set for setting state(). For those the issue is solved now.

So all that don't care as much win and those that specifically tune can measure and opt-out if needed.

Updating IS.

#156 need a reroll - no interdiff, as the problem was just other variables have been committed to default.settings.php

I'm actually fairly happy with the minimal comment in default.settings.php as it is; @Steven Jones provides much more detail in #185 but I'm inclined to leave that for the CR / release notes and keep the comments in (default.)settings.php simple.

Looks good!

Thanks everyone!

Excellent! my patch anxiety levels have dropped significantly!