From 17de18691706ae390b8391819d332d881beca604 Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Wed, 22 May 2019 11:00:55 -0700 Subject: [PATCH] rdbms: fix ChronologyProtector client IDs to not be empty Follow up to 8107fddd8fe3d5 Also improve debug logging and add some unit tests Bug: T223310 Bug: T223978 Change-Id: I35484385e4da2912bc10f5e8d2fb07cb1097347e --- includes/libs/rdbms/ChronologyProtector.php | 58 +++++++------ includes/libs/rdbms/lbfactory/LBFactory.php | 6 +- tests/phpunit/includes/db/LBFactoryTest.php | 16 ++-- .../libs/rdbms/ChronologyProtectorTest.php | 81 +++++++++++++++++++ 4 files changed, 127 insertions(+), 34 deletions(-) create mode 100644 tests/phpunit/includes/libs/rdbms/ChronologyProtectorTest.php diff --git a/includes/libs/rdbms/ChronologyProtector.php b/includes/libs/rdbms/ChronologyProtector.php index fa454c88d1..88bc049386 100644 --- a/includes/libs/rdbms/ChronologyProtector.php +++ b/includes/libs/rdbms/ChronologyProtector.php @@ -30,7 +30,10 @@ use Wikimedia\WaitConditionLoop; use BagOStuff; /** - * Class for ensuring a consistent ordering of events as seen by the user, despite replication. + * Helper class for mitigating DB replication lag in order to provide "session consistency" + * + * This helps to ensure a consistent ordering of events as seen by an client + * * Kind of like Hawking's [[Chronology Protection Agency]]. */ class ChronologyProtector implements LoggerAwareInterface { @@ -82,7 +85,7 @@ class ChronologyProtector implements LoggerAwareInterface { if ( isset( $client['clientId'] ) ) { $this->clientId = $client['clientId']; } else { - $this->clientId = strlen( $secret ) + $this->clientId = ( $secret != '' ) ? hash_hmac( 'md5', $client['ip'] . "\n" . $client['agent'], $secret ) : md5( $client['ip'] . "\n" . $client['agent'] ); } @@ -127,44 +130,46 @@ class ChronologyProtector implements LoggerAwareInterface { } /** - * Initialise a ILoadBalancer to give it appropriate chronology protection. + * Apply the "session consistency" DB replication position to a new ILoadBalancer * - * If the stash has a previous master position recorded, this will try to - * make sure that the next query to a replica DB of that master will see changes up + * If the stash has a previous master position recorded, this will try to make + * sure that the next query to a replica DB of that master will see changes up * to that position by delaying execution. The delay may timeout and allow stale * data if no non-lagged replica DBs are available. * + * This method should only be called from LBFactory. + * * @param ILoadBalancer $lb * @return void */ - public function initLB( ILoadBalancer $lb ) { - if ( !$this->enabled || $lb->getServerCount() <= 1 ) { - return; // non-replicated setup or disabled + public function applySessionReplicationPosition( ILoadBalancer $lb ) { + if ( !$this->enabled ) { + return; // disabled } - $this->initPositions(); - $masterName = $lb->getServerName( $lb->getWriterIndex() ); - if ( - isset( $this->startupPositions[$masterName] ) && - $this->startupPositions[$masterName] instanceof DBMasterPos - ) { - $pos = $this->startupPositions[$masterName]; - $this->logger->debug( __METHOD__ . ": LB for '$masterName' set to pos $pos\n" ); + $startupPositions = $this->getStartupMasterPositions(); + + $pos = $startupPositions[$masterName] ?? null; + if ( $pos instanceof DBMasterPos ) { + $this->logger->debug( __METHOD__ . ": pos for DB '$masterName' set to '$pos'\n" ); $lb->waitFor( $pos ); } } /** - * Notify the ChronologyProtector that the ILoadBalancer is about to shut - * down. Saves replication positions. + * Save the "session consistency" DB replication position for an end-of-life ILoadBalancer + * + * This saves the replication position of the master DB if this request made writes to it. + * + * This method should only be called from LBFactory. * * @param ILoadBalancer $lb * @return void */ - public function shutdownLB( ILoadBalancer $lb ) { + public function storeSessionReplicationPosition( ILoadBalancer $lb ) { if ( !$this->enabled ) { - return; // not enabled + return; // disabled } elseif ( !$lb->hasOrMadeRecentMasterChanges( INF ) ) { // Only save the position if writes have been done on the connection return; @@ -209,10 +214,13 @@ class ChronologyProtector implements LoggerAwareInterface { } if ( $this->shutdownPositions === [] ) { + $this->logger->debug( __METHOD__ . ": no master positions to save\n" ); + return []; // nothing to save } - $this->logger->debug( __METHOD__ . ": saving master pos for " . + $this->logger->debug( + __METHOD__ . ": saving master pos for " . implode( ', ', array_keys( $this->shutdownPositions ) ) . "\n" ); @@ -282,12 +290,14 @@ class ChronologyProtector implements LoggerAwareInterface { /** * Load in previous master positions for the client */ - protected function initPositions() { + protected function getStartupMasterPositions() { if ( $this->initialized ) { - return; + return $this->startupPositions; } $this->initialized = true; + $this->logger->debug( __METHOD__ . ": client ID is {$this->clientId} (read)\n" ); + if ( $this->wait ) { // 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. @@ -344,6 +354,8 @@ class ChronologyProtector implements LoggerAwareInterface { $this->startupPositions = []; $this->logger->debug( __METHOD__ . ": key is {$this->key} (unread)\n" ); } + + return $this->startupPositions; } /** diff --git a/includes/libs/rdbms/lbfactory/LBFactory.php b/includes/libs/rdbms/lbfactory/LBFactory.php index bf10053385..8608a7d6f9 100644 --- a/includes/libs/rdbms/lbfactory/LBFactory.php +++ b/includes/libs/rdbms/lbfactory/LBFactory.php @@ -511,7 +511,7 @@ abstract class LBFactory implements ILBFactory { [ 'ip' => $this->requestInfo['IPAddress'], 'agent' => $this->requestInfo['UserAgent'], - 'clientId' => $this->requestInfo['ChronologyClientId'] + 'clientId' => $this->requestInfo['ChronologyClientId'] ?: null ], $this->requestInfo['ChronologyPositionIndex'], $this->secret @@ -549,7 +549,7 @@ abstract class LBFactory implements ILBFactory { ) { // Record all the master positions needed $this->forEachLB( function ( ILoadBalancer $lb ) use ( $cp ) { - $cp->shutdownLB( $lb ); + $cp->storeSessionReplicationPosition( $lb ); } ); // 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. @@ -603,7 +603,7 @@ abstract class LBFactory implements ILBFactory { 'chronologyCallback' => function ( ILoadBalancer $lb ) { // Defer ChronologyProtector construction in case setRequestInfo() ends up // being called later (but before the first connection attempt) (T192611) - $this->getChronologyProtector()->initLB( $lb ); + $this->getChronologyProtector()->applySessionReplicationPosition( $lb ); }, 'roundStage' => $initStage ]; diff --git a/tests/phpunit/includes/db/LBFactoryTest.php b/tests/phpunit/includes/db/LBFactoryTest.php index 106a13bc77..f7007e72e3 100644 --- a/tests/phpunit/includes/db/LBFactoryTest.php +++ b/tests/phpunit/includes/db/LBFactoryTest.php @@ -356,11 +356,11 @@ class LBFactoryTest extends MediaWikiTestCase { $mockDB2->expects( $this->exactly( 1 ) )->method( 'lastDoneWrites' ); // Nothing to wait for on first HTTP request start - $cp->initLB( $lb1 ); - $cp->initLB( $lb2 ); + $cp->applySessionReplicationPosition( $lb1 ); + $cp->applySessionReplicationPosition( $lb2 ); // Record positions in stash on first HTTP request end - $cp->shutdownLB( $lb1 ); - $cp->shutdownLB( $lb2 ); + $cp->storeSessionReplicationPosition( $lb1 ); + $cp->storeSessionReplicationPosition( $lb2 ); $cpIndex = null; $cp->shutdown( null, 'sync', $cpIndex ); @@ -395,11 +395,11 @@ class LBFactoryTest extends MediaWikiTestCase { ); // Wait for last positions to be reached on second HTTP request start - $cp->initLB( $lb1 ); - $cp->initLB( $lb2 ); + $cp->applySessionReplicationPosition( $lb1 ); + $cp->applySessionReplicationPosition( $lb2 ); // Shutdown (nothing to record) - $cp->shutdownLB( $lb1 ); - $cp->shutdownLB( $lb2 ); + $cp->storeSessionReplicationPosition( $lb1 ); + $cp->storeSessionReplicationPosition( $lb2 ); $cpIndex = null; $cp->shutdown( null, 'sync', $cpIndex ); diff --git a/tests/phpunit/includes/libs/rdbms/ChronologyProtectorTest.php b/tests/phpunit/includes/libs/rdbms/ChronologyProtectorTest.php new file mode 100644 index 0000000000..5901bc108c --- /dev/null +++ b/tests/phpunit/includes/libs/rdbms/ChronologyProtectorTest.php @@ -0,0 +1,81 @@ +assertEquals( $expectedId, $cp->getClientId() ); + } + + public function clientIdProvider() { + return [ + [ + [ + 'ip' => '127.0.0.1', + 'agent' => "Totally-Not-FireFox" + ], + '', + '45e93a9c215c031d38b7c42d8e4700ca', + ], + [ + [ + 'ip' => '127.0.0.7', + 'agent' => "Totally-Not-FireFox" + ], + '', + 'b1d604117b51746c35c3df9f293c84dc' + ], + [ + [ + 'ip' => '127.0.0.1', + 'agent' => "Totally-FireFox" + ], + '', + '731b4e06a65e2346b497fc811571c4d7' + ], + [ + [ + 'ip' => '127.0.0.1', + 'agent' => "Totally-Not-FireFox" + ], + 'secret', + 'defff51ded73cd901253d874c9b2077d' + ] + ]; + } +} -- 2.20.1