Added reentrant lock support to BagOStuff

* Also fixed HashBagOStuff::delete() return value for non-keys

Change-Id: I9a977750c6fc6b8406bc1c9366a6b1b34bf48b6b

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;
 		}
 

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;

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 ) {

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;
 		}

tests/phpunit/includes/objectcache/BagOStuffTest.php

@@ -1,6 +1,7 @@
 <?php
 /**
  * @author Matthias Mullie <mmullie@wikimedia.org>
+ * @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' );
 	}
 }