From 52af356cad3799ebec3826e1e4743d76a114da3e Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Thu, 10 May 2018 16:18:19 -0700 Subject: [PATCH] Avoid unnecessary WaitConditionLoop delays in ChronologyProtector Since it takes time for the agent to get the response and set the cookie and, as well, the time into a request that a LoadBalancer is initialized varies by many seconds (cookies loaded from the start), give the cookie a much lower TTL than the DB positions in the stash. This avoids having to wait for a position with a given cpPosIndex value, when the position already expired from the store, which is a waste of time. Also include the timestamp in "cpPosIndex" cookies to implement logical expiration in case clients do not expire them correctly. Bug: T194403 Bug: T190082 Change-Id: I97d8f108dec59c5ccead66432a097cda8ef4a178 --- includes/MediaWiki.php | 6 ++-- includes/Setup.php | 14 ++++++-- includes/libs/rdbms/ChronologyProtector.php | 2 ++ includes/libs/rdbms/lbfactory/ILBFactory.php | 4 +-- includes/libs/rdbms/lbfactory/LBFactory.php | 30 +++++++++++++++++ tests/phpunit/includes/db/LBFactoryTest.php | 34 ++++++++++++++++++++ 6 files changed, 83 insertions(+), 7 deletions(-) diff --git a/includes/MediaWiki.php b/includes/MediaWiki.php index e6dc0fecd3..459a7e1dcc 100644 --- a/includes/MediaWiki.php +++ b/includes/MediaWiki.php @@ -636,9 +636,11 @@ class MediaWiki { if ( $cpIndex > 0 ) { if ( $allowHeaders ) { - $expires = time() + ChronologyProtector::POSITION_TTL; + $now = time(); + $expires = $now + ChronologyProtector::POSITION_COOKIE_TTL; $options = [ 'prefix' => '' ]; - $request->response()->setCookie( 'cpPosIndex', $cpIndex, $expires, $options ); + $value = LBFactory::makeCookieValueFromCPIndex( $cpIndex, $now ); // T190082 + $request->response()->setCookie( 'cpPosIndex', $value, $expires, $options ); } if ( $strategy === 'cookie+url' ) { diff --git a/includes/Setup.php b/includes/Setup.php index 5cc9a96aa3..2c315a3155 100644 --- a/includes/Setup.php +++ b/includes/Setup.php @@ -24,6 +24,8 @@ * @file */ use MediaWiki\MediaWikiServices; +use Wikimedia\Rdbms\LBFactory; +use Wikimedia\Rdbms\ChronologyProtector; /** * This file is not a valid entry point, perform no further processing unless @@ -738,9 +740,15 @@ MediaWikiServices::getInstance()->getDBLoadBalancerFactory()->setRequestInfo( [ 'IPAddress' => $wgRequest->getIP(), 'UserAgent' => $wgRequest->getHeader( 'User-Agent' ), 'ChronologyProtection' => $wgRequest->getHeader( 'ChronologyProtection' ), - // The cpPosIndex cookie has no prefix and is set by MediaWiki::preOutputCommit() - 'ChronologyPositionIndex' => - $wgRequest->getInt( 'cpPosIndex', (int)$wgRequest->getCookie( 'cpPosIndex', '' ) ) + 'ChronologyPositionIndex' => $wgRequest->getInt( + 'cpPosIndex', + LBFactory::getCPIndexFromCookieValue( + // The cookie has no prefix and is set by MediaWiki::preOutputCommit() + $wgRequest->getCookie( 'cpPosIndex', '' ), + // Mitigate broken client-side cookie expiration handling (T190082) + time() - ChronologyProtector::POSITION_COOKIE_TTL + ) + ) ] ); // Make sure that object caching does not undermine the ChronologyProtector improvements if ( $wgRequest->getCookie( 'UseDC', '' ) === 'master' ) { diff --git a/includes/libs/rdbms/ChronologyProtector.php b/includes/libs/rdbms/ChronologyProtector.php index 90e697ec16..1c58dd0a6b 100644 --- a/includes/libs/rdbms/ChronologyProtector.php +++ b/includes/libs/rdbms/ChronologyProtector.php @@ -63,6 +63,8 @@ class ChronologyProtector implements LoggerAwareInterface { /** @var int Seconds to store positions */ const POSITION_TTL = 60; + /** @var int Seconds to store position write index cookies (safely less than POSITION_TTL) */ + const POSITION_COOKIE_TTL = 60; /** @var int Max time to wait for positions to appear */ const POS_STORE_WAIT_TIMEOUT = 5; diff --git a/includes/libs/rdbms/lbfactory/ILBFactory.php b/includes/libs/rdbms/lbfactory/ILBFactory.php index 45e7cbb756..16d0e31e27 100644 --- a/includes/libs/rdbms/lbfactory/ILBFactory.php +++ b/includes/libs/rdbms/lbfactory/ILBFactory.php @@ -321,10 +321,10 @@ interface ILBFactory { * Note that unlike cookies, this works accross domains * * @param string $url - * @param float $time UNIX timestamp just before shutdown() was called + * @param int $index Write counter index * @return string */ - public function appendShutdownCPIndexAsQuery( $url, $time ); + public function appendShutdownCPIndexAsQuery( $url, $index ); /** * @param array $info Map of fields, including: diff --git a/includes/libs/rdbms/lbfactory/LBFactory.php b/includes/libs/rdbms/lbfactory/LBFactory.php index 097775afc9..5501e6a9ca 100644 --- a/includes/libs/rdbms/lbfactory/LBFactory.php +++ b/includes/libs/rdbms/lbfactory/LBFactory.php @@ -640,6 +640,36 @@ abstract class LBFactory implements ILBFactory { return strpos( $url, '?' ) === false ? "$url?cpPosIndex=$index" : "$url&cpPosIndex=$index"; } + /** + * @param int $index Write index + * @param int $time UNIX timestamp + * @return string Timestamp-qualified write index of the form "." + * @since 1.32 + */ + public static function makeCookieValueFromCPIndex( $index, $time ) { + return $index . '@' . $time; + } + + /** + * @param string $value String possibly of the form "" or "@" + * @param int $minTimestamp Lowest UNIX timestamp of non-expired values (if present) + * @return int|null Write index or null if $value is empty or expired + * @since 1.32 + */ + public static function getCPIndexFromCookieValue( $value, $minTimestamp ) { + if ( !preg_match( '/^(\d+)(?:@(\d+))?$/', $value, $m ) ) { + return null; + } + + $index = (int)$m[1]; + + if ( isset( $m[2] ) && $m[2] !== '' && (int)$m[2] < $minTimestamp ) { + return null; // expired + } + + return ( $index > 0 ) ? $index : null; + } + public function setRequestInfo( array $info ) { if ( $this->chronProt ) { throw new LogicException( 'ChronologyProtector already initialized.' ); diff --git a/tests/phpunit/includes/db/LBFactoryTest.php b/tests/phpunit/includes/db/LBFactoryTest.php index 6e23e53639..0395bff2b4 100644 --- a/tests/phpunit/includes/db/LBFactoryTest.php +++ b/tests/phpunit/includes/db/LBFactoryTest.php @@ -606,4 +606,38 @@ class LBFactoryTest extends MediaWikiTestCase { return $db->addIdentifierQuotes( $table ); } } + + /** + * @covers \Wikimedia\Rdbms\LBFactory::makeCookieValueFromCPIndex() + * @covers \Wikimedia\Rdbms\LBFactory::getCPIndexFromCookieValue() + */ + public function testCPPosIndexCookieValues() { + $this->assertEquals( '3@542', LBFactory::makeCookieValueFromCPIndex( 3, 542 ) ); + + $time = 1526522031; + $this->assertSame( + 5, + LBFactory::getCPIndexFromCookieValue( "5", $time - 10 ) + ); + $this->assertSame( + null, + LBFactory::getCPIndexFromCookieValue( "0", $time - 10 ) + ); + $this->assertSame( + 2, + LBFactory::getCPIndexFromCookieValue( "2@$time", $time - 10 ) + ); + $this->assertSame( + 2, + LBFactory::getCPIndexFromCookieValue( "2@$time", $time + 9 - 10 ) + ); + $this->assertSame( + null, + LBFactory::getCPIndexFromCookieValue( "0@$time", $time + 9 - 10 ) + ); + $this->assertSame( + null, + LBFactory::getCPIndexFromCookieValue( "2@$time", $time + 11 - 10 ) + ); + } } -- 2.20.1