Added reentrant lock support to BagOStuff

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

Change-Id: I9a977750c6fc6b8406bc1c9366a6b1b34bf48b6b

diff --git a/includes/libs/objectcache/BagOStuff.php b/includes/libs/objectcache/BagOStuff.php
index 5004a8a..ddbe8ea 100644 (file)
--- 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 e03c83f..b685e41 100644 (file)
--- 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 20e146d..9e80e9f 100644 (file)
--- 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 b5f3bd9..bbfaa5e 100644 (file)
--- 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 fcc15d3..f5814e4 100644 (file)
--- a/tests/phpunit/includes/objectcache/BagOStuffTest.php
+++ b/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' );
        }
 }