Aaron Schulz has uploaded a new change for review.
https://gerrit.wikimedia.org/r/227274
Change subject: Avoid some possible deadlocks on account creation
......................................................................
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.
* This adds a BagOStuff::getScopedLock() convenience method.
It uses less queries than LockManager by being EX only.
Other callers, like auth plugins may want to use this too.
Bug: T106850
Change-Id: Iecf95206d712367f5d202f76ab0eaa9d7bdabf2b
---
M includes/libs/objectcache/BagOStuff.php
M includes/specials/SpecialUserlogin.php
M tests/phpunit/includes/objectcache/BagOStuffTest.php
3 files changed, 61 insertions(+), 3 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/core
refs/changes/74/227274/1
diff --git a/includes/libs/objectcache/BagOStuff.php
b/includes/libs/objectcache/BagOStuff.php
index 65ff0ee..c4842a1 100644
--- a/includes/libs/objectcache/BagOStuff.php
+++ b/includes/libs/objectcache/BagOStuff.php
@@ -206,10 +206,12 @@
/**
* @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 ) ) {
@@ -221,7 +223,6 @@
$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,6 +250,41 @@
}
/**
+ * 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 ) {
+ 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
* @param callable|bool $progressCallback Optional, a function which
will be called
diff --git a/includes/specials/SpecialUserlogin.php
b/includes/specials/SpecialUserlogin.php
index 8491f89..fb1abc1 100644
--- a/includes/specials/SpecialUserlogin.php
+++ b/includes/specials/SpecialUserlogin.php
@@ -531,7 +531,11 @@
$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( wfMemcKey( 'username', md5(
$this->mUsername ) ), 0 );
+ if ( !$lock || $u->idForName( User::READ_LOCKING ) ) {
return Status::newFatal( 'userexists' );
}
diff --git a/tests/phpunit/includes/objectcache/BagOStuffTest.php
b/tests/phpunit/includes/objectcache/BagOStuffTest.php
index 4516bb4..f6c7c17 100644
--- a/tests/phpunit/includes/objectcache/BagOStuffTest.php
+++ b/tests/phpunit/includes/objectcache/BagOStuffTest.php
@@ -3,6 +3,7 @@
* @author Matthias Mullie <[email protected]>
*/
class BagOStuffTest extends MediaWikiTestCase {
+ /** @var BagOStuff */
private $cache;
protected function setUp() {
@@ -152,4 +153,21 @@
$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 lock
returned callback' );
+ $this->assertNull( $value2, 'Second lock returned no callback'
);
+
+ unset( $value1 );
+
+ $value3 = $this->cache->getScopedLock( $key, 0 );
+ $this->assertType( 'ScopedCallback', $value3, 'Lock returned
callback after release' );
+ }
}
--
To view, visit https://gerrit.wikimedia.org/r/227274
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iecf95206d712367f5d202f76ab0eaa9d7bdabf2b
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/core
Gerrit-Branch: master
Gerrit-Owner: Aaron Schulz <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
