From e626d014ac95581cfce25426a10a60e17725f66d Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Wed, 20 Mar 2019 11:11:00 -0700 Subject: [PATCH] objectcache: remove BagOStuff::mergeViaLock() and update RESTBagOStuff All subclasses are now using the CAS variant, which respects $attempts. Change-Id: Ia7ec6a7f3337cabe95c54c1142c3c5464c1794e7 --- includes/libs/objectcache/BagOStuff.php | 58 ++---------------- .../libs/objectcache/MultiWriteBagOStuff.php | 2 +- includes/libs/objectcache/RESTBagOStuff.php | 55 ++++++++++------- .../libs/objectcache/BagOStuffTest.php | 60 +++++-------------- 4 files changed, 52 insertions(+), 123 deletions(-) diff --git a/includes/libs/objectcache/BagOStuff.php b/includes/libs/objectcache/BagOStuff.php index e2b021293a..4fe64f2641 100644 --- a/includes/libs/objectcache/BagOStuff.php +++ b/includes/libs/objectcache/BagOStuff.php @@ -278,7 +278,7 @@ abstract class BagOStuff implements IExpiringStore, LoggerAwareInterface { * @throws InvalidArgumentException */ public function merge( $key, callable $callback, $exptime = 0, $attempts = 10, $flags = 0 ) { - return $this->mergeViaLock( $key, $callback, $exptime, $attempts, $flags ); + return $this->mergeViaCas( $key, $callback, $exptime, $attempts, $flags ); } /** @@ -311,11 +311,13 @@ abstract class BagOStuff implements IExpiringStore, LoggerAwareInterface { // Derive the new value from the old value $value = call_user_func( $callback, $this, $key, $currentValue, $exptime ); + $hadNoCurrentValue = ( $currentValue === false ); + unset( $currentValue ); // free RAM in case the value is large $this->clearLastError(); if ( $value === false ) { $success = true; // do nothing - } elseif ( $currentValue === false ) { + } elseif ( $hadNoCurrentValue ) { // Try to create the key, failing if it gets created in the meantime $success = $this->add( $key, $value, $exptime, $flags ); } else { @@ -369,58 +371,6 @@ abstract class BagOStuff implements IExpiringStore, LoggerAwareInterface { return $success; } - /** - * @see BagOStuff::merge() - * - * @param string $key - * @param callable $callback Callback method to be executed - * @param int $exptime Either an interval in seconds or a unix timestamp for expiry - * @param int $attempts The amount of times to attempt a merge in case of failure - * @param int $flags Bitfield of BagOStuff::WRITE_* constants - * @return bool Success - */ - protected function mergeViaLock( $key, $callback, $exptime = 0, $attempts = 10, $flags = 0 ) { - if ( $attempts <= 1 ) { - $timeout = 0; // clearly intended to be "non-blocking" - } else { - $timeout = 3; - } - - if ( !$this->lock( $key, $timeout ) ) { - return false; - } - - $this->clearLastError(); - $reportDupes = $this->reportDupes; - $this->reportDupes = false; - $currentValue = $this->get( $key, self::READ_LATEST ); - $this->reportDupes = $reportDupes; - - if ( $this->getLastError() ) { - $this->logger->warning( - __METHOD__ . ' failed due to I/O error on get() for {key}.', - [ 'key' => $key ] - ); - - $success = false; - } else { - // Derive the new value from the old value - $value = call_user_func( $callback, $this, $key, $currentValue, $exptime ); - if ( $value === false ) { - $success = true; // do nothing - } else { - $success = $this->set( $key, $value, $exptime, $flags ); // set the new value - } - } - - if ( !$this->unlock( $key ) ) { - // this should never happen - trigger_error( "Could not release lock for key '$key'." ); - } - - return $success; - } - /** * Change the expiration on a key if it exists * diff --git a/includes/libs/objectcache/MultiWriteBagOStuff.php b/includes/libs/objectcache/MultiWriteBagOStuff.php index 5cf9de4cc8..2d3eed583a 100644 --- a/includes/libs/objectcache/MultiWriteBagOStuff.php +++ b/includes/libs/objectcache/MultiWriteBagOStuff.php @@ -104,7 +104,7 @@ class MultiWriteBagOStuff extends BagOStuff { if ( ( $flags & self::READ_LATEST ) == self::READ_LATEST ) { // If the latest write was a delete(), we do NOT want to fallback // to the other tiers and possibly see the old value. Also, this - // is used by mergeViaLock(), which only needs to hit the primary. + // is used by merge(), which only needs to hit the primary. return $this->caches[0]->get( $key, $flags ); } diff --git a/includes/libs/objectcache/RESTBagOStuff.php b/includes/libs/objectcache/RESTBagOStuff.php index c127ec6910..7e578f96d9 100644 --- a/includes/libs/objectcache/RESTBagOStuff.php +++ b/includes/libs/objectcache/RESTBagOStuff.php @@ -84,12 +84,15 @@ class RESTBagOStuff extends BagOStuff { $this->client->setLogger( $logger ); } - /** - * @param string $key - * @param int $flags Bitfield of BagOStuff::READ_* constants [optional] - * @return mixed Returns false on failure and if the item does not exist - */ protected function doGet( $key, $flags = 0 ) { + $casToken = null; + + return $this->getWithToken( $key, $casToken, $flags ); + } + + protected function getWithToken( $key, &$casToken, $flags = 0 ) { + $casToken = null; + $req = [ 'method' => 'GET', 'url' => $this->url . rawurlencode( $key ), @@ -98,7 +101,11 @@ class RESTBagOStuff extends BagOStuff { list( $rcode, $rdesc, $rhdrs, $rbody, $rerr ) = $this->client->run( $req ); if ( $rcode === 200 ) { if ( is_string( $rbody ) ) { - return unserialize( $rbody ); + $value = unserialize( $rbody ); + /// @FIXME: use some kind of hash or UUID header as CAS token + $casToken = ( $value !== false ) ? $rbody : null; + + return $value; } return false; } @@ -108,22 +115,6 @@ class RESTBagOStuff extends BagOStuff { return false; } - /** - * Handle storage error - * @param string $msg Error message - * @param int $rcode Error code from client - * @param string $rerr Error message from client - * @return false - */ - protected function handleError( $msg, $rcode, $rerr ) { - $this->logger->error( "$msg : ({code}) {error}", [ - 'code' => $rcode, - 'error' => $rerr - ] ); - $this->setLastError( $rcode === 0 ? self::ERR_UNREACHABLE : self::ERR_UNEXPECTED ); - return false; - } - public function set( $key, $value, $exptime = 0, $flags = 0 ) { // @TODO: respect WRITE_SYNC (e.g. EACH_QUORUM) // @TODO: respect $exptime @@ -172,4 +163,24 @@ class RESTBagOStuff extends BagOStuff { return false; } + + public function merge( $key, callable $callback, $exptime = 0, $attempts = 10, $flags = 0 ) { + return $this->mergeViaCas( $key, $callback, $exptime, $attempts, $flags ); + } + + /** + * Handle storage error + * @param string $msg Error message + * @param int $rcode Error code from client + * @param string $rerr Error message from client + * @return false + */ + protected function handleError( $msg, $rcode, $rerr ) { + $this->logger->error( "$msg : ({code}) {error}", [ + 'code' => $rcode, + 'error' => $rerr + ] ); + $this->setLastError( $rcode === 0 ? self::ERR_UNREACHABLE : self::ERR_UNEXPECTED ); + return false; + } } diff --git a/tests/phpunit/includes/libs/objectcache/BagOStuffTest.php b/tests/phpunit/includes/libs/objectcache/BagOStuffTest.php index b68ffaf8d6..3d8c9cbb0b 100644 --- a/tests/phpunit/includes/libs/objectcache/BagOStuffTest.php +++ b/tests/phpunit/includes/libs/objectcache/BagOStuffTest.php @@ -65,20 +65,10 @@ class BagOStuffTest extends MediaWikiTestCase { /** * @covers BagOStuff::merge - * @covers BagOStuff::mergeViaLock * @covers BagOStuff::mergeViaCas */ public function testMerge() { $key = $this->cache->makeKey( self::TEST_KEY ); - $locks = false; - $checkLockingCallback = function ( BagOStuff $cache, $key, $oldVal ) use ( &$locks ) { - $locks = $cache->get( "$key:lock" ); - - return false; - }; - - $this->cache->merge( $key, $checkLockingCallback, 5 ); - $this->assertFalse( $this->cache->get( $key ) ); $calls = 0; $casRace = false; // emulate a race @@ -103,31 +93,19 @@ class BagOStuffTest extends MediaWikiTestCase { $this->assertEquals( 'mergedmerged', $this->cache->get( $key ) ); $calls = 0; - if ( $locks ) { - // merge were something else already was merging (e.g. had the lock) - $this->cache->lock( $key ); - $this->assertFalse( - $this->cache->merge( $key, $callback, 5, 1 ), - 'Non-blocking merge (locking)' - ); - $this->cache->unlock( $key ); - $this->assertEquals( 0, $calls ); - } else { - $casRace = true; - $this->assertFalse( - $this->cache->merge( $key, $callback, 5, 1 ), - 'Non-blocking merge (CAS)' - ); - $this->assertEquals( 1, $calls ); - } + $casRace = true; + $this->assertFalse( + $this->cache->merge( $key, $callback, 5, 1 ), + 'Non-blocking merge (CAS)' + ); + $this->assertEquals( 1, $calls ); } /** * @covers BagOStuff::merge - * @covers BagOStuff::mergeViaLock * @dataProvider provideTestMerge_fork */ - public function testMerge_fork( $exists, $winsLocking, $resLocking, $resCAS ) { + public function testMerge_fork( $exists, $childWins, $resCAS ) { $key = $this->cache->makeKey( self::TEST_KEY ); $pCallback = function ( BagOStuff $cache, $key, $oldVal ) { return ( $oldVal === false ) ? 'init-parent' : $oldVal . '-merged-parent'; @@ -153,16 +131,12 @@ class BagOStuffTest extends MediaWikiTestCase { $fork &= !$this->cache instanceof MultiWriteBagOStuff; if ( $fork ) { $pid = null; - $locked = false; // Function to start merge(), run another merge() midway through, then finish - $func = function ( BagOStuff $cache, $key, $cur ) - use ( $pCallback, $cCallback, &$pid, &$locked ) - { + $func = function ( $cache, $key, $cur ) use ( $pCallback, $cCallback, &$pid ) { $pid = pcntl_fork(); if ( $pid == -1 ) { return false; } elseif ( $pid ) { - $locked = $cache->get( "$key:lock" ); // parent has lock? pcntl_wait( $status ); return $pCallback( $cache, $key, $cur ); @@ -182,15 +156,9 @@ class BagOStuffTest extends MediaWikiTestCase { return; // can't fork, ignore this test... } - if ( $locked ) { - // merge succeed since child was locked out - $this->assertEquals( $winsLocking, $merged ); - $this->assertEquals( $this->cache->get( $key ), $resLocking ); - } else { - // merge has failed because child process was merging (and we only attempted once) - $this->assertEquals( !$winsLocking, $merged ); - $this->assertEquals( $this->cache->get( $key ), $resCAS ); - } + // merge has failed because child process was merging (and we only attempted once) + $this->assertEquals( !$childWins, $merged ); + $this->assertEquals( $this->cache->get( $key ), $resCAS ); } else { $this->markTestSkipped( 'No pcntl methods available' ); } @@ -198,9 +166,9 @@ class BagOStuffTest extends MediaWikiTestCase { function provideTestMerge_fork() { return [ - // (already exists, parent wins if locking, result if locking, result if CAS) - [ false, true, 'init-parent', 'init-child' ], - [ true, true, 'x-merged-parent', 'x-merged-child' ] + // (already exists, child wins CAS, result of CAS) + [ false, true, 'init-child' ], + [ true, true, 'x-merged-child' ] ]; } -- 2.20.1