From d5aa846d84e33967f086ef90a532243823b7b4a3 Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Fri, 8 Dec 2017 13:36:25 -0800 Subject: [PATCH] Replace cpPosTime cookie/parameter with cpPosIndex This handles multi-DB transactions properly, instead of causing wait timeouts in the WaitConditionLoop. It also is more correct in using a counter instead of relying on wall clocks. In addition: * Refactor related code in MediaWiki.php to be comprehensible. * Always send the cookie even the "remote wiki redirect" case. * Renamed ChronologyProtector field and constant to avoid any confusion of "wait for server X to reach Y" with "wait for Y to show up in position store". * Add an "asOfTime" field to the position keys for debugging. Bug: T182322 Change-Id: I5c73cd07eaf664f02ba00c38fab9f49b609f4284 --- includes/MediaWiki.php | 78 ++++++------ includes/Setup.php | 10 +- includes/libs/rdbms/ChronologyProtector.php | 99 ++++++++-------- includes/libs/rdbms/lbfactory/ILBFactory.php | 9 +- includes/libs/rdbms/lbfactory/LBFactory.php | 19 +-- tests/phpunit/includes/db/LBFactoryTest.php | 118 ++++++++++++++----- 6 files changed, 196 insertions(+), 137 deletions(-) diff --git a/includes/MediaWiki.php b/includes/MediaWiki.php index beb9de5d21..b1b1a7aee7 100644 --- a/includes/MediaWiki.php +++ b/includes/MediaWiki.php @@ -603,50 +603,54 @@ class MediaWiki { DeferredUpdates::doUpdates( 'enqueue', DeferredUpdates::PRESEND ); wfDebug( __METHOD__ . ': pre-send deferred updates completed' ); - // Decide when clients block on ChronologyProtector DB position writes - $urlDomainDistance = ( - $request->wasPosted() && - $output->getRedirect() && - $lbFactory->hasOrMadeRecentMasterChanges( INF ) - ) ? self::getUrlDomainDistance( $output->getRedirect() ) : false; + // Should the client return, their request should observe the new ChronologyProtector + // DB positions. This request might be on a foreign wiki domain, so synchronously update + // the DB positions in all datacenters to be safe. If this output is not a redirect, + // then OutputPage::output() will be relatively slow, meaning that running it in + // $postCommitWork should help mask the latency of those updates. + $flags = $lbFactory::SHUTDOWN_CHRONPROT_SYNC; + $strategy = 'cookie+sync'; $allowHeaders = !( $output->isDisabled() || headers_sent() ); - if ( $urlDomainDistance === 'local' || $urlDomainDistance === 'remote' ) { - // OutputPage::output() will be fast; $postCommitWork will not be useful for - // masking the latency of syncing DB positions accross all datacenters synchronously. - // Instead, make use of the RTT time of the client follow redirects. - $flags = $lbFactory::SHUTDOWN_CHRONPROT_ASYNC; - $cpPosTime = microtime( true ); - // Client's next request should see 1+ positions with this DBMasterPos::asOf() time - if ( $urlDomainDistance === 'local' && $allowHeaders ) { - // Client will stay on this domain, so set an unobtrusive cookie - $expires = time() + ChronologyProtector::POSITION_TTL; - $options = [ 'prefix' => '' ]; - $request->response()->setCookie( 'cpPosTime', $cpPosTime, $expires, $options ); - } else { - // Cookies may not work across wiki domains, so use a URL parameter - $safeUrl = $lbFactory->appendPreShutdownTimeAsQuery( - $output->getRedirect(), - $cpPosTime - ); - $output->redirect( $safeUrl ); + if ( $output->getRedirect() && $lbFactory->hasOrMadeRecentMasterChanges( INF ) ) { + // OutputPage::output() will be fast, so $postCommitWork is useless for masking + // the latency of synchronously updating the DB positions in all datacenters. + // Try to make use of the time the client spends following redirects instead. + $domainDistance = self::getUrlDomainDistance( $output->getRedirect() ); + if ( $domainDistance === 'local' && $allowHeaders ) { + $flags = $lbFactory::SHUTDOWN_CHRONPROT_ASYNC; + $strategy = 'cookie'; // use same-domain cookie and keep the URL uncluttered + } elseif ( $domainDistance === 'remote' ) { + $flags = $lbFactory::SHUTDOWN_CHRONPROT_ASYNC; + $strategy = 'cookie+url'; // cross-domain cookie might not work } - } else { - // OutputPage::output() is fairly slow; run it in $postCommitWork to mask - // the latency of syncing DB positions accross all datacenters synchronously - $flags = $lbFactory::SHUTDOWN_CHRONPROT_SYNC; - if ( $lbFactory->hasOrMadeRecentMasterChanges( INF ) && $allowHeaders ) { - $cpPosTime = microtime( true ); - // Set a cookie in case the DB position store cannot sync accross datacenters. - // This will at least cover the common case of the user staying on the domain. + } + + // Record ChronologyProtector positions for DBs affected in this request at this point + $cpIndex = null; + $lbFactory->shutdown( $flags, $postCommitWork, $cpIndex ); + wfDebug( __METHOD__ . ': LBFactory shutdown completed' ); + + if ( $cpIndex > 0 ) { + if ( $allowHeaders ) { $expires = time() + ChronologyProtector::POSITION_TTL; $options = [ 'prefix' => '' ]; - $request->response()->setCookie( 'cpPosTime', $cpPosTime, $expires, $options ); + $request->response()->setCookie( 'cpPosIndex', $cpIndex, $expires, $options ); + } + + if ( $strategy === 'cookie+url' ) { + if ( $output->getRedirect() ) { // sanity + $safeUrl = $lbFactory->appendShutdownCPIndexAsQuery( + $output->getRedirect(), + $cpIndex + ); + $output->redirect( $safeUrl ); + } else { + $e = new LogicException( "No redirect; cannot append cpPosIndex parameter." ); + MWExceptionHandler::logException( $e ); + } } } - // Record ChronologyProtector positions for DBs affected in this request at this point - $lbFactory->shutdown( $flags, $postCommitWork ); - wfDebug( __METHOD__ . ': LBFactory shutdown completed' ); // Set a cookie to tell all CDN edge nodes to "stick" the user to the DC that handles this // POST request (e.g. the "master" data center). Also have the user briefly bypass CDN so diff --git a/includes/Setup.php b/includes/Setup.php index 4936b0b11a..d1f225b5fc 100644 --- a/includes/Setup.php +++ b/includes/Setup.php @@ -736,19 +736,19 @@ if ( !$wgDBerrorLogTZ ) { // Initialize the request object in $wgRequest $wgRequest = RequestContext::getMain()->getRequest(); // BackCompat // Set user IP/agent information for causal consistency purposes. -// The cpPosTime cookie has no prefix and is set by MediaWiki::preOutputCommit(). -$cpPosTime = $wgRequest->getFloat( 'cpPosTime', $wgRequest->getCookie( 'cpPosTime', '' ) ); +// The cpPosIndex cookie has no prefix and is set by MediaWiki::preOutputCommit(). +$cpPosIndex = $wgRequest->getInt( 'cpPosIndex', (int)$wgRequest->getCookie( 'cpPosIndex', '' ) ); MediaWikiServices::getInstance()->getDBLoadBalancerFactory()->setRequestInfo( [ 'IPAddress' => $wgRequest->getIP(), 'UserAgent' => $wgRequest->getHeader( 'User-Agent' ), 'ChronologyProtection' => $wgRequest->getHeader( 'ChronologyProtection' ), - 'ChronologyPositionTime' => $cpPosTime + 'ChronologyPositionIndex' => $cpPosIndex ] ); // Make sure that caching does not compromise the consistency improvements -if ( $cpPosTime ) { +if ( $cpPosIndex ) { MediaWikiServices::getInstance()->getMainWANObjectCache()->useInterimHoldOffCaching( false ); } -unset( $cpPosTime ); +unset( $cpPosIndex ); // Useful debug output if ( $wgCommandLineMode ) { diff --git a/includes/libs/rdbms/ChronologyProtector.php b/includes/libs/rdbms/ChronologyProtector.php index 8121654e33..152ff74d57 100644 --- a/includes/libs/rdbms/ChronologyProtector.php +++ b/includes/libs/rdbms/ChronologyProtector.php @@ -43,10 +43,10 @@ class ChronologyProtector implements LoggerAwareInterface { protected $key; /** @var string Hash of client parameters */ protected $clientId; - /** @var float|null Minimum UNIX timestamp of 1+ expected startup positions */ - protected $waitForPosTime; + /** @var int|null Expected minimum index of the last write to the position store */ + protected $waitForPosIndex; /** @var int Max seconds to wait on positions to appear */ - protected $waitForPosTimeout = self::POS_WAIT_TIMEOUT; + protected $waitForPosStoreTimeout = self::POS_STORE_WAIT_TIMEOUT; /** @var bool Whether to no-op all method calls */ protected $enabled = true; /** @var bool Whether to check and wait on positions */ @@ -64,19 +64,19 @@ class ChronologyProtector implements LoggerAwareInterface { /** @var int Seconds to store positions */ const POSITION_TTL = 60; /** @var int Max time to wait for positions to appear */ - const POS_WAIT_TIMEOUT = 5; + const POS_STORE_WAIT_TIMEOUT = 5; /** * @param BagOStuff $store - * @param array $client Map of (ip: , agent: ) - * @param float $posTime UNIX timestamp + * @param array[] $client Map of (ip: , agent: ) + * @param int|null $posIndex Write counter index [optional] * @since 1.27 */ - public function __construct( BagOStuff $store, array $client, $posTime = null ) { + public function __construct( BagOStuff $store, array $client, $posIndex = null ) { $this->store = $store; $this->clientId = md5( $client['ip'] . "\n" . $client['agent'] ); $this->key = $store->makeGlobalKey( __CLASS__, $this->clientId, 'v1' ); - $this->waitForPosTime = $posTime; + $this->waitForPosIndex = $posIndex; $this->logger = new NullLogger(); } @@ -161,9 +161,10 @@ class ChronologyProtector implements LoggerAwareInterface { * * @param callable|null $workCallback Work to do instead of waiting on syncing positions * @param string $mode One of (sync, async); whether to wait on remote datacenters + * @param int|null &$cpIndex DB position key write counter; incremented on update * @return DBMasterPos[] Empty on success; returns the (db name => position) map on failure */ - public function shutdown( callable $workCallback = null, $mode = 'sync' ) { + public function shutdown( callable $workCallback = null, $mode = 'sync', &$cpIndex = null ) { if ( !$this->enabled ) { return []; } @@ -198,13 +199,18 @@ class ChronologyProtector implements LoggerAwareInterface { } $ok = $store->set( $this->key, - self::mergePositions( $store->get( $this->key ), $this->shutdownPositions ), + $this->mergePositions( + $store->get( $this->key ), + $this->shutdownPositions, + $cpIndex + ), self::POSITION_TTL, ( $mode === 'sync' ) ? $store::WRITE_SYNC : 0 ); $store->unlock( $this->key ); } else { $ok = false; + $cpIndex = null; // nothing saved } if ( !$ok ) { @@ -254,28 +260,36 @@ class ChronologyProtector implements LoggerAwareInterface { $this->initialized = true; if ( $this->wait ) { - // If there is an expectation to see master positions with a certain min - // timestamp, then block until they appear, or until a timeout is reached. - if ( $this->waitForPosTime > 0.0 ) { + // If there is an expectation to see master positions from a certain write + // index or higher, then block until it appears, or until a timeout is reached. + // Since the write index restarts each time the key is created, it is possible that + // a lagged store has a matching key write index. However, in that case, it should + // already be expired and thus treated as non-existing, maintaining correctness. + if ( $this->waitForPosIndex > 0 ) { $data = null; $loop = new WaitConditionLoop( function () use ( &$data ) { $data = $this->store->get( $this->key ); + if ( !is_array( $data ) ) { + return WaitConditionLoop::CONDITION_CONTINUE; // not found yet + } elseif ( !isset( $data['writeIndex'] ) ) { + return WaitConditionLoop::CONDITION_REACHED; // b/c + } - return ( self::minPosTime( $data ) >= $this->waitForPosTime ) + return ( $data['writeIndex'] >= $this->waitForPosIndex ) ? WaitConditionLoop::CONDITION_REACHED : WaitConditionLoop::CONDITION_CONTINUE; }, - $this->waitForPosTimeout + $this->waitForPosStoreTimeout ); $result = $loop->invoke(); $waitedMs = $loop->getLastWaitTime() * 1e3; if ( $result == $loop::CONDITION_REACHED ) { - $msg = "expected and found pos time {$this->waitForPosTime} ({$waitedMs}ms)"; + $msg = "expected and found pos index {$this->waitForPosIndex} ({$waitedMs}ms)"; $this->logger->debug( $msg ); } else { - $msg = "expected but missed pos time {$this->waitForPosTime} ({$waitedMs}ms)"; + $msg = "expected but missed pos index {$this->waitForPosIndex} ({$waitedMs}ms)"; $this->logger->info( $msg ); } } else { @@ -290,48 +304,31 @@ class ChronologyProtector implements LoggerAwareInterface { } } - /** - * @param array|bool $data - * @return float|null - */ - private static function minPosTime( $data ) { - if ( !isset( $data['positions'] ) ) { - return null; - } - - $min = null; - foreach ( $data['positions'] as $pos ) { - if ( $pos instanceof DBMasterPos ) { - $min = $min ? min( $pos->asOfTime(), $min ) : $pos->asOfTime(); - } - } - - return $min; - } - /** * @param array|bool $curValue * @param DBMasterPos[] $shutdownPositions + * @param int|null &$cpIndex * @return array */ - private static function mergePositions( $curValue, array $shutdownPositions ) { + protected function mergePositions( $curValue, array $shutdownPositions, &$cpIndex = null ) { /** @var DBMasterPos[] $curPositions */ - if ( $curValue === false ) { - $curPositions = $shutdownPositions; - } else { - $curPositions = $curValue['positions']; - // Use the newest positions for each DB master - foreach ( $shutdownPositions as $db => $pos ) { - if ( - !isset( $curPositions[$db] ) || - !( $curPositions[$db] instanceof DBMasterPos ) || - $pos->asOfTime() > $curPositions[$db]->asOfTime() - ) { - $curPositions[$db] = $pos; - } + $curPositions = isset( $curValue['positions'] ) ? $curValue['positions'] : []; + // Use the newest positions for each DB master + foreach ( $shutdownPositions as $db => $pos ) { + if ( + !isset( $curPositions[$db] ) || + !( $curPositions[$db] instanceof DBMasterPos ) || + $pos->asOfTime() > $curPositions[$db]->asOfTime() + ) { + $curPositions[$db] = $pos; } } - return [ 'positions' => $curPositions ]; + $cpIndex = isset( $curValue['writeIndex'] ) ? $curValue['writeIndex'] : 0; + + return [ + 'positions' => $curPositions, + 'writeIndex' => ++$cpIndex + ]; } } diff --git a/includes/libs/rdbms/lbfactory/ILBFactory.php b/includes/libs/rdbms/lbfactory/ILBFactory.php index 697af0ed76..98108a7df2 100644 --- a/includes/libs/rdbms/lbfactory/ILBFactory.php +++ b/includes/libs/rdbms/lbfactory/ILBFactory.php @@ -140,9 +140,10 @@ interface ILBFactory { * Prepare all tracked load balancers for shutdown * @param int $mode One of the class SHUTDOWN_* constants * @param callable|null $workCallback Work to mask ChronologyProtector writes + * @param int|null &$cpIndex Position key write counter for ChronologyProtector */ public function shutdown( - $mode = self::SHUTDOWN_CHRONPROT_SYNC, callable $workCallback = null + $mode = self::SHUTDOWN_CHRONPROT_SYNC, callable $workCallback = null, &$cpIndex = null ); /** @@ -304,7 +305,7 @@ interface ILBFactory { public function setAgentName( $agent ); /** - * Append ?cpPosTime parameter to a URL for ChronologyProtector purposes if needed + * Append ?cpPosIndex parameter to a URL for ChronologyProtector purposes if needed * * Note that unlike cookies, this works accross domains * @@ -312,14 +313,14 @@ interface ILBFactory { * @param float $time UNIX timestamp just before shutdown() was called * @return string */ - public function appendPreShutdownTimeAsQuery( $url, $time ); + public function appendShutdownCPIndexAsQuery( $url, $time ); /** * @param array $info Map of fields, including: * - IPAddress : IP address * - UserAgent : User-Agent HTTP header * - ChronologyProtection : cookie/header value specifying ChronologyProtector usage - * - ChronologyPositionTime: timestamp used to get up-to-date DB positions for the agent + * - ChronologyPositionIndex: timestamp used to get up-to-date DB positions for the agent */ public function setRequestInfo( array $info ); } diff --git a/includes/libs/rdbms/lbfactory/LBFactory.php b/includes/libs/rdbms/lbfactory/LBFactory.php index ef716b68a6..187527e557 100644 --- a/includes/libs/rdbms/lbfactory/LBFactory.php +++ b/includes/libs/rdbms/lbfactory/LBFactory.php @@ -116,7 +116,7 @@ abstract class LBFactory implements ILBFactory { 'IPAddress' => isset( $_SERVER[ 'REMOTE_ADDR' ] ) ? $_SERVER[ 'REMOTE_ADDR' ] : '', 'UserAgent' => isset( $_SERVER['HTTP_USER_AGENT'] ) ? $_SERVER['HTTP_USER_AGENT'] : '', 'ChronologyProtection' => 'true', - 'ChronologyPositionTime' => isset( $_GET['cpPosTime'] ) ? $_GET['cpPosTime'] : null + 'ChronologyPositionIndex' => isset( $_GET['cpPosIndex'] ) ? $_GET['cpPosIndex'] : null ]; $this->cliMode = isset( $conf['cliMode'] ) ? $conf['cliMode'] : PHP_SAPI === 'cli'; @@ -132,13 +132,13 @@ abstract class LBFactory implements ILBFactory { } public function shutdown( - $mode = self::SHUTDOWN_CHRONPROT_SYNC, callable $workCallback = null + $mode = self::SHUTDOWN_CHRONPROT_SYNC, callable $workCallback = null, &$cpIndex = null ) { $chronProt = $this->getChronologyProtector(); if ( $mode === self::SHUTDOWN_CHRONPROT_SYNC ) { - $this->shutdownChronologyProtector( $chronProt, $workCallback, 'sync' ); + $this->shutdownChronologyProtector( $chronProt, $workCallback, 'sync', $cpIndex ); } elseif ( $mode === self::SHUTDOWN_CHRONPROT_ASYNC ) { - $this->shutdownChronologyProtector( $chronProt, null, 'async' ); + $this->shutdownChronologyProtector( $chronProt, null, 'async', $cpIndex ); } $this->commitMasterChanges( __METHOD__ ); // sanity @@ -441,7 +441,7 @@ abstract class LBFactory implements ILBFactory { 'ip' => $this->requestInfo['IPAddress'], 'agent' => $this->requestInfo['UserAgent'], ], - $this->requestInfo['ChronologyPositionTime'] + $this->requestInfo['ChronologyPositionIndex'] ); $this->chronProt->setLogger( $this->replLogger ); @@ -465,9 +465,10 @@ abstract class LBFactory implements ILBFactory { * @param ChronologyProtector $cp * @param callable|null $workCallback Work to do instead of waiting on syncing positions * @param string $mode One of (sync, async); whether to wait on remote datacenters + * @param int|null &$cpIndex DB position key write counter; incremented on update */ protected function shutdownChronologyProtector( - ChronologyProtector $cp, $workCallback, $mode + ChronologyProtector $cp, $workCallback, $mode, &$cpIndex = null ) { // Record all the master positions needed $this->forEachLB( function ( ILoadBalancer $lb ) use ( $cp ) { @@ -475,7 +476,7 @@ abstract class LBFactory implements ILBFactory { } ); // Write them to the persistent stash. Try to do something useful by running $work // while ChronologyProtector waits for the stash write to replicate to all DCs. - $unsavedPositions = $cp->shutdown( $workCallback, $mode ); + $unsavedPositions = $cp->shutdown( $workCallback, $mode, $cpIndex ); if ( $unsavedPositions && $workCallback ) { // Invoke callback in case it did not cache the result yet $workCallback(); // work now to block for less time in waitForAll() @@ -544,7 +545,7 @@ abstract class LBFactory implements ILBFactory { $this->agent = $agent; } - public function appendPreShutdownTimeAsQuery( $url, $time ) { + public function appendShutdownCPIndexAsQuery( $url, $index ) { $usedCluster = 0; $this->forEachLB( function ( ILoadBalancer $lb ) use ( &$usedCluster ) { $usedCluster |= ( $lb->getServerCount() > 1 ); @@ -554,7 +555,7 @@ abstract class LBFactory implements ILBFactory { return $url; // no master/replica clusters touched } - return strpos( $url, '?' ) === false ? "$url?cpPosTime=$time" : "$url&cpPosTime=$time"; + return strpos( $url, '?' ) === false ? "$url?cpPosIndex=$index" : "$url&cpPosIndex=$index"; } public function setRequestInfo( array $info ) { diff --git a/tests/phpunit/includes/db/LBFactoryTest.php b/tests/phpunit/includes/db/LBFactoryTest.php index 75d54146e1..e46a0ddbf9 100644 --- a/tests/phpunit/includes/db/LBFactoryTest.php +++ b/tests/phpunit/includes/db/LBFactoryTest.php @@ -192,33 +192,61 @@ class LBFactoryTest extends MediaWikiTestCase { */ public function testChronologyProtector() { // (a) First HTTP request - $mPos = new MySQLMasterPos( 'db1034-bin.000976', '843431247' ); + $m1Pos = new MySQLMasterPos( 'db1034-bin.000976', '843431247' ); + $m2Pos = new MySQLMasterPos( 'db1064-bin.002400', '794074907' ); $now = microtime( true ); - $mockDB = $this->getMockBuilder( 'DatabaseMysqli' ) + + // Master DB 1 + $mockDB1 = $this->getMockBuilder( 'DatabaseMysqli' ) ->disableOriginalConstructor() ->getMock(); - $mockDB->method( 'writesOrCallbacksPending' )->willReturn( true ); - $mockDB->method( 'lastDoneWrites' )->willReturn( $now ); - $mockDB->method( 'getMasterPos' )->willReturn( $mPos ); - - $lb = $this->getMockBuilder( 'LoadBalancer' ) + $mockDB1->method( 'writesOrCallbacksPending' )->willReturn( true ); + $mockDB1->method( 'lastDoneWrites' )->willReturn( $now ); + $mockDB1->method( 'getMasterPos' )->willReturn( $m1Pos ); + // Load balancer for master DB 1 + $lb1 = $this->getMockBuilder( 'LoadBalancer' ) ->disableOriginalConstructor() ->getMock(); - $lb->method( 'getConnection' )->willReturn( $mockDB ); - $lb->method( 'getServerCount' )->willReturn( 2 ); - $lb->method( 'parentInfo' )->willReturn( [ 'id' => "main-DEFAULT" ] ); - $lb->method( 'getAnyOpenConnection' )->willReturn( $mockDB ); - $lb->method( 'hasOrMadeRecentMasterChanges' )->will( $this->returnCallback( - function () use ( $mockDB ) { + $lb1->method( 'getConnection' )->willReturn( $mockDB1 ); + $lb1->method( 'getServerCount' )->willReturn( 2 ); + $lb1->method( 'getAnyOpenConnection' )->willReturn( $mockDB1 ); + $lb1->method( 'hasOrMadeRecentMasterChanges' )->will( $this->returnCallback( + function () use ( $mockDB1 ) { $p = 0; - $p |= call_user_func( [ $mockDB, 'writesOrCallbacksPending' ] ); - $p |= call_user_func( [ $mockDB, 'lastDoneWrites' ] ); + $p |= call_user_func( [ $mockDB1, 'writesOrCallbacksPending' ] ); + $p |= call_user_func( [ $mockDB1, 'lastDoneWrites' ] ); return (bool)$p; } ) ); - $lb->method( 'getMasterPos' )->willReturn( $mPos ); + $lb1->method( 'getMasterPos' )->willReturn( $m1Pos ); + $lb1->method( 'getServerName' )->with( 0 )->willReturn( 'master1' ); + // Master DB 2 + $mockDB2 = $this->getMockBuilder( 'DatabaseMysqli' ) + ->disableOriginalConstructor() + ->getMock(); + $mockDB2->method( 'writesOrCallbacksPending' )->willReturn( true ); + $mockDB2->method( 'lastDoneWrites' )->willReturn( $now ); + $mockDB2->method( 'getMasterPos' )->willReturn( $m2Pos ); + // Load balancer for master DB 2 + $lb2 = $this->getMockBuilder( 'LoadBalancer' ) + ->disableOriginalConstructor() + ->getMock(); + $lb2->method( 'getConnection' )->willReturn( $mockDB2 ); + $lb2->method( 'getServerCount' )->willReturn( 2 ); + $lb2->method( 'getAnyOpenConnection' )->willReturn( $mockDB2 ); + $lb2->method( 'hasOrMadeRecentMasterChanges' )->will( $this->returnCallback( + function () use ( $mockDB2 ) { + $p = 0; + $p |= call_user_func( [ $mockDB2, 'writesOrCallbacksPending' ] ); + $p |= call_user_func( [ $mockDB2, 'lastDoneWrites' ] ); + + return (bool)$p; + } + ) ); + $lb2->method( 'getMasterPos' )->willReturn( $m2Pos ); + $lb2->method( 'getServerName' )->with( 0 )->willReturn( 'master2' ); $bag = new HashBagOStuff(); $cp = new ChronologyProtector( @@ -229,32 +257,60 @@ class LBFactoryTest extends MediaWikiTestCase { ] ); - $mockDB->expects( $this->exactly( 2 ) )->method( 'writesOrCallbacksPending' ); - $mockDB->expects( $this->exactly( 2 ) )->method( 'lastDoneWrites' ); + $mockDB1->expects( $this->exactly( 1 ) )->method( 'writesOrCallbacksPending' ); + $mockDB1->expects( $this->exactly( 1 ) )->method( 'lastDoneWrites' ); + $mockDB2->expects( $this->exactly( 1 ) )->method( 'writesOrCallbacksPending' ); + $mockDB2->expects( $this->exactly( 1 ) )->method( 'lastDoneWrites' ); + + // Nothing to wait for on first HTTP request start + $cp->initLB( $lb1 ); + $cp->initLB( $lb2 ); + // Record positions in stash on first HTTP request end + $cp->shutdownLB( $lb1 ); + $cp->shutdownLB( $lb2 ); + $cpIndex = null; + $cp->shutdown( null, 'sync', $cpIndex ); - // Nothing to wait for - $cp->initLB( $lb ); - // Record in stash - $cp->shutdownLB( $lb ); - $cp->shutdown(); + $this->assertEquals( 1, $cpIndex, "CP write index set" ); // (b) Second HTTP request + + // Load balancer for master DB 1 + $lb1 = $this->getMockBuilder( 'LoadBalancer' ) + ->disableOriginalConstructor() + ->getMock(); + $lb1->method( 'getServerCount' )->willReturn( 2 ); + $lb1->method( 'getServerName' )->with( 0 )->willReturn( 'master1' ); + $lb1->expects( $this->once() ) + ->method( 'waitFor' )->with( $this->equalTo( $m1Pos ) ); + // Load balancer for master DB 2 + $lb2 = $this->getMockBuilder( 'LoadBalancer' ) + ->disableOriginalConstructor() + ->getMock(); + $lb2->method( 'getServerCount' )->willReturn( 2 ); + $lb2->method( 'getServerName' )->with( 0 )->willReturn( 'master2' ); + $lb2->expects( $this->once() ) + ->method( 'waitFor' )->with( $this->equalTo( $m2Pos ) ); + $cp = new ChronologyProtector( $bag, [ 'ip' => '127.0.0.1', 'agent' => "Totally-Not-FireFox" - ] + ], + $cpIndex ); - $lb->expects( $this->once() ) - ->method( 'waitFor' )->with( $this->equalTo( $mPos ) ); + // Wait for last positions to be reached on second HTTP request start + $cp->initLB( $lb1 ); + $cp->initLB( $lb2 ); + // Shutdown (nothing to record) + $cp->shutdownLB( $lb1 ); + $cp->shutdownLB( $lb2 ); + $cpIndex = null; + $cp->shutdown( null, 'sync', $cpIndex ); - // Wait - $cp->initLB( $lb ); - // Record in stash - $cp->shutdownLB( $lb ); - $cp->shutdown(); + $this->assertEquals( null, $cpIndex, "CP write index retained" ); } private function newLBFactoryMulti( array $baseOverride = [], array $serverOverride = [] ) { -- 2.20.1