From d32b64235f26acba05c33157dbbb17bd39f4e190 Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Thu, 8 Sep 2016 00:12:56 -0700 Subject: [PATCH] Switch some callers to WaitConditionLoop Also fixed up backwards documentation Change-Id: I00c36aa751a79ca86a754e049a6da78cbb417b81 --- includes/db/DatabasePostgres.php | 26 ++++++------- .../filebackend/lockmanager/LockManager.php | 22 +++++------ .../lockmanager/MemcLockManager.php | 28 +++++++------- includes/libs/WaitConditionLoop.php | 25 ++++++++---- includes/libs/objectcache/BagOStuff.php | 38 ++++++------------- .../includes/libs/WaitConditionLoopTest.php | 25 ++++++++++++ 6 files changed, 94 insertions(+), 70 deletions(-) diff --git a/includes/db/DatabasePostgres.php b/includes/db/DatabasePostgres.php index 9cd95a1b92..0a178deddc 100644 --- a/includes/db/DatabasePostgres.php +++ b/includes/db/DatabasePostgres.php @@ -1588,21 +1588,21 @@ SQL; */ public function lock( $lockName, $method, $timeout = 5 ) { $key = $this->addQuotes( $this->bigintFromLockName( $lockName ) ); - for ( $attempts = 1; $attempts <= $timeout; ++$attempts ) { - $result = $this->query( - "SELECT pg_try_advisory_lock($key) AS lockstatus", $method ); - $row = $this->fetchObject( $result ); - if ( $row->lockstatus === 't' ) { - parent::lock( $lockName, $method, $timeout ); // record - return true; - } else { - sleep( 1 ); - } - } + $loop = new WaitConditionLoop( + function () use ( $lockName, $key, $timeout, $method ) { + $res = $this->query( "SELECT pg_try_advisory_lock($key) AS lockstatus", $method ); + $row = $this->fetchObject( $res ); + if ( $row->lockstatus === 't' ) { + parent::lock( $lockName, $method, $timeout ); // record + return true; + } - wfDebug( __METHOD__ . " failed to acquire lock\n" ); + return WaitConditionLoop::CONDITION_CONTINUE; + }, + $timeout + ); - return false; + return ( $loop->invoke() === $loop::CONDITION_REACHED ); } /** diff --git a/includes/filebackend/lockmanager/LockManager.php b/includes/filebackend/lockmanager/LockManager.php index 567a29892e..a3cb3b1986 100644 --- a/includes/filebackend/lockmanager/LockManager.php +++ b/includes/filebackend/lockmanager/LockManager.php @@ -103,17 +103,17 @@ abstract class LockManager { */ final public function lockByType( array $pathsByType, $timeout = 0 ) { $pathsByType = $this->normalizePathsByType( $pathsByType ); - $msleep = [ 0, 50, 100, 300, 500 ]; // retry backoff times - $start = microtime( true ); - do { - $status = $this->doLockByType( $pathsByType ); - $elapsed = microtime( true ) - $start; - if ( $status->isOK() || $elapsed >= $timeout || $elapsed < 0 ) { - break; // success, timeout, or clock set back - } - usleep( 1e3 * ( next( $msleep ) ?: 1000 ) ); // use 1 sec after enough times - $elapsed = microtime( true ) - $start; - } while ( $elapsed < $timeout && $elapsed >= 0 ); + + $status = null; + $loop = new WaitConditionLoop( + function () use ( &$status, $pathsByType ) { + $status = $this->doLockByType( $pathsByType ); + + return $status->isOK() ?: WaitConditionLoop::CONDITION_CONTINUE; + }, + $timeout + ); + $loop->invoke(); return $status; } diff --git a/includes/filebackend/lockmanager/MemcLockManager.php b/includes/filebackend/lockmanager/MemcLockManager.php index cb5266acd8..2f17e2722c 100644 --- a/includes/filebackend/lockmanager/MemcLockManager.php +++ b/includes/filebackend/lockmanager/MemcLockManager.php @@ -276,6 +276,7 @@ class MemcLockManager extends QuorumLockManager { * @return MemcachedBagOStuff|null */ protected function getCache( $lockSrv ) { + /** @var BagOStuff $memc */ $memc = null; if ( isset( $this->bagOStuffs[$lockSrv] ) ) { $memc = $this->bagOStuffs[$lockSrv]; @@ -337,20 +338,21 @@ class MemcLockManager extends QuorumLockManager { // Try to quickly loop to acquire the keys, but back off after a few rounds. // This reduces memcached spam, especially in the rare case where a server acquires // some lock keys and dies without releasing them. Lock keys expire after a few minutes. - $rounds = 0; - $start = microtime( true ); - do { - if ( ( ++$rounds % 4 ) == 0 ) { - usleep( 1000 * 50 ); // 50 ms - } - foreach ( array_diff( $keys, $lockedKeys ) as $key ) { - if ( $memc->add( "$key:mutex", 1, 180 ) ) { // lock record - $lockedKeys[] = $key; - } else { - continue; // acquire in order + $loop = new WaitConditionLoop( + function () use ( $memc, $keys, &$lockedKeys ) { + foreach ( array_diff( $keys, $lockedKeys ) as $key ) { + if ( $memc->add( "$key:mutex", 1, 180 ) ) { // lock record + $lockedKeys[] = $key; + } } - } - } while ( count( $lockedKeys ) < count( $keys ) && ( microtime( true ) - $start ) <= 3 ); + + return array_diff( $keys, $lockedKeys ) + ? WaitConditionLoop::CONDITION_CONTINUE + : true; + }, + 3.0 // timeout + ); + $loop->invoke(); if ( count( $lockedKeys ) != count( $keys ) ) { $this->releaseMutexes( $memc, $lockedKeys ); // failed; release what was locked diff --git a/includes/libs/WaitConditionLoop.php b/includes/libs/WaitConditionLoop.php index 3cc6236893..3339eb3108 100644 --- a/includes/libs/WaitConditionLoop.php +++ b/includes/libs/WaitConditionLoop.php @@ -37,8 +37,9 @@ class WaitConditionLoop { const CONDITION_REACHED = 1; const CONDITION_CONTINUE = 0; // evaluates as falsey - const CONDITION_TIMED_OUT = -1; - const CONDITION_ABORTED = -2; + const CONDITION_FAILED = -1; + const CONDITION_TIMED_OUT = -2; + const CONDITION_ABORTED = -3; /** * @param callable $condition Callback that returns a WaitConditionLoop::CONDITION_ constant @@ -53,11 +54,14 @@ class WaitConditionLoop { /** * Invoke the loop and continue until either: - * - a) The condition callback does not return either CONDITION_CONTINUE or true + * - a) The condition callback returns neither CONDITION_CONTINUE nor false * - b) The timeout is reached * This a condition callback can return true (stop) or false (continue) for convenience. * In such cases, the halting result of "true" will be converted to CONDITION_REACHED. * + * If $timeout is 0, then only the condition callback will be called (no busy callbacks), + * and this will immediately return CONDITION_FAILED if the condition was not met. + * * Exceptions in callbacks will be caught and the callback will be swapped with * one that simply rethrows that exception back to the caller when invoked. * @@ -77,12 +81,19 @@ class WaitConditionLoop { $checkResult = call_user_func( $this->condition ); $cpu = $this->getCpuTime() - $cpuStart; $real = $this->getWallTime() - $realStart; - // Exit if the condition is reached - if ( (int)$checkResult !== self::CONDITION_CONTINUE ) { - $finalResult = is_int( $checkResult ) ? $checkResult : self::CONDITION_REACHED; + // Exit if the condition is reached, and error occurs, or this is non-blocking + if ( $this->timeout <= 0 ) { + $finalResult = $checkResult ? self::CONDITION_REACHED : self::CONDITION_FAILED; + break; + } elseif ( (int)$checkResult !== self::CONDITION_CONTINUE ) { + if ( is_int( $checkResult ) ) { + $finalResult = $checkResult; + } else { + $finalResult = self::CONDITION_REACHED; + } break; } elseif ( $lastCheck ) { - break; // timeout + break; // timeout reached } // Detect if condition callback seems to block or if justs burns CPU $conditionUsesInterrupts = ( $real > 0.100 && $cpu <= $real * .03 ); diff --git a/includes/libs/objectcache/BagOStuff.php b/includes/libs/objectcache/BagOStuff.php index a679be8f0c..cd79b67136 100644 --- a/includes/libs/objectcache/BagOStuff.php +++ b/includes/libs/objectcache/BagOStuff.php @@ -410,35 +410,21 @@ abstract class BagOStuff implements IExpiringStore, LoggerAwareInterface { } $expiry = min( $expiry ?: INF, self::TTL_DAY ); - - $this->clearLastError(); - $timestamp = microtime( true ); // starting UNIX timestamp - if ( $this->add( "{$key}:lock", 1, $expiry ) ) { - $locked = true; - } elseif ( $this->getLastError() || $timeout <= 0 ) { - $locked = false; // network partition or non-blocking - } else { - // Estimate the RTT (us); use 1ms minimum for sanity - $uRTT = max( 1e3, ceil( 1e6 * ( microtime( true ) - $timestamp ) ) ); - $sleep = 2 * $uRTT; // rough time to do get()+set() - - $attempts = 0; // failed attempts - do { - if ( ++$attempts >= 3 && $sleep <= 5e5 ) { - // Exponentially back off after failed attempts to avoid network spam. - // About 2*$uRTT*(2^n-1) us of "sleep" happen for the next n attempts. - $sleep *= 2; - } - usleep( $sleep ); // back off + $loop = new WaitConditionLoop( + function () use ( $key, $timeout, $expiry ) { $this->clearLastError(); - $locked = $this->add( "{$key}:lock", 1, $expiry ); - if ( $this->getLastError() ) { - $locked = false; // network partition - break; + if ( $this->add( "{$key}:lock", 1, $expiry ) ) { + return true; // locked! + } elseif ( $this->getLastError() ) { + return WaitConditionLoop::CONDITION_ABORTED; // network partition? } - } while ( !$locked && ( microtime( true ) - $timestamp ) < $timeout ); - } + return WaitConditionLoop::CONDITION_CONTINUE; + }, + $timeout + ); + + $locked = ( $loop->invoke() === $loop::CONDITION_REACHED ); if ( $locked ) { $this->locks[$key] = [ 'class' => $rclass, 'depth' => 1 ]; } diff --git a/tests/phpunit/includes/libs/WaitConditionLoopTest.php b/tests/phpunit/includes/libs/WaitConditionLoopTest.php index b75adca99e..9ce93d6291 100644 --- a/tests/phpunit/includes/libs/WaitConditionLoopTest.php +++ b/tests/phpunit/includes/libs/WaitConditionLoopTest.php @@ -102,6 +102,31 @@ class WaitConditionLoopTest extends PHPUnit_Framework_TestCase { $loop->setWallClock( $wallClock ); $this->assertEquals( $loop::CONDITION_TIMED_OUT, $loop->invoke() ); $this->assertEquals( [ 1, 1, 1 ], [ $x, $y, $z ], "Busy work done" ); + + $loop = new WaitConditionLoopFakeTime( + function () use ( &$count, &$wallClock ) { + $wallClock += 3; + ++$count; + + return true; + }, + 0.0, + $this->newBusyWork( $x, $y, $z, $wallClock ) + ); + $this->assertEquals( $loop::CONDITION_REACHED, $loop->invoke() ); + + $count = 0; + $loop = new WaitConditionLoopFakeTime( + function () use ( &$count, &$wallClock ) { + $wallClock += 3; + ++$count; + + return $count > 10 ? true : false; + }, + 0, + $this->newBusyWork( $x, $y, $z, $wallClock ) + ); + $this->assertEquals( $loop::CONDITION_FAILED, $loop->invoke() ); } public function testCallbackAborted() { -- 2.20.1