currently, variable_initialize can take down sites when the lock system is failing. Among other things, the use of recursion can cause a PHP segfault under high enough load.

this patch allows a configurable (as in a setting in settings.php) number of tries to get a lock when rebuilding the variable cache.

if a request fails to get a lock after $configurable_attempts, it simply proceeds with the non-cached code path.

Discussed with catch in IRC. while a better approach was at: #973436: Overzealous locking in variable_initialize() we should go ahead with this simpler fix of giving up reading from the DB.

CommentFileSizeAuthor
#14 2303721-13.patch2.84 KBAnonymous (not verified)
#7 2303721-7.patch1.75 KBAnonymous (not verified)
#3 2303721-3.patch1.74 KBAnonymous (not verified)
variable_initialize.patch1.7 KBAnonymous (not verified)

Comments

pwolanin’s picture

pwolanin commented
Status: Needs review » Needs work

Code style fixes neede: else if -> elseif

Also, need a 2nd param to lock_wait(), since we don't want to wait 30 sec.

pwolanin’s picture

pwolanin commented
Issue summary: View changes
Issue tags: +Performance, +Needs backport to D6
Related issues: +#973436: Overzealous locking in variable_initialize()
Anonymous’s picture

Anonymous (not verified) commented
Status: Needs work » Needs review
StatusFileSize
new 2303721-3.patch1.74 KB

updated patch.

Status: Needs review » Needs work

The last submitted patch, 3: 2303721-3.patch, failed testing.

Status: Needs work » Needs review

pwolanin queued 3: 2303721-3.patch for re-testing.

pwolanin’s picture

pwolanin commented

This is an important fix - the recursion instead of a loop is a really bad pattern, and allowing this to fail out and read is an important improvement.

Discussion the failure modes, I think a lock lifetime of somethign shorter like 5 sec passed to lock_acquire() might be a good idea, but I think 1 in current core is too small.

An then we should reduce the lock wait time to like .2 sec, so in the fail case of a proc holding the lock for it's full lifetime, the user's request is only delayed by 1 sec by default.

Anonymous’s picture

Anonymous (not verified) commented
StatusFileSize
new 2303721-7.patch1.75 KB

here's a patch that implements the suggestions in #6.

pwolanin’s picture

pwolanin commented
Status: Needs review » Reviewed & tested by the community

This looks good, assuming the test comes back green again

catch’s picture

catch
he/him
Primary language English
commented

Looks sensible to me.

damien tournoud’s picture

damien tournoud commented

I still don't get why we don't just proceed if we cannot acquire the lock. Sounds *a lot* more sensible to me than retrying.

Anonymous’s picture

Anonymous (not verified) commented

re #10 - putting this in settings.php achieves that:

$conf['variable_initialize_retry_limit'] = 1;
deviantintegral’s picture

deviantintegral commented

lock_wait($name, 0.2);

lock_wait() takes an integer, not a float. Are you running with #2096259: Allow subsecond delays for lock_wait? It looks like it would still work even though it doesn't match the docs.

pwolanin’s picture

pwolanin commented
Status: Reviewed & tested by the community » Needs work

indeed, we need that patch first.

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs work » Needs review
StatusFileSize
new 2303721-13.patch2.84 KB

re #12 - good point, updated patch to include changes to lock_wait to make it consistent.

Status: Needs review » Closed (outdated)

Automatically closed because Drupal 7 security and bugfix support has ended as of 5 January 2025. If the issue verifiably applies to later versions, please reopen with details and update the version.