From c9b1fe1896a85ad64204157d3b57cd7f0eaeefbd Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Fri, 21 Aug 2015 14:03:48 -0700 Subject: [PATCH] Added reentrant lock support to BagOStuff * Also fixed HashBagOStuff::delete() return value for non-keys Change-Id: I9a977750c6fc6b8406bc1c9366a6b1b34bf48b6b --- includes/libs/objectcache/BagOStuff.php | 86 +++++++++++++------ includes/libs/objectcache/HashBagOStuff.php | 4 - .../libs/objectcache/ReplicatedBagOStuff.php | 4 +- includes/objectcache/MultiWriteBagOStuff.php | 5 +- .../includes/objectcache/BagOStuffTest.php | 8 ++ 5 files changed, 71 insertions(+), 36 deletions(-) diff --git a/includes/libs/objectcache/BagOStuff.php b/includes/libs/objectcache/BagOStuff.php index 5004a8a6b1..ddbe8eaa3e 100644 --- a/includes/libs/objectcache/BagOStuff.php +++ b/includes/libs/objectcache/BagOStuff.php @@ -43,16 +43,17 @@ use Psr\Log\NullLogger; * @ingroup Cache */ abstract class BagOStuff implements LoggerAwareInterface { - private $debugMode = false; - + /** @var array[] Lock tracking */ + protected $locks = array(); /** @var integer */ protected $lastError = self::ERR_NONE; - /** - * @var LoggerInterface - */ + /** @var LoggerInterface */ protected $logger; + /** @var bool */ + private $debugMode = false; + /** Possible values for getLastError() */ const ERR_NONE = 0; // no error const ERR_NO_RESPONSE = 1; // no response @@ -221,49 +222,77 @@ abstract class BagOStuff implements LoggerAwareInterface { } /** + * Acquire an advisory lock on a key string + * + * Note that if reentry is enabled, duplicate calls ignore $expiry + * * @param string $key * @param int $timeout Lock wait timeout; 0 for non-blocking [optional] * @param int $expiry Lock expiry [optional]; 1 day maximum + * @param string $rclass Allow reentry if set and the current lock used this value * @return bool Success */ - public function lock( $key, $timeout = 6, $expiry = 6 ) { + public function lock( $key, $timeout = 6, $expiry = 6, $rclass = '' ) { + // Avoid deadlocks and allow lock reentry if specified + if ( isset( $this->locks[$key] ) ) { + if ( $rclass != '' && $this->locks[$key]['class'] === $rclass ) { + ++$this->locks[$key]['depth']; + return true; + } else { + return false; + } + } + $expiry = min( $expiry ?: INF, 86400 ); $this->clearLastError(); $timestamp = microtime( true ); // starting UNIX timestamp if ( $this->add( "{$key}:lock", 1, $expiry ) ) { - return true; + $locked = true; } elseif ( $this->getLastError() || $timeout <= 0 ) { - return false; // network partition or non-blocking + $locked = false; // network partition or non-blocking + } else { + $uRTT = ceil( 1e6 * ( microtime( true ) - $timestamp ) ); // estimate RTT (us) + $sleep = 2 * $uRTT; // rough time to do get()+set() + + $attempts = 0; // failed attempts + do { + if ( ++$attempts >= 3 && $sleep <= 5e5 ) { + // Exponentially back off after failed attempts to avoid network spam. + // About 2*$uRTT*(2^n-1) us of "sleep" happen for the next n attempts. + $sleep *= 2; + } + usleep( $sleep ); // back off + $this->clearLastError(); + $locked = $this->add( "{$key}:lock", 1, $expiry ); + if ( $this->getLastError() ) { + $locked = false; // network partition + break; + } + } while ( !$locked && ( microtime( true ) - $timestamp ) < $timeout ); } - $uRTT = ceil( 1e6 * ( microtime( true ) - $timestamp ) ); // estimate RTT (us) - $sleep = 2 * $uRTT; // rough time to do get()+set() - - $attempts = 0; // failed attempts - do { - if ( ++$attempts >= 3 && $sleep <= 5e5 ) { - // Exponentially back off after failed attempts to avoid network spam. - // About 2*$uRTT*(2^n-1) us of "sleep" happen for the next n attempts. - $sleep *= 2; - } - usleep( $sleep ); // back off - $this->clearLastError(); - $locked = $this->add( "{$key}:lock", 1, $expiry ); - if ( $this->getLastError() ) { - return false; // network partition - } - } while ( !$locked && ( microtime( true ) - $timestamp ) < $timeout ); + if ( $locked ) { + $this->locks[$key] = array( 'class' => $rclass, 'depth' => 1 ); + } return $locked; } /** + * Release an advisory lock on a key string + * * @param string $key * @return bool Success */ public function unlock( $key ) { - return $this->delete( "{$key}:lock" ); + if ( isset( $this->locks[$key] ) && --$this->locks[$key]['depth'] <= 0 ) { + unset( $this->locks[$key] ); + + return $this->delete( "{$key}:lock" ); + } + + return true; } /** @@ -278,13 +307,14 @@ 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]; 1 day maximum + * @param string $rclass Allow reentry if set and the current lock used this value * @return ScopedCallback|null Returns null on failure * @since 1.26 */ - final public function getScopedLock( $key, $timeout = 6, $expiry = 30 ) { + final public function getScopedLock( $key, $timeout = 6, $expiry = 30, $rclass = '' ) { $expiry = min( $expiry ?: INF, 86400 ); - if ( !$this->lock( $key, $timeout, $expiry ) ) { + if ( !$this->lock( $key, $timeout, $expiry, $rclass ) ) { return null; } diff --git a/includes/libs/objectcache/HashBagOStuff.php b/includes/libs/objectcache/HashBagOStuff.php index e03c83f53a..b685e41fc3 100644 --- a/includes/libs/objectcache/HashBagOStuff.php +++ b/includes/libs/objectcache/HashBagOStuff.php @@ -68,10 +68,6 @@ class HashBagOStuff extends BagOStuff { } function delete( $key ) { - if ( !isset( $this->bag[$key] ) ) { - return false; - } - unset( $this->bag[$key] ); return true; diff --git a/includes/libs/objectcache/ReplicatedBagOStuff.php b/includes/libs/objectcache/ReplicatedBagOStuff.php index 20e146d281..9e80e9fd1a 100644 --- a/includes/libs/objectcache/ReplicatedBagOStuff.php +++ b/includes/libs/objectcache/ReplicatedBagOStuff.php @@ -104,8 +104,8 @@ class ReplicatedBagOStuff extends BagOStuff { return $this->writeStore->decr( $key, $value ); } - public function lock( $key, $timeout = 6, $expiry = 6 ) { - return $this->writeStore->lock( $key, $timeout, $expiry ); + public function lock( $key, $timeout = 6, $expiry = 6, $rclass = '' ) { + return $this->writeStore->lock( $key, $timeout, $expiry, $rclass ); } public function unlock( $key ) { diff --git a/includes/objectcache/MultiWriteBagOStuff.php b/includes/objectcache/MultiWriteBagOStuff.php index b5f3bd96d2..bbfaa5e1c9 100644 --- a/includes/objectcache/MultiWriteBagOStuff.php +++ b/includes/objectcache/MultiWriteBagOStuff.php @@ -121,12 +121,13 @@ class MultiWriteBagOStuff extends BagOStuff { * @param string $key * @param int $timeout * @param int $expiry + * @param string $rclass * @return bool */ - public function lock( $key, $timeout = 6, $expiry = 6 ) { + public function lock( $key, $timeout = 6, $expiry = 6, $rclass = '' ) { // Lock only the first cache, to avoid deadlocks if ( isset( $this->caches[0] ) ) { - return $this->caches[0]->lock( $key, $timeout, $expiry ); + return $this->caches[0]->lock( $key, $timeout, $expiry, $rclass ); } else { return true; } diff --git a/tests/phpunit/includes/objectcache/BagOStuffTest.php b/tests/phpunit/includes/objectcache/BagOStuffTest.php index fcc15d317b..f5814e4483 100644 --- a/tests/phpunit/includes/objectcache/BagOStuffTest.php +++ b/tests/phpunit/includes/objectcache/BagOStuffTest.php @@ -1,6 +1,7 @@ + * @group BagOStuff */ class BagOStuffTest extends MediaWikiTestCase { /** @var BagOStuff */ @@ -169,5 +170,12 @@ class BagOStuffTest extends MediaWikiTestCase { $value3 = $this->cache->getScopedLock( $key, 0 ); $this->assertType( 'ScopedCallback', $value3, 'Lock returned callback after release' ); + unset( $value3 ); + + $value1 = $this->cache->getScopedLock( $key, 0, 5, 'reentry' ); + $value2 = $this->cache->getScopedLock( $key, 0, 5, 'reentry' ); + + $this->assertType( 'ScopedCallback', $value1, 'First reentrant call returned lock' ); + $this->assertType( 'ScopedCallback', $value1, 'Second reentrant call returned lock' ); } } -- 2.20.1