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

diff --git a/includes/libs/objectcache/BagOStuff.php b/includes/libs/objectcache/BagOStuff.php
index cc07db4..1e929e6 100644 (file)
--- 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 185c74b..e03c83f 100644 (file)
--- 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 0b500f4..d3e334b 100644 (file)
--- 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 00ffa0e..0387df5 100644 (file)
--- 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 c180a4a..ceb8642 100644 (file)
--- 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 4516bb4..fcc15d3 100644 (file)
--- a/tests/phpunit/includes/objectcache/BagOStuffTest.php
+++ b/tests/phpunit/includes/objectcache/BagOStuffTest.php
@@ -3,6 +3,7 @@
  * @author Matthias Mullie <mmullie@wikimedia.org>
  */
 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' );
+       }
 }