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

diff --git a/includes/MediaWiki.php b/includes/MediaWiki.php
index e6dc0fe..459a7e1 100644 (file)
--- 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 5cc9a96..2c315a3 100644 (file)
--- 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 90e697e..1c58dd0 100644 (file)
--- 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 45e7cbb..16d0e31 100644 (file)
--- 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 097775a..5501e6a 100644 (file)
--- 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 "<index>.<timestamp>"
+        * @since 1.32
+        */
+       public static function makeCookieValueFromCPIndex( $index, $time ) {
+               return $index . '@' . $time;
+       }
+
+       /**
+        * @param string $value String possibly of the form "<index>" or "<index>@<timestamp>"
+        * @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 6e23e53..0395bff 100644 (file)
--- 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 )
+               );
+       }
 }