Merge "PreprocessorTest: test both implementations"

diff --git a/includes/db/ChronologyProtector.php b/includes/db/ChronologyProtector.php
index cc35999..2539b87 100644 (file)
--- a/includes/db/ChronologyProtector.php
+++ b/includes/db/ChronologyProtector.php
@@ -147,31 +147,20 @@ class ChronologyProtector {
                        implode( ', ', array_keys( $this->shutdownPositions ) ) . "\n"
                );
 
-               $shutdownPositions = $this->shutdownPositions;
-               $ok = $this->store->merge(
-                       $this->key,
-                       function ( $store, $key, $curValue ) use ( $shutdownPositions ) {
-                               /** @var $curPositions DBMasterPos[] */
-                               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] )
-                                                       || $pos->asOfTime() > $curPositions[$db]->asOfTime()
-                                               ) {
-                                                       $curPositions[$db] = $pos;
-                                               }
-                                       }
-                               }
-
-                               return [ 'positions' => $curPositions ];
-                       },
-                       BagOStuff::TTL_MINUTE,
-                       10,
-                       BagOStuff::WRITE_SYNC // visible in all datacenters
-               );
+               // CP-protected writes should overwhemingly go to the master datacenter, so get DC-local
+               // lock to merge the values. Use a DC-local get() and a synchronous all-DC set(). This
+               // makes it possible for the BagOStuff class to write in parallel to all DCs with one RTT.
+               if ( $this->store->lock( $this->key, 3 ) ) {
+                       $ok = $this->store->set(
+                               $this->key,
+                               self::mergePositions( $this->store->get( $this->key ), $this->shutdownPositions ),
+                               BagOStuff::TTL_MINUTE,
+                               BagOStuff::WRITE_SYNC
+                       );
+                       $this->store->unlock( $this->key );
+               } else {
+                       $ok = false;
+               }
 
                if ( !$ok ) {
                        // Raced out too many times or stash is down
@@ -206,4 +195,28 @@ class ChronologyProtector {
                        wfDebugLog( 'replication', __METHOD__ . ": key is {$this->key} (unread)\n" );
                }
        }
+
+       /**
+        * @param array|bool $curValue
+        * @param DBMasterPos[] $shutdownPositions
+        * @return array
+        */
+       private static function mergePositions( $curValue, array $shutdownPositions ) {
+               /** @var $curPositions DBMasterPos[] */
+               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] )
+                                       || $pos->asOfTime() > $curPositions[$db]->asOfTime()
+                               ) {
+                                       $curPositions[$db] = $pos;
+                               }
+                       }
+               }
+
+               return [ 'positions' => $curPositions ];
+       }
 }

diff --git a/includes/objectcache/SqlBagOStuff.php b/includes/objectcache/SqlBagOStuff.php
index 9ab28aa..7a89991 100644 (file)
--- a/includes/objectcache/SqlBagOStuff.php
+++ b/includes/objectcache/SqlBagOStuff.php
@@ -48,6 +48,8 @@ class SqlBagOStuff extends BagOStuff {
        /** @var int */
        protected $syncTimeout = 3;
 
+       /** @var LoadBalancer|null */
+       protected $separateMainLB;
        /** @var array */
        protected $conns;
        /** @var array UNIX timestamps */
@@ -114,6 +116,7 @@ class SqlBagOStuff extends BagOStuff {
                        $this->serverInfos = [ $params['server'] ];
                        $this->numServers = count( $this->serverInfos );
                } else {
+                       // Default to using the main wiki's database servers
                        $this->serverInfos = false;
                        $this->numServers = 1;
                }
@@ -132,6 +135,23 @@ class SqlBagOStuff extends BagOStuff {
                $this->slaveOnly = !empty( $params['slaveOnly'] );
        }
 
+       protected function getSeparateMainLB() {
+               global $wgDBtype;
+
+               if ( $wgDBtype === 'mysql' && $this->usesMainDB() ) {
+                       if ( !$this->separateMainLB ) {
+                               // We must keep a separate connection to MySQL in order to avoid deadlocks
+                               $lbFactory = MediaWikiServices::getInstance()->getDBLoadBalancerFactory();
+                               $this->separateMainLB = $lbFactory->newMainLB();
+                       }
+                       return $this->separateMainLB;
+               } else {
+                       // However, SQLite has an opposite behavior. And PostgreSQL needs to know
+                       // if we are in transaction or not (@TODO: find some PostgreSQL work-around).
+                       return null;
+               }
+       }
+
        /**
         * Get a connection to the specified database
         *
@@ -163,16 +183,13 @@ class SqlBagOStuff extends BagOStuff {
                                $db = DatabaseBase::factory( $type, $info );
                                $db->clearFlag( DBO_TRX );
                        } else {
-                               // We must keep a separate connection to MySQL in order to avoid deadlocks
-                               // However, SQLite has an opposite behavior. And PostgreSQL needs to know
-                               // if we are in transaction or not (@TODO: find some work-around).
                                $index = $this->slaveOnly ? DB_SLAVE : DB_MASTER;
-                               if ( wfGetDB( $index )->getType() == 'mysql' ) {
-                                       $lb = wfGetLBFactory()->newMainLB();
-                                       $db = $lb->getConnection( $index );
+                               if ( $this->getSeparateMainLB() ) {
+                                       $db = $this->getSeparateMainLB()->getConnection( $index );
                                        $db->clearFlag( DBO_TRX ); // auto-commit mode
                                } else {
                                        $db = wfGetDB( $index );
+                                       // Can't mess with transaction rounds (DBO_TRX) :(
                                }
                        }
                        $this->logger->debug( sprintf( "Connection %s will be used for SqlBagOStuff", $db ) );
@@ -782,17 +799,20 @@ class SqlBagOStuff extends BagOStuff {
 
        protected function waitForSlaves() {
                if ( $this->usesMainDB() ) {
+                       $lb = $this->getSeparateMainLB()
+                               ?: MediaWikiServices::getInstance()->getDBLoadBalancer();
                        // Main LB is used; wait for any slaves to catch up
                        try {
-                               $lbFactory = MediaWikiServices::getInstance()->getDBLoadBalancerFactory();
-                               $lbFactory->waitForReplication( [ 'wiki' => wfWikiID() ] );
-                               return true;
+                               $pos = $lb->getMasterPos();
+                               if ( $pos ) {
+                                       return $lb->waitForAll( $pos, 3 );
+                               }
                        } catch ( DBReplicationWaitError $e ) {
                                return false;
                        }
-               } else {
-                       // Custom DB server list; probably doesn't use replication
-                       return true;
                }
+
+               // Custom DB server list; probably doesn't use replication
+               return true;
        }
 }

diff --git a/includes/specialpage/AuthManagerSpecialPage.php b/includes/specialpage/AuthManagerSpecialPage.php
index 68f0d00..3adf5a6 100644 (file)
--- a/includes/specialpage/AuthManagerSpecialPage.php
+++ b/includes/specialpage/AuthManagerSpecialPage.php
@@ -554,38 +554,46 @@ abstract class AuthManagerSpecialPage extends SpecialPage {
        }
 
        /**
-        * Returns true if the form built from the given AuthenticationRequests has fields which take
-        * values. If all available providers use the redirect flow, the form might contain nothing
-        * but submit buttons, in which case we should not add an extra submit button which does nothing.
+        * Returns true if the form built from the given AuthenticationRequests needs a submit button.
+        * Providers using redirect flow (e.g. Google login) need their own submit buttons; if using
+        * one of those custom buttons is the only way to proceed, there is no point in displaying the
+        * default button which won't do anything useful.
         *
         * @param AuthenticationRequest[] $requests An array of AuthenticationRequests from which the
         *  form will be built
         * @return bool
         */
        protected function needsSubmitButton( array $requests ) {
+               $customSubmitButtonPresent = false;
+
+               // Secondary and preauth providers always need their data; they will not care what button
+               // is used, so they can be ignored. So can OPTIONAL buttons createdby primary providers;
+               // that's the point in being optional. Se we need to check whether all primary providers
+               // have their own buttons and whether there is at least one button present.
                foreach ( $requests as $req ) {
-                       if ( $req->required === AuthenticationRequest::PRIMARY_REQUIRED &&
-                               $this->doesRequestNeedsSubmitButton( $req )
-                       ) {
-                               return true;
+                       if ( $req->required === AuthenticationRequest::PRIMARY_REQUIRED ) {
+                               if ( $this->hasOwnSubmitButton( $req ) ) {
+                                       $customSubmitButtonPresent = true;
+                               } else {
+                                       return true;
+                               }
                        }
                }
-               return false;
+               return !$customSubmitButtonPresent;
        }
 
        /**
-        * Checks if the given AuthenticationRequest needs a submit button or not.
-        *
-        * @param AuthenticationRequest $req The request to check
+        * Checks whether the given AuthenticationRequest has its own submit button.
+        * @param AuthenticationRequest $req
         * @return bool
         */
-       protected function doesRequestNeedsSubmitButton( AuthenticationRequest $req ) {
+       protected function hasOwnSubmitButton( AuthenticationRequest $req ) {
                foreach ( $req->getFieldInfo() as $field => $info ) {
                        if ( $info['type'] === 'button' ) {
-                               return false;
+                               return true;
                        }
                }
-               return true;
+               return false;
        }
 
        /**