Merge "Avoid some possible deadlocks on account creation"

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 13957c3..5b6d22b 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' );
+       }
 }