Merge "Add global limit to PoolCounter"

diff --git a/includes/poolcounter/PoolCounter.php b/includes/poolcounter/PoolCounter.php
index 34953c0..f8d48cc 100644 (file)
--- a/includes/poolcounter/PoolCounter.php
+++ b/includes/poolcounter/PoolCounter.php
@@ -53,8 +53,15 @@ abstract class PoolCounter {
 
        /** @var string All workers with the same key share the lock */
        protected $key;
-       /** @var int Maximum number of workers doing the task simultaneously */
+       /** @var int Maximum number of workers working on tasks with the same key simultaneously */
        protected $workers;
+       /**
+        * Maximum number of workers working on this task type, regardless of key.
+        * 0 means unlimited. Max allowed value is 65536.
+        * The way the slot limit is enforced is overzealous - this option should be used with caution.
+        * @var int
+        */
+       protected $slots = 0;
        /** @var int If this number of workers are already working/waiting, fail instead of wait */
        protected $maxqueue;
        /** @var float Maximum time in seconds to wait for the lock */
@@ -66,10 +73,17 @@ abstract class PoolCounter {
         * @param string $key
         */
        protected function __construct( $conf, $type, $key ) {
-               $this->key = $key;
                $this->workers = $conf['workers'];
                $this->maxqueue = $conf['maxqueue'];
                $this->timeout = $conf['timeout'];
+               if ( isset( $conf['slots'] ) ) {
+                       $this->slots = $conf['slots'];
+               }
+
+               if ( $this->slots ) {
+                       $key = $this->hashKeyIntoSlots( $key, $this->slots );
+               }
+               $this->key = $key;
        }
 
        /**
@@ -121,6 +135,20 @@ abstract class PoolCounter {
         * @return Status value is one of Released/NotLocked/Error
         */
        abstract public function release();
+
+       /**
+        * Given a key (any string) and the number of lots, returns a slot number (an integer from the [0..($slots-1)] range).
+        * This is used for a global limit on the number of instances  of a given type that can acquire a lock.
+        * The hashing is deterministic so that PoolCounter::$workers is always an upper limit of how many instances with
+        * the same key can acquire a lock.
+        *
+        * @param string $key PoolCounter instance key (any string)
+        * @param int $slots the number of slots (max allowed value is 65536)
+        * @return int
+        */
+       protected function hashKeyIntoSlots( $key, $slots ) {
+               return hexdec( substr( sha1( $key ), 0, 4 ) ) % $slots;
+       }
 }
 
 // @codingStandardsIgnoreStart Squiz.Classes.ValidClassName.NotCamelCaps

diff --git a/tests/phpunit/includes/poolcounter/PoolCounterTest.php b/tests/phpunit/includes/poolcounter/PoolCounterTest.php
new file mode 100644 (file)
index 0000000..2d31d08
--- /dev/null
+++ b/tests/phpunit/includes/poolcounter/PoolCounterTest.php
@@ -0,0 +1,72 @@
+<?php
+
+// We will use this class with getMockForAbstractClass to create a concrete mock class. That call will die if the
+// contructor is not public, unless we use disableOriginalConstructor(), in which case we could not test the constructor.
+abstract class PoolCounterAbstractMock extends PoolCounter {
+       public function __construct() {
+               call_user_func_array( 'parent::__construct', func_get_args() );
+       }
+}
+
+class PoolCounterTest extends MediaWikiTestCase {
+       public function testConstruct() {
+               $poolCounterConfig = array(
+                       'class' => 'PoolCounterMock',
+                       'timeout' => 10,
+                       'workers' => 10,
+                       'maxqueue' => 100,
+               );
+
+               $poolCounter = $this->getMockBuilder( 'PoolCounterAbstractMock' )
+                       ->setConstructorArgs( array( $poolCounterConfig, 'testCounter', 'someKey' ) )
+                       // don't mock anything - the proper syntax would be setMethods(null), but due to a PHPUnit bug that
+                       // does not work with getMockForAbstractClass()
+                       ->setMethods( array( 'idontexist' ) )
+                       ->getMockForAbstractClass();
+               $this->assertInstanceOf( 'PoolCounter', $poolCounter );
+       }
+
+       public function testConstructWithSlots() {
+               $poolCounterConfig = array(
+                       'class' => 'PoolCounterMock',
+                       'timeout' => 10,
+                       'workers' => 10,
+                       'slots' => 2,
+                       'maxqueue' => 100,
+               );
+
+               $poolCounter = $this->getMockBuilder( 'PoolCounterAbstractMock' )
+                       ->setConstructorArgs( array( $poolCounterConfig, 'testCounter', 'key' ) )
+                       ->setMethods( array( 'idontexist' ) ) // don't mock anything
+                       ->getMockForAbstractClass();
+               $this->assertInstanceOf( 'PoolCounter', $poolCounter );
+       }
+
+       public function testHashKeyIntoSlots() {
+               $poolCounter = $this->getMockBuilder( 'PoolCounterAbstractMock' )
+                       // don't mock anything - the proper syntax would be setMethods(null), but due to a PHPUnit bug that
+                       // does not work with getMockForAbstractClass()
+                       ->setMethods( array( 'idontexist' ) )
+                       ->disableOriginalConstructor()
+                       ->getMockForAbstractClass();
+
+               $hashKeyIntoSlots = new ReflectionMethod($poolCounter, 'hashKeyIntoSlots' );
+               $hashKeyIntoSlots->setAccessible( true );
+
+
+               $keysWithTwoSlots = $keysWithFiveSlots = array();
+               foreach ( range( 1, 100 ) as $i ) {
+                       $keysWithTwoSlots[] = $hashKeyIntoSlots->invoke( $poolCounter, 'key ' . $i, 2 );
+                       $keysWithFiveSlots[] = $hashKeyIntoSlots->invoke( $poolCounter, 'key ' . $i, 5 );
+               }
+
+               $this->assertArrayEquals( range( 0, 1 ), array_unique( $keysWithTwoSlots ) );
+               $this->assertArrayEquals( range( 0, 4 ), array_unique( $keysWithFiveSlots ) );
+
+               // make sure it is deterministic
+               $this->assertEquals(
+                       $hashKeyIntoSlots->invoke( $poolCounter, 'asdfgh', 1000 ),
+                       $hashKeyIntoSlots->invoke( $poolCounter, 'asdfgh', 1000 )
+               );
+       }
+}