Include type in hashKeyIntoSlots()

Otherwise, all pool types that use slots will collide
due to the slotted keys not using a type prefix.

Bug: T134144
Change-Id: Ib367fedf2cfb7fecc290206e69e0d105276e96e6

diff --git a/includes/poolcounter/PoolCounter.php b/includes/poolcounter/PoolCounter.php
index acdbd81..bd7072a 100644 (file)
--- a/includes/poolcounter/PoolCounter.php
+++ b/includes/poolcounter/PoolCounter.php
@@ -81,7 +81,7 @@ abstract class PoolCounter {
 
        /**
         * @param array $conf
-        * @param string $type
+        * @param string $type The class of actions to limit concurrency for (task type)
         * @param string $key
         */
        protected function __construct( $conf, $type, $key ) {
@@ -93,8 +93,9 @@ abstract class PoolCounter {
                }
 
                if ( $this->slots ) {
-                       $key = $this->hashKeyIntoSlots( $key, $this->slots );
+                       $key = $this->hashKeyIntoSlots( $type, $key, $this->slots );
                }
+
                $this->key = $key;
                $this->isMightWaitKey = !preg_match( '/^nowait:/', $this->key );
        }
@@ -102,7 +103,7 @@ abstract class PoolCounter {
        /**
         * Create a Pool counter. This should only be called from the PoolWorks.
         *
-        * @param string $type
+        * @param string $type The class of actions to limit concurrency for (task type)
         * @param string $key
         *
         * @return PoolCounter
@@ -192,18 +193,19 @@ abstract class PoolCounter {
        }
 
        /**
-        * 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
+        * Given a key (any string) and the number of lots, returns a slot key (a prefix with a suffix
+        * 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 $type The class of actions to limit concurrency for (task type)
         * @param string $key PoolCounter instance key (any string)
         * @param int $slots The number of slots (max allowed value is 65536)
-        * @return int
+        * @return string Slot key with the type and slot number
         */
-       protected function hashKeyIntoSlots( $key, $slots ) {
-               return hexdec( substr( sha1( $key ), 0, 4 ) ) % $slots;
+       protected function hashKeyIntoSlots( $type, $key, $slots ) {
+               return $type . ':' . ( hexdec( substr( sha1( $key ), 0, 4 ) ) % $slots );
        }
 }
 

diff --git a/includes/poolcounter/PoolCounterWork.php b/includes/poolcounter/PoolCounterWork.php
index e61b65a..a570d78 100644 (file)
--- a/includes/poolcounter/PoolCounterWork.php
+++ b/includes/poolcounter/PoolCounterWork.php
@@ -31,7 +31,7 @@ abstract class PoolCounterWork {
        protected $cacheable = false; // does this override getCachedWork() ?
 
        /**
-        * @param string $type The type of PoolCounter to use
+        * @param string $type The class of actions to limit concurrency for (task type)
         * @param string $key Key that identifies the queue this work is placed on
         */
        public function __construct( $type, $key ) {

diff --git a/includes/poolcounter/PoolCounterWorkViaCallback.php b/includes/poolcounter/PoolCounterWorkViaCallback.php
index 85a7cef..834b8b1 100644 (file)
--- a/includes/poolcounter/PoolCounterWorkViaCallback.php
+++ b/includes/poolcounter/PoolCounterWorkViaCallback.php
@@ -44,7 +44,7 @@ class PoolCounterWorkViaCallback extends PoolCounterWork {
         * If a 'doCachedWork' callback is provided, then execute() may wait for any prior
         * process in the pool to finish and reuse its cached result.
         *
-        * @param string $type
+        * @param string $type The class of actions to limit concurrency for
         * @param string $key
         * @param array $callbacks Map of callbacks
         * @throws MWException

diff --git a/tests/phpunit/includes/poolcounter/PoolCounterTest.php b/tests/phpunit/includes/poolcounter/PoolCounterTest.php
index 15b47b4..d57ad04 100644 (file)
--- a/tests/phpunit/includes/poolcounter/PoolCounterTest.php
+++ b/tests/phpunit/includes/poolcounter/PoolCounterTest.php
@@ -56,17 +56,26 @@ class PoolCounterTest extends MediaWikiTestCase {
 
                $keysWithTwoSlots = $keysWithFiveSlots = [];
                foreach ( range( 1, 100 ) as $i ) {
-                       $keysWithTwoSlots[] = $hashKeyIntoSlots->invoke( $poolCounter, 'key ' . $i, 2 );
-                       $keysWithFiveSlots[] = $hashKeyIntoSlots->invoke( $poolCounter, 'key ' . $i, 5 );
+                       $keysWithTwoSlots[] = $hashKeyIntoSlots->invoke( $poolCounter, 'test', 'key ' . $i, 2 );
+                       $keysWithFiveSlots[] = $hashKeyIntoSlots->invoke( $poolCounter, 'test', 'key ' . $i, 5 );
                }
 
-               $this->assertArrayEquals( range( 0, 1 ), array_unique( $keysWithTwoSlots ) );
-               $this->assertArrayEquals( range( 0, 4 ), array_unique( $keysWithFiveSlots ) );
+               $twoSlotKeys = [];
+               for ( $i = 0; $i <= 1; $i++ ) {
+                       $twoSlotKeys[] = "test:$i";
+               }
+               $fiveSlotKeys = [];
+               for ( $i = 0; $i <= 4; $i++ ) {
+                       $fiveSlotKeys[] = "test:$i";
+               }
+
+               $this->assertArrayEquals( $twoSlotKeys, array_unique( $keysWithTwoSlots ) );
+               $this->assertArrayEquals( $fiveSlotKeys, array_unique( $keysWithFiveSlots ) );
 
                // make sure it is deterministic
                $this->assertEquals(
-                       $hashKeyIntoSlots->invoke( $poolCounter, 'asdfgh', 1000 ),
-                       $hashKeyIntoSlots->invoke( $poolCounter, 'asdfgh', 1000 )
+                       $hashKeyIntoSlots->invoke( $poolCounter, 'test', 'asdfgh', 1000 ),
+                       $hashKeyIntoSlots->invoke( $poolCounter, 'test', 'asdfgh', 1000 )
                );
        }
 }