From 97c7a897c8a9c613ad7c6d977a2ebd7b48211ef0 Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Mon, 27 Jul 2015 12:28:59 -0700 Subject: [PATCH] Avoid some possible deadlocks on account creation * This uses a non-blocking $wgMemc lock to reserve the user name in question. This should prevent two threads from reaching LOCK IN SHARE MODE and getting stuck on INSERT. The lock is global to better cover auth plugins. * This adds a BagOStuff::getScopedLock() convenience method. It uses less queries than LockManager by being EX only. * Avoid extra lock attempt in lock() in non-blocking mode. * Removed (un)lock() HashBagOStuff overrides that made it behave differently than other caches (wrt to re-entrance). Bug: T106850 Change-Id: Iecf95206d712367f5d202f76ab0eaa9d7bdabf2b --- includes/libs/objectcache/BagOStuff.php | 47 +++++++++++++++++-- includes/libs/objectcache/HashBagOStuff.php | 8 ---- includes/specials/SpecialUserlogin.php | 8 +++- languages/i18n/en.json | 1 + languages/i18n/qqq.json | 1 + .../includes/objectcache/BagOStuffTest.php | 18 +++++++ 6 files changed, 69 insertions(+), 14 deletions(-) diff --git a/includes/libs/objectcache/BagOStuff.php b/includes/libs/objectcache/BagOStuff.php index cc07db4453..1e929e63d4 100644 --- a/includes/libs/objectcache/BagOStuff.php +++ b/includes/libs/objectcache/BagOStuff.php @@ -222,22 +222,23 @@ abstract class BagOStuff implements LoggerAwareInterface { /** * @param string $key * @param int $timeout Lock wait timeout; 0 for non-blocking [optional] - * @param int $expiry Lock expiry [optional] + * @param int $expiry Lock expiry [optional]; 1 day maximum * @return bool Success */ public function lock( $key, $timeout = 6, $expiry = 6 ) { + $expiry = min( $expiry ?: INF, 86400 ); + $this->clearLastError(); $timestamp = microtime( true ); // starting UNIX timestamp if ( $this->add( "{$key}:lock", 1, $expiry ) ) { return true; - } elseif ( $this->getLastError() ) { - return false; + } elseif ( $this->getLastError() || $timeout <= 0 ) { + return false; // network partition or non-blocking } $uRTT = ceil( 1e6 * ( microtime( true ) - $timestamp ) ); // estimate RTT (us) $sleep = 2 * $uRTT; // rough time to do get()+set() - $locked = false; // lock acquired $attempts = 0; // failed attempts do { if ( ++$attempts >= 3 && $sleep <= 5e5 ) { @@ -249,7 +250,7 @@ abstract class BagOStuff implements LoggerAwareInterface { $this->clearLastError(); $locked = $this->add( "{$key}:lock", 1, $expiry ); if ( $this->getLastError() ) { - return false; + return false; // network partition } } while ( !$locked && ( microtime( true ) - $timestamp ) < $timeout ); @@ -264,6 +265,42 @@ abstract class BagOStuff implements LoggerAwareInterface { return $this->delete( "{$key}:lock" ); } + /** + * Get a lightweight exclusive self-unlocking lock + * + * Note that the same lock cannot be acquired twice. + * + * This is useful for task de-duplication or to avoid obtrusive + * (though non-corrupting) DB errors like INSERT key conflicts + * or deadlocks when using LOCK IN SHARE MODE. + * + * @param string $key + * @param int $timeout Lock wait timeout; 0 for non-blocking [optional] + * @param int $expiry Lock expiry [optional]; 1 day maximum + * @return ScopedLock|null Returns null on failure + * @since 1.26 + */ + final public function getScopedLock( $key, $timeout = 6, $expiry = 30 ) { + $expiry = min( $expiry ?: INF, 86400 ); + + if ( !$this->lock( $key, $timeout, $expiry ) ) { + return null; + } + + $lSince = microtime( true ); // lock timestamp + $that = $this; + + return new ScopedCallback( function() use ( $that, $key, $lSince, $expiry ) { + $latency = .050; // latency skew (err towards keeping lock present) + $age = ( microtime( true ) - $lSince + $latency ); + if ( ( $age + $latency ) >= $expiry ) { + $this->logger->warning( "Lock for $key held too long ($age sec)." ); + return; // expired; it's not "safe" to delete the key + } + $that->unlock( $key ); + } ); + } + /** * Delete all objects expiring before a certain date. * @param string $date The reference date in MW format diff --git a/includes/libs/objectcache/HashBagOStuff.php b/includes/libs/objectcache/HashBagOStuff.php index 185c74bfec..e03c83f53a 100644 --- a/includes/libs/objectcache/HashBagOStuff.php +++ b/includes/libs/objectcache/HashBagOStuff.php @@ -76,12 +76,4 @@ class HashBagOStuff extends BagOStuff { return true; } - - public function lock( $key, $timeout = 6, $expiry = 6 ) { - return true; - } - - function unlock( $key ) { - return true; - } } diff --git a/includes/specials/SpecialUserlogin.php b/includes/specials/SpecialUserlogin.php index 0b500f4752..d3e334b988 100644 --- a/includes/specials/SpecialUserlogin.php +++ b/includes/specials/SpecialUserlogin.php @@ -559,7 +559,13 @@ class LoginForm extends SpecialPage { $u = User::newFromName( $this->mUsername, 'creatable' ); if ( !$u ) { return Status::newFatal( 'noname' ); - } elseif ( 0 != $u->idForName( User::READ_LOCKING ) ) { + } + + # Make sure the user does not exist already + $lock = $wgMemc->getScopedLock( wfGlobalCacheKey( 'account', md5( $this->mUsername ) ) ); + if ( !$lock ) { + return Status::newFatal( 'usernameinprogress' ); + } elseif ( $u->idForName( User::READ_LOCKING ) ) { return Status::newFatal( 'userexists' ); } diff --git a/languages/i18n/en.json b/languages/i18n/en.json index 00ffa0eba9..0387df59bf 100644 --- a/languages/i18n/en.json +++ b/languages/i18n/en.json @@ -451,6 +451,7 @@ "createacct-benefit-head3": "{{NUMBEROFACTIVEUSERS}}", "createacct-benefit-body3": "recent {{PLURAL:$1|contributor|contributors}}", "badretype": "The passwords you entered do not match.", + "usernameinprogress": "An account creation for this user name is already in progress.\nPlease wait.", "userexists": "Username entered already in use.\nPlease choose a different name.", "loginerror": "Login error", "createacct-error": "Account creation error", diff --git a/languages/i18n/qqq.json b/languages/i18n/qqq.json index c180a4a10b..ceb8642dfd 100644 --- a/languages/i18n/qqq.json +++ b/languages/i18n/qqq.json @@ -623,6 +623,7 @@ "createacct-benefit-body3": "In vertical-layout create account form, the text for the third benefit.\n\nPreceded by the message {{msg-mw|Createacct-benefit-head3}} (number of contributors).\n\nSee example: [{{canonicalurl:Special:UserLogin|type=signup}} Special:UserLogin?type=signup]\n\nParameters:\n* $1 - number of contributors (users)", "badretype": "Used as error message when the new password and its retype do not match.", "userexists": "Used as error message in creating a user account.", + "usernameinprogress": "Used as error message in creating a user account.", "loginerror": "Used as title of error message.\n{{Identical|Login error}}", "createacct-error": "Used as heading for the error message.", "createaccounterror": "Parameters:\n* $1 - an error message", diff --git a/tests/phpunit/includes/objectcache/BagOStuffTest.php b/tests/phpunit/includes/objectcache/BagOStuffTest.php index 4516bb4e71..fcc15d317b 100644 --- a/tests/phpunit/includes/objectcache/BagOStuffTest.php +++ b/tests/phpunit/includes/objectcache/BagOStuffTest.php @@ -3,6 +3,7 @@ * @author Matthias Mullie */ class BagOStuffTest extends MediaWikiTestCase { + /** @var BagOStuff */ private $cache; protected function setUp() { @@ -152,4 +153,21 @@ class BagOStuffTest extends MediaWikiTestCase { $this->cache->delete( $key1 ); $this->cache->delete( $key2 ); } + + /** + * @covers BagOStuff::getScopedLock + */ + public function testGetScopedLock() { + $key = wfMemcKey( 'test' ); + $value1 = $this->cache->getScopedLock( $key, 0 ); + $value2 = $this->cache->getScopedLock( $key, 0 ); + + $this->assertType( 'ScopedCallback', $value1, 'First call returned lock' ); + $this->assertNull( $value2, 'Duplicate call returned no lock' ); + + unset( $value1 ); + + $value3 = $this->cache->getScopedLock( $key, 0 ); + $this->assertType( 'ScopedCallback', $value3, 'Lock returned callback after release' ); + } } -- 2.20.1