From 39ed32922e76422ab6dfbb50d497a2b89d47c9bf Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Tue, 3 May 2016 03:53:06 -0700 Subject: [PATCH] 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 --- includes/poolcounter/PoolCounter.php | 20 ++++++++++-------- includes/poolcounter/PoolCounterWork.php | 2 +- .../PoolCounterWorkViaCallback.php | 2 +- .../includes/poolcounter/PoolCounterTest.php | 21 +++++++++++++------ 4 files changed, 28 insertions(+), 17 deletions(-) diff --git a/includes/poolcounter/PoolCounter.php b/includes/poolcounter/PoolCounter.php index acdbd81361..bd7072ab7e 100644 --- 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 e61b65ad72..a570d78c48 100644 --- 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 85a7cef6ef..834b8b1f83 100644 --- 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 15b47b4a8c..d57ad04125 100644 --- 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 ) ); } } -- 2.20.1