Code cleanups to SqlBagOStuff

* Refactor local DB usage check into usesMainDB() method.
* Avoid using the db member of DBError instances.

Change-Id: I7350f5a471c551492094bfaf545ebc222eb6f7dd

diff --git a/includes/objectcache/SqlBagOStuff.php b/includes/objectcache/SqlBagOStuff.php
index 84936a4..9ab28aa 100644 (file)
--- a/includes/objectcache/SqlBagOStuff.php
+++ b/includes/objectcache/SqlBagOStuff.php
@@ -21,6 +21,8 @@
  * @ingroup Cache
  */
 
+use \MediaWiki\MediaWikiServices;
+
 /**
  * Class to store objects in the database
  *
@@ -276,6 +278,7 @@ class SqlBagOStuff extends BagOStuff {
                        if ( isset( $dataRows[$key] ) ) { // HIT?
                                $row = $dataRows[$key];
                                $this->debug( "get: retrieved data; expiry time is " . $row->exptime );
+                               $db = null;
                                try {
                                        $db = $this->getDB( $row->serverIndex );
                                        if ( $this->isExpired( $db, $row->exptime ) ) { // MISS
@@ -284,7 +287,7 @@ class SqlBagOStuff extends BagOStuff {
                                                $values[$key] = $this->unserialize( $db->decodeBlob( $row->value ) );
                                        }
                                } catch ( DBQueryError $e ) {
-                                       $this->handleWriteError( $e, $row->serverIndex );
+                                       $this->handleWriteError( $e, $db, $row->serverIndex );
                                }
                        } else { // MISS
                                $this->debug( 'get: no matching rows' );
@@ -306,10 +309,11 @@ class SqlBagOStuff extends BagOStuff {
                $result = true;
                $exptime = (int)$expiry;
                foreach ( $keysByTable as $serverIndex => $serverKeys ) {
+                       $db = null;
                        try {
                                $db = $this->getDB( $serverIndex );
                        } catch ( DBError $e ) {
-                               $this->handleWriteError( $e, $serverIndex );
+                               $this->handleWriteError( $e, $db, $serverIndex );
                                $result = false;
                                continue;
                        }
@@ -342,7 +346,7 @@ class SqlBagOStuff extends BagOStuff {
                                                __METHOD__
                                        );
                                } catch ( DBError $e ) {
-                                       $this->handleWriteError( $e, $serverIndex );
+                                       $this->handleWriteError( $e, $db, $serverIndex );
                                        $result = false;
                                }
 
@@ -364,6 +368,7 @@ class SqlBagOStuff extends BagOStuff {
 
        protected function cas( $casToken, $key, $value, $exptime = 0 ) {
                list( $serverIndex, $tableName ) = $this->getTableByKey( $key );
+               $db = null;
                try {
                        $db = $this->getDB( $serverIndex );
                        $exptime = intval( $exptime );
@@ -394,7 +399,7 @@ class SqlBagOStuff extends BagOStuff {
                                __METHOD__
                        );
                } catch ( DBQueryError $e ) {
-                       $this->handleWriteError( $e, $serverIndex );
+                       $this->handleWriteError( $e, $db, $serverIndex );
 
                        return false;
                }
@@ -404,6 +409,7 @@ class SqlBagOStuff extends BagOStuff {
 
        public function delete( $key ) {
                list( $serverIndex, $tableName ) = $this->getTableByKey( $key );
+               $db = null;
                try {
                        $db = $this->getDB( $serverIndex );
                        $db->delete(
@@ -411,7 +417,7 @@ class SqlBagOStuff extends BagOStuff {
                                [ 'keyname' => $key ],
                                __METHOD__ );
                } catch ( DBError $e ) {
-                       $this->handleWriteError( $e, $serverIndex );
+                       $this->handleWriteError( $e, $db, $serverIndex );
                        return false;
                }
 
@@ -420,6 +426,7 @@ class SqlBagOStuff extends BagOStuff {
 
        public function incr( $key, $step = 1 ) {
                list( $serverIndex, $tableName ) = $this->getTableByKey( $key );
+               $db = null;
                try {
                        $db = $this->getDB( $serverIndex );
                        $step = intval( $step );
@@ -455,7 +462,7 @@ class SqlBagOStuff extends BagOStuff {
                                $newValue = null;
                        }
                } catch ( DBError $e ) {
-                       $this->handleWriteError( $e, $serverIndex );
+                       $this->handleWriteError( $e, $db, $serverIndex );
                        return null;
                }
 
@@ -473,6 +480,7 @@ class SqlBagOStuff extends BagOStuff {
 
        public function changeTTL( $key, $expiry = 0 ) {
                list( $serverIndex, $tableName ) = $this->getTableByKey( $key );
+               $db = null;
                try {
                        $db = $this->getDB( $serverIndex );
                        $db->update(
@@ -485,7 +493,7 @@ class SqlBagOStuff extends BagOStuff {
                                return false;
                        }
                } catch ( DBError $e ) {
-                       $this->handleWriteError( $e, $serverIndex );
+                       $this->handleWriteError( $e, $db, $serverIndex );
                        return false;
                }
 
@@ -542,6 +550,7 @@ class SqlBagOStuff extends BagOStuff {
         */
        public function deleteObjectsExpiringBefore( $timestamp, $progressCallback = false ) {
                for ( $serverIndex = 0; $serverIndex < $this->numServers; $serverIndex++ ) {
+                       $db = null;
                        try {
                                $db = $this->getDB( $serverIndex );
                                $dbTimestamp = $db->timestamp( $timestamp );
@@ -604,7 +613,7 @@ class SqlBagOStuff extends BagOStuff {
                                        }
                                }
                        } catch ( DBError $e ) {
-                               $this->handleWriteError( $e, $serverIndex );
+                               $this->handleWriteError( $e, $db, $serverIndex );
                                return false;
                        }
                }
@@ -618,13 +627,14 @@ class SqlBagOStuff extends BagOStuff {
         */
        public function deleteAll() {
                for ( $serverIndex = 0; $serverIndex < $this->numServers; $serverIndex++ ) {
+                       $db = null;
                        try {
                                $db = $this->getDB( $serverIndex );
                                for ( $i = 0; $i < $this->shards; $i++ ) {
                                        $db->delete( $this->getTableNameByShard( $i ), '*', __METHOD__ );
                                }
                        } catch ( DBError $e ) {
-                               $this->handleWriteError( $e, $serverIndex );
+                               $this->handleWriteError( $e, $db, $serverIndex );
                                return false;
                        }
                }
@@ -694,26 +704,19 @@ class SqlBagOStuff extends BagOStuff {
         * Handle a DBQueryError which occurred during a write operation.
         *
         * @param DBError $exception
+        * @param IDatabase|null $db DB handle or null if connection failed
         * @param int $serverIndex
         * @throws Exception
         */
-       protected function handleWriteError( DBError $exception, $serverIndex ) {
-               if ( $exception instanceof DBConnectionError ) {
+       protected function handleWriteError( DBError $exception, IDatabase $db = null, $serverIndex ) {
+               if ( !$db ) {
                        $this->markServerDown( $exception, $serverIndex );
-               }
-               if ( $exception->db && $exception->db->wasReadOnlyError() ) {
-                       if ( $exception->db->trxLevel() ) {
-                               if ( !$this->serverInfos ) {
-                                       // Errors like deadlocks and connection drops already cause rollback.
-                                       // For consistency, we have no choice but to throw an error and trigger
-                                       // complete rollback if the main DB is also being used as the cache DB.
-                                       throw $exception;
-                               }
-
-                               try {
-                                       $exception->db->rollback( __METHOD__ );
-                               } catch ( DBError $e ) {
-                               }
+               } elseif ( $db->wasReadOnlyError() ) {
+                       if ( $db->trxLevel() && $this->usesMainDB() ) {
+                               // Errors like deadlocks and connection drops already cause rollback.
+                               // For consistency, we have no choice but to throw an error and trigger
+                               // complete rollback if the main DB is also being used as the cache DB.
+                               throw $exception;
                        }
                }
 
@@ -733,7 +736,7 @@ class SqlBagOStuff extends BagOStuff {
         * @param DBError $exception
         * @param int $serverIndex
         */
-       protected function markServerDown( $exception, $serverIndex ) {
+       protected function markServerDown( DBError $exception, $serverIndex ) {
                unset( $this->conns[$serverIndex] ); // bug T103435
 
                if ( isset( $this->connFailureTimes[$serverIndex] ) ) {
@@ -770,11 +773,19 @@ class SqlBagOStuff extends BagOStuff {
                }
        }
 
+       /**
+        * @return bool Whether the main DB is used, e.g. wfGetDB( DB_MASTER )
+        */
+       protected function usesMainDB() {
+               return !$this->serverInfos;
+       }
+
        protected function waitForSlaves() {
-               if ( !$this->serverInfos ) {
+               if ( $this->usesMainDB() ) {
                        // Main LB is used; wait for any slaves to catch up
                        try {
-                               wfGetLBFactory()->waitForReplication( [ 'wiki' => wfWikiID() ] );
+                               $lbFactory = MediaWikiServices::getInstance()->getDBLoadBalancerFactory();
+                               $lbFactory->waitForReplication( [ 'wiki' => wfWikiID() ] );
                                return true;
                        } catch ( DBReplicationWaitError $e ) {
                                return false;