Add global limit to PoolCounter

Adds a 'slot' option to $wgPoolCounterConf. When this option is set,
there are a limited number of slots for the given worker type,
regardless of key. Workers with the same key are still limited by
the 'workers' option.

The global limit is implemented by simply using a deterministic
hash of the key instead of the real key. This avoids deadlocks,
but results in slot underallocation due to hash collisions - even
when there are significantly more jobs that slots, some of the slots
might remain empty.

Bug: 65691
Change-Id: Ibdf8ec222f9756d70de2bed7ff14913351dc394b

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