rdbms: expand on LoadBalancer ownership concept
authorAaron Schulz <aschulz@wikimedia.org>
Thu, 29 Aug 2019 04:05:10 +0000 (21:05 -0700)
committerAaron Schulz <aschulz@wikimedia.org>
Thu, 5 Sep 2019 19:08:13 +0000 (12:08 -0700)
Enforce this pattern for the remaining LoadBalancer methods.
Carry this over into Database::close() to decide how loud the
error handling should be.

In LBFactory, clean up ownership of newMainLB()/newExternalLB().
The should have a null owner if called from outside the class
since the LBFactory does not track nor care about them anymore
in that case. Disable newMainLB() for LBFactorySingle as it
makes no sense and was broken.

Also remove some redundant abstract LBFactory methods that
just duplciate ILBFactory.

Bug: T231443
Bug: T217819
Depends-On: I7ed5c799320430c196a9a8e81af901999e2de7d0
Change-Id: I90b30a79754cfcc290277d302052e60df4fbc649

12 files changed:
includes/libs/rdbms/database/DBConnRef.php
includes/libs/rdbms/database/Database.php
includes/libs/rdbms/database/IDatabase.php
includes/libs/rdbms/lbfactory/ILBFactory.php
includes/libs/rdbms/lbfactory/LBFactory.php
includes/libs/rdbms/lbfactory/LBFactoryMulti.php
includes/libs/rdbms/lbfactory/LBFactorySimple.php
includes/libs/rdbms/lbfactory/LBFactorySingle.php
includes/libs/rdbms/loadbalancer/ILoadBalancer.php
includes/libs/rdbms/loadbalancer/LoadBalancer.php
tests/phpunit/includes/db/LBFactoryTest.php
tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php

index f0b135f..7870f69 100644 (file)
@@ -280,7 +280,7 @@ class DBConnRef implements IDatabase {
                return $this->__call( __FUNCTION__, func_get_args() );
        }
 
-       public function close() {
+       public function close( $fname = __METHOD__, $owner = null ) {
                throw new DBUnexpectedError( $this->conn, 'Cannot close shared connection.' );
        }
 
index be41ee0..51596da 100644 (file)
@@ -174,6 +174,9 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
        /** @var float Query rount trip time estimate */
        private $lastRoundTripEstimate = 0.0;
 
+       /** @var int|null Integer ID of the managing LBFactory instance or null if none */
+       private $ownerId;
+
        /** @var string Lock granularity is on the level of the entire database */
        const ATTR_DB_LEVEL_LOCKING = 'db-level-locking';
        /** @var string The SCHEMA keyword refers to a grouping of tables in a database */
@@ -268,6 +271,8 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
                        $params['schema'] != '' ? $params['schema'] : null,
                        $params['tablePrefix']
                );
+
+               $this->ownerId = $params['ownerId'] ?? null;
        }
 
        /**
@@ -355,7 +360,8 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
         *   - cliMode: Whether to consider the execution context that of a CLI script.
         *   - agent: Optional name used to identify the end-user in query profiling/logging.
         *   - srvCache: Optional BagOStuff instance to an APC-style cache.
-        *   - nonNativeInsertSelectBatchSize: Optional batch size for non-native INSERT SELECT emulation.
+        *   - nonNativeInsertSelectBatchSize: Optional batch size for non-native INSERT SELECT.
+        *   - ownerId: Optional integer ID of a LoadBalancer instance that manages this instance.
         * @param int $connect One of the class constants (NEW_CONNECTED, NEW_UNCONNECTED) [optional]
         * @return Database|null If the database driver or extension cannot be found
         * @throws InvalidArgumentException If the database driver or extension cannot be found
@@ -375,7 +381,8 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
                                'flags' => 0,
                                'variables' => [],
                                'cliMode' => ( PHP_SAPI === 'cli' || PHP_SAPI === 'phpdbg' ),
-                               'agent' => basename( $_SERVER['SCRIPT_NAME'] ) . '@' . gethostname()
+                               'agent' => basename( $_SERVER['SCRIPT_NAME'] ) . '@' . gethostname(),
+                               'ownerId' => null
                        ];
 
                        $normalizedParams = [
@@ -866,8 +873,8 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
                );
        }
 
-       final public function close() {
-               $exception = null; // error to throw after disconnecting
+       final public function close( $fname = __METHOD__, $owner = null ) {
+               $error = null; // error to throw after disconnecting
 
                $wasOpen = (bool)$this->conn;
                // This should mostly do nothing if the connection is already closed
@@ -877,34 +884,22 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
                                if ( $this->trxAtomicLevels ) {
                                        // Cannot let incomplete atomic sections be committed
                                        $levels = $this->flatAtomicSectionList();
-                                       $exception = new DBUnexpectedError(
-                                               $this,
-                                               __METHOD__ . ": atomic sections $levels are still open"
-                                       );
+                                       $error = "$fname: atomic sections $levels are still open";
                                } elseif ( $this->trxAutomatic ) {
                                        // Only the connection manager can commit non-empty DBO_TRX transactions
                                        // (empty ones we can silently roll back)
                                        if ( $this->writesOrCallbacksPending() ) {
-                                               $exception = new DBUnexpectedError(
-                                                       $this,
-                                                       __METHOD__ .
-                                                       ": mass commit/rollback of peer transaction required (DBO_TRX set)"
-                                               );
+                                               $error = "$fname: " .
+                                                       "expected mass rollback of all peer transactions (DBO_TRX set)";
                                        }
                                } else {
                                        // Manual transactions should have been committed or rolled
                                        // back, even if empty.
-                                       $exception = new DBUnexpectedError(
-                                               $this,
-                                               __METHOD__ . ": transaction is still open (from {$this->trxFname})"
-                                       );
+                                       $error = "$fname: transaction is still open (from {$this->trxFname})";
                                }
 
-                               if ( $this->trxEndCallbacksSuppressed ) {
-                                       $exception = $exception ?: new DBUnexpectedError(
-                                               $this,
-                                               __METHOD__ . ': callbacks are suppressed; cannot properly commit'
-                                       );
+                               if ( $this->trxEndCallbacksSuppressed && $error === null ) {
+                                       $error = "$fname: callbacks are suppressed; cannot properly commit";
                                }
 
                                // Rollback the changes and run any callbacks as needed
@@ -919,9 +914,16 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
 
                $this->conn = null;
 
-               // Throw any unexpected errors after having disconnected
-               if ( $exception instanceof Exception ) {
-                       throw $exception;
+               // Log or throw any unexpected errors after having disconnected
+               if ( $error !== null ) {
+                       // T217819, T231443: if this is probably just LoadBalancer trying to recover from
+                       // errors and shutdown, then log any problems and move on since the request has to
+                       // end one way or another. Throwing errors is not very useful at some point.
+                       if ( $this->ownerId !== null && $owner === $this->ownerId ) {
+                               $this->queryLogger->error( $error );
+                       } else {
+                               throw new DBUnexpectedError( $this, $error );
+                       }
                }
 
                // Note that various subclasses call close() at the start of open(), which itself is
@@ -3981,17 +3983,17 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
                if ( $this->trxLevel() ) {
                        if ( $this->trxAtomicLevels ) {
                                $levels = $this->flatAtomicSectionList();
-                               $msg = "$fname: Got explicit BEGIN while atomic section(s) $levels are open";
+                               $msg = "$fname: got explicit BEGIN while atomic section(s) $levels are open";
                                throw new DBUnexpectedError( $this, $msg );
                        } elseif ( !$this->trxAutomatic ) {
-                               $msg = "$fname: Explicit transaction already active (from {$this->trxFname})";
+                               $msg = "$fname: explicit transaction already active (from {$this->trxFname})";
                                throw new DBUnexpectedError( $this, $msg );
                        } else {
-                               $msg = "$fname: Implicit transaction already active (from {$this->trxFname})";
+                               $msg = "$fname: implicit transaction already active (from {$this->trxFname})";
                                throw new DBUnexpectedError( $this, $msg );
                        }
                } elseif ( $this->getFlag( self::DBO_TRX ) && $mode !== self::TRANSACTION_INTERNAL ) {
-                       $msg = "$fname: Implicit transaction expected (DBO_TRX set)";
+                       $msg = "$fname: implicit transaction expected (DBO_TRX set)";
                        throw new DBUnexpectedError( $this, $msg );
                }
 
@@ -4045,7 +4047,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
                        $levels = $this->flatAtomicSectionList();
                        throw new DBUnexpectedError(
                                $this,
-                               "$fname: Got COMMIT while atomic sections $levels are still open"
+                               "$fname: got COMMIT while atomic sections $levels are still open"
                        );
                }
 
@@ -4055,17 +4057,17 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
                        } elseif ( !$this->trxAutomatic ) {
                                throw new DBUnexpectedError(
                                        $this,
-                                       "$fname: Flushing an explicit transaction, getting out of sync"
+                                       "$fname: flushing an explicit transaction, getting out of sync"
                                );
                        }
                } elseif ( !$this->trxLevel() ) {
                        $this->queryLogger->error(
-                               "$fname: No transaction to commit, something got out of sync" );
+                               "$fname: no transaction to commit, something got out of sync" );
                        return; // nothing to do
                } elseif ( $this->trxAutomatic ) {
                        throw new DBUnexpectedError(
                                $this,
-                               "$fname: Expected mass commit of all peer transactions (DBO_TRX set)"
+                               "$fname: expected mass commit of all peer transactions (DBO_TRX set)"
                        );
                }
 
index 41f7cb6..befc80c 100644 (file)
@@ -460,10 +460,12 @@ interface IDatabase {
         * aside from read-only automatic transactions (assuming no callbacks are registered).
         * If a transaction is still open anyway, it will be rolled back.
         *
+        * @param string $fname Caller name
+        * @param int|null $owner ID of the calling instance (e.g. the LBFactory ID)
         * @return bool Success
         * @throws DBError
         */
-       public function close();
+       public function close( $fname = __METHOD__, $owner = null );
 
        /**
         * Run an SQL query and return the result
index 6e9591b..23232f4 100644 (file)
@@ -109,9 +109,10 @@ interface ILBFactory {
         * but still use DBO_TRX transaction rounds on other tables.
         *
         * @param bool|string $domain Domain ID, or false for the current domain
+        * @param int|null $owner Owner ID of the new instance (e.g. this LBFactory ID)
         * @return ILoadBalancer
         */
-       public function newMainLB( $domain = false );
+       public function newMainLB( $domain = false, $owner = null );
 
        /**
         * Get a cached (tracked) load balancer object.
@@ -131,9 +132,10 @@ interface ILBFactory {
         * (DBO_TRX off) but still use DBO_TRX transaction rounds on other tables.
         *
         * @param string $cluster External storage cluster name
+        * @param int|null $owner Owner ID of the new instance (e.g. this LBFactory ID)
         * @return ILoadBalancer
         */
-       public function newExternalLB( $cluster );
+       public function newExternalLB( $cluster, $owner = null );
 
        /**
         * Get a cached (tracked) load balancer for external storage
index 36e961a..07a5fe6 100644 (file)
@@ -155,15 +155,16 @@ abstract class LBFactory implements ILBFactory {
                $this->defaultGroup = $conf['defaultGroup'] ?? null;
                $this->secret = $conf['secret'] ?? '';
 
-               $this->id = mt_rand();
-               $this->ticket = mt_rand();
+               static $nextId, $nextTicket;
+               $this->id = $nextId = ( is_int( $nextId ) ? $nextId++ : mt_rand() );
+               $this->ticket = $nextTicket = ( is_int( $nextTicket ) ? $nextTicket++ : mt_rand() );
        }
 
        public function destroy() {
                /** @noinspection PhpUnusedLocalVariableInspection */
                $scope = ScopedCallback::newScopedIgnoreUserAbort();
 
-               $this->forEachLBCallMethod( 'disable' );
+               $this->forEachLBCallMethod( 'disable', [ __METHOD__, $this->id ] );
        }
 
        public function getLocalDomainID() {
@@ -195,34 +196,6 @@ abstract class LBFactory implements ILBFactory {
                $this->commitMasterChanges( __METHOD__ ); // sanity
        }
 
-       /**
-        * @see ILBFactory::newMainLB()
-        * @param bool $domain
-        * @return ILoadBalancer
-        */
-       abstract public function newMainLB( $domain = false );
-
-       /**
-        * @see ILBFactory::getMainLB()
-        * @param bool $domain
-        * @return ILoadBalancer
-        */
-       abstract public function getMainLB( $domain = false );
-
-       /**
-        * @see ILBFactory::newExternalLB()
-        * @param string $cluster
-        * @return ILoadBalancer
-        */
-       abstract public function newExternalLB( $cluster );
-
-       /**
-        * @see ILBFactory::getExternalLB()
-        * @param string $cluster
-        * @return ILoadBalancer
-        */
-       abstract public function getExternalLB( $cluster );
-
        /**
         * Call a method of each tracked load balancer
         *
@@ -245,13 +218,13 @@ abstract class LBFactory implements ILBFactory {
                                [ 'trace' => ( new RuntimeException() )->getTraceAsString() ]
                        );
                }
-               $this->forEachLBCallMethod( 'flushReplicaSnapshots', [ $fname ] );
+               $this->forEachLBCallMethod( 'flushReplicaSnapshots', [ $fname, $this->id ] );
        }
 
        final public function commitAll( $fname = __METHOD__, array $options = [] ) {
                $this->commitMasterChanges( $fname, $options );
-               $this->forEachLBCallMethod( 'flushMasterSnapshots', [ $fname ] );
-               $this->forEachLBCallMethod( 'flushReplicaSnapshots', [ $fname ] );
+               $this->forEachLBCallMethod( 'flushMasterSnapshots', [ $fname, $this->id ] );
+               $this->forEachLBCallMethod( 'flushReplicaSnapshots', [ $fname, $this->id ] );
        }
 
        final public function beginMasterChanges( $fname = __METHOD__ ) {
@@ -604,10 +577,12 @@ abstract class LBFactory implements ILBFactory {
        }
 
        /**
-        * Base parameters to ILoadBalancer::__construct()
+        * Get parameters to ILoadBalancer::__construct()
+        *
+        * @param int|null $owner Use getOwnershipId() if this is for getMainLB()/getExternalLB()
         * @return array
         */
-       final protected function baseLoadBalancerParams() {
+       final protected function baseLoadBalancerParams( $owner ) {
                if ( $this->trxRoundStage === self::ROUND_COMMIT_CALLBACKS ) {
                        $initStage = ILoadBalancer::STAGE_POSTCOMMIT_CALLBACKS;
                } elseif ( $this->trxRoundStage === self::ROUND_ROLLBACK_CALLBACKS ) {
@@ -639,7 +614,7 @@ abstract class LBFactory implements ILBFactory {
                                $this->getChronologyProtector()->applySessionReplicationPosition( $lb );
                        },
                        'roundStage' => $initStage,
-                       'ownerId' => $this->id
+                       'ownerId' => $owner
                ];
        }
 
@@ -689,7 +664,7 @@ abstract class LBFactory implements ILBFactory {
                /** @noinspection PhpUnusedLocalVariableInspection */
                $scope = ScopedCallback::newScopedIgnoreUserAbort();
 
-               $this->forEachLBCallMethod( 'closeAll' );
+               $this->forEachLBCallMethod( 'closeAll', [ __METHOD__, $this->id ] );
        }
 
        public function setAgentName( $agent ) {
@@ -757,6 +732,14 @@ abstract class LBFactory implements ILBFactory {
                $this->requestInfo = $info + $this->requestInfo;
        }
 
+       /**
+        * @return int Internal instance ID used to assert ownership of ILoadBalancer instances
+        * @since 1.34
+        */
+       final protected function getOwnershipId() {
+               return $this->id;
+       }
+
        /**
         * @param string $stage
         */
index ef1f0a6..77b029f 100644 (file)
@@ -157,7 +157,7 @@ class LBFactoryMulti extends LBFactory {
                $this->loadMonitorClass = $conf['loadMonitorClass'] ?? LoadMonitor::class;
        }
 
-       public function newMainLB( $domain = false ) {
+       public function newMainLB( $domain = false, $owner = null ) {
                $section = $this->getSectionForDomain( $domain );
                if ( !isset( $this->groupLoadsBySection[$section][ILoadBalancer::GROUP_GENERIC] ) ) {
                        throw new UnexpectedValueException( "Section '$section' has no hosts defined." );
@@ -175,7 +175,8 @@ class LBFactoryMulti extends LBFactory {
                        // Use the LB-specific read-only reason if everything isn't already read-only
                        is_string( $this->readOnlyReason )
                                ? $this->readOnlyReason
-                               : ( $this->readOnlyBySection[$section] ?? false )
+                               : ( $this->readOnlyBySection[$section] ?? false ),
+                       $owner
                );
        }
 
@@ -183,13 +184,13 @@ class LBFactoryMulti extends LBFactory {
                $section = $this->getSectionForDomain( $domain );
 
                if ( !isset( $this->mainLBs[$section] ) ) {
-                       $this->mainLBs[$section] = $this->newMainLB( $domain );
+                       $this->mainLBs[$section] = $this->newMainLB( $domain, $this->getOwnershipId() );
                }
 
                return $this->mainLBs[$section];
        }
 
-       public function newExternalLB( $cluster ) {
+       public function newExternalLB( $cluster, $owner = null ) {
                if ( !isset( $this->externalLoads[$cluster] ) ) {
                        throw new InvalidArgumentException( "Unknown cluster '$cluster'" );
                }
@@ -201,13 +202,15 @@ class LBFactoryMulti extends LBFactory {
                                $this->templateOverridesByCluster[$cluster] ?? []
                        ),
                        [ ILoadBalancer::GROUP_GENERIC => $this->externalLoads[$cluster] ],
-                       $this->readOnlyReason
+                       $this->readOnlyReason,
+                       $owner
                );
        }
 
        public function getExternalLB( $cluster ) {
                if ( !isset( $this->externalLBs[$cluster] ) ) {
-                       $this->externalLBs[$cluster] = $this->newExternalLB( $cluster );
+                       $this->externalLBs[$cluster] =
+                               $this->newExternalLB( $cluster, $this->getOwnershipId() );
                }
 
                return $this->externalLBs[$cluster];
@@ -265,11 +268,12 @@ class LBFactoryMulti extends LBFactory {
         * @param array $serverTemplate Server config map
         * @param int[][] $groupLoads Map of (group => host => load)
         * @param string|bool $readOnlyReason
+        * @param int|null $owner
         * @return LoadBalancer
         */
-       private function newLoadBalancer( $serverTemplate, $groupLoads, $readOnlyReason ) {
+       private function newLoadBalancer( $serverTemplate, $groupLoads, $readOnlyReason, $owner ) {
                $lb = new LoadBalancer( array_merge(
-                       $this->baseLoadBalancerParams(),
+                       $this->baseLoadBalancerParams( $owner ),
                        [
                                'servers' => $this->makeServerArray( $serverTemplate, $groupLoads ),
                                'loadMonitor' => [ 'class' => $this->loadMonitorClass ],
index 7e73e5b..cc79f99 100644 (file)
@@ -75,29 +75,29 @@ class LBFactorySimple extends LBFactory {
                $this->loadMonitorClass = $conf['loadMonitorClass'] ?? LoadMonitor::class;
        }
 
-       public function newMainLB( $domain = false ) {
-               return $this->newLoadBalancer( $this->mainServers );
+       public function newMainLB( $domain = false, $owner = null ) {
+               return $this->newLoadBalancer( $this->mainServers, $owner );
        }
 
        public function getMainLB( $domain = false ) {
                if ( $this->mainLB === null ) {
-                       $this->mainLB = $this->newMainLB( $domain );
+                       $this->mainLB = $this->newMainLB( $domain, $this->getOwnershipId() );
                }
 
                return $this->mainLB;
        }
 
-       public function newExternalLB( $cluster ) {
+       public function newExternalLB( $cluster, $owner = null ) {
                if ( !isset( $this->externalServersByCluster[$cluster] ) ) {
                        throw new InvalidArgumentException( "Unknown cluster '$cluster'." );
                }
 
-               return $this->newLoadBalancer( $this->externalServersByCluster[$cluster] );
+               return $this->newLoadBalancer( $this->externalServersByCluster[$cluster], $owner );
        }
 
        public function getExternalLB( $cluster ) {
                if ( !isset( $this->externalLBs[$cluster] ) ) {
-                       $this->externalLBs[$cluster] = $this->newExternalLB( $cluster );
+                       $this->externalLBs[$cluster] = $this->newExternalLB( $cluster, $this->getOwnershipId() );
                }
 
                return $this->externalLBs[$cluster];
@@ -116,9 +116,9 @@ class LBFactorySimple extends LBFactory {
                return $lbs;
        }
 
-       private function newLoadBalancer( array $servers ) {
+       private function newLoadBalancer( array $servers, $owner ) {
                $lb = new LoadBalancer( array_merge(
-                       $this->baseLoadBalancerParams(),
+                       $this->baseLoadBalancerParams( $owner ),
                        [
                                'servers' => $servers,
                                'loadMonitor' => [ 'class' => $this->loadMonitorClass ],
index 60044ba..97daf10 100644 (file)
@@ -44,8 +44,12 @@ class LBFactorySingle extends LBFactory {
                        throw new InvalidArgumentException( "Missing 'connection' argument." );
                }
 
-               $lb = new LoadBalancerSingle( array_merge( $this->baseLoadBalancerParams(), $conf ) );
+               $lb = new LoadBalancerSingle( array_merge(
+                       $this->baseLoadBalancerParams( $this->getOwnershipId() ),
+                       $conf
+               ) );
                $this->initLoadBalancer( $lb );
+
                $this->lb = $lb;
        }
 
@@ -63,23 +67,15 @@ class LBFactorySingle extends LBFactory {
                ) );
        }
 
-       /**
-        * @param bool|string $domain (unused)
-        * @return LoadBalancerSingle
-        */
-       public function newMainLB( $domain = false ) {
-               return $this->lb;
+       public function newMainLB( $domain = false, $owner = null ) {
+               throw new BadMethodCallException( "Method is not supported." );
        }
 
-       /**
-        * @param bool|string $domain (unused)
-        * @return LoadBalancerSingle
-        */
        public function getMainLB( $domain = false ) {
                return $this->lb;
        }
 
-       public function newExternalLB( $cluster ) {
+       public function newExternalLB( $cluster, $owner = null ) {
                throw new BadMethodCallException( "Method is not supported." );
        }
 
@@ -87,24 +83,14 @@ class LBFactorySingle extends LBFactory {
                throw new BadMethodCallException( "Method is not supported." );
        }
 
-       /**
-        * @return LoadBalancerSingle[] Map of (cluster name => LoadBalancer)
-        */
        public function getAllMainLBs() {
                return [ 'DEFAULT' => $this->lb ];
        }
 
-       /**
-        * @return LoadBalancerSingle[] Map of (cluster name => LoadBalancer)
-        */
        public function getAllExternalLBs() {
                return [];
        }
 
-       /**
-        * @param string|callable $callback
-        * @param array $params
-        */
        public function forEachLB( $callback, array $params = [] ) {
                if ( isset( $this->lb ) ) { // may not be set during _destruct()
                        $callback( $this->lb, ...$params );
index 160d501..4ca4250 100644 (file)
@@ -493,15 +493,22 @@ interface ILoadBalancer {
        public function getReplicaResumePos();
 
        /**
-        * Disable this load balancer. All connections are closed, and any attempt to
-        * open a new connection will result in a DBAccessError.
+        * Close all connections and disable this load balancer
+        *
+        * Any attempt to open a new connection will result in a DBAccessError.
+        *
+        * @param string $fname Caller name
+        * @param int|null $owner ID of the calling instance (e.g. the LBFactory ID)
         */
-       public function disable();
+       public function disable( $fname = __METHOD__, $owner = null );
 
        /**
         * Close all open connections
+        *
+        * @param string $fname Caller name
+        * @param int|null $owner ID of the calling instance (e.g. the LBFactory ID)
         */
-       public function closeAll();
+       public function closeAll( $fname = __METHOD__, $owner = null );
 
        /**
         * Close a connection
@@ -598,8 +605,9 @@ interface ILoadBalancer {
         * Commit all replica DB transactions so as to flush any REPEATABLE-READ or SSI snapshots
         *
         * @param string $fname Caller name
+        * @param int|null $owner ID of the calling instance (e.g. the LBFactory ID)
         */
-       public function flushReplicaSnapshots( $fname = __METHOD__ );
+       public function flushReplicaSnapshots( $fname = __METHOD__, $owner = null );
 
        /**
         * Commit all master DB transactions so as to flush any REPEATABLE-READ or SSI snapshots
@@ -607,8 +615,9 @@ interface ILoadBalancer {
         * An error will be thrown if a connection has pending writes or callbacks
         *
         * @param string $fname Caller name
+        * @param int|null $owner ID of the calling instance (e.g. the LBFactory ID)
         */
-       public function flushMasterSnapshots( $fname = __METHOD__ );
+       public function flushMasterSnapshots( $fname = __METHOD__, $owner = null );
 
        /**
         * @return bool Whether a master connection is already open
index f60e8db..39d1bd6 100644 (file)
@@ -106,6 +106,10 @@ class LoadBalancer implements ILoadBalancer {
        /** @var bool[] Map of (domain => whether to use "temp tables only" mode) */
        private $tempTablesOnlyMode = [];
 
+       /** @var string|bool Explicit DBO_TRX transaction round active or false if none */
+       private $trxRoundId = false;
+       /** @var string Stage of the current transaction round in the transaction round life-cycle */
+       private $trxRoundStage = self::ROUND_CURSORY;
        /** @var Database Connection handle that caused a problem */
        private $errorConnection;
        /** @var int[] The group replica server indexes keyed by group */
@@ -125,12 +129,10 @@ class LoadBalancer implements ILoadBalancer {
        /** @var bool Whether any connection has been attempted yet */
        private $connectionAttempted = false;
 
+       /** var int An identifier for this class instance */
+       private $id;
        /** @var int|null Integer ID of the managing LBFactory instance or null if none */
        private $ownerId;
-       /** @var string|bool Explicit DBO_TRX transaction round active or false if none */
-       private $trxRoundId = false;
-       /** @var string Stage of the current transaction round in the transaction round life-cycle */
-       private $trxRoundStage = self::ROUND_CURSORY;
 
        /** @var int Warn when this many connection are held */
        const CONN_HELD_WARN_THRESHOLD = 10;
@@ -218,7 +220,6 @@ class LoadBalancer implements ILoadBalancer {
                $this->deprecationLogger = $params['deprecationLogger'] ?? function ( $msg ) {
                        trigger_error( $msg, E_USER_DEPRECATED );
                };
-
                foreach ( [ 'replLogger', 'connLogger', 'queryLogger', 'perfLogger' ] as $key ) {
                        $this->$key = $params[$key] ?? new NullLogger();
                }
@@ -242,6 +243,8 @@ class LoadBalancer implements ILoadBalancer {
                $group = $params['defaultGroup'] ?? self::GROUP_GENERIC;
                $this->defaultGroup = isset( $this->groupLoads[$group] ) ? $group : self::GROUP_GENERIC;
 
+               static $nextId;
+               $this->id = $nextId = ( is_int( $nextId ) ? $nextId++ : mt_rand() );
                $this->ownerId = $params['ownerId'] ?? null;
        }
 
@@ -1301,6 +1304,7 @@ class LoadBalancer implements ILoadBalancer {
                // Use DBO_DEFAULT flags by default for LoadBalancer managed databases. Assume that the
                // application calls LoadBalancer::commitMasterChanges() before the PHP script completes.
                $server['flags'] = $server['flags'] ?? IDatabase::DBO_DEFAULT;
+               $server['ownerId'] = $this->id;
 
                // Create a live connection object
                $conn = Database::factory( $server['type'], $server, Database::NEW_UNCONNECTED );
@@ -1498,23 +1502,22 @@ class LoadBalancer implements ILoadBalancer {
                return $highestPos;
        }
 
-       public function disable() {
-               $this->closeAll();
+       public function disable( $fname = __METHOD__, $owner = null ) {
+               $this->assertOwnership( $fname, $owner );
+               $this->closeAll( $fname, $owner );
                $this->disabled = true;
        }
 
-       public function closeAll() {
+       public function closeAll( $fname = __METHOD__, $owner = null ) {
+               $this->assertOwnership( $fname, $owner );
                if ( $this->ownerId === null ) {
                        /** @noinspection PhpUnusedLocalVariableInspection */
                        $scope = ScopedCallback::newScopedIgnoreUserAbort();
                }
-
-               $fname = __METHOD__;
                $this->forEachOpenConnection( function ( IDatabase $conn ) use ( $fname ) {
                        $host = $conn->getServer();
-                       $this->connLogger->debug(
-                               $fname . ": closing connection to database '$host'." );
-                       $conn->close();
+                       $this->connLogger->debug( "$fname: closing connection to database '$host'." );
+                       $conn->close( $fname, $this->id );
                } );
 
                $this->conns = self::newTrackedConnectionsArray();
@@ -1543,13 +1546,13 @@ class LoadBalancer implements ILoadBalancer {
                        }
                }
 
-               $conn->close();
+               $conn->close( __METHOD__ );
        }
 
        public function commitAll( $fname = __METHOD__, $owner = null ) {
                $this->commitMasterChanges( $fname, $owner );
-               $this->flushMasterSnapshots( $fname );
-               $this->flushReplicaSnapshots( $fname );
+               $this->flushMasterSnapshots( $fname, $owner );
+               $this->flushReplicaSnapshots( $fname, $owner );
        }
 
        public function finalizeMasterChanges( $fname = __METHOD__, $owner = null ) {
@@ -1634,7 +1637,7 @@ class LoadBalancer implements ILoadBalancer {
                }
 
                // Clear any empty transactions (no writes/callbacks) from the implicit round
-               $this->flushMasterSnapshots( $fname );
+               $this->flushMasterSnapshots( $fname, $owner );
 
                $this->trxRoundId = $fname;
                $this->trxRoundStage = self::ROUND_ERROR; // "failed" until proven otherwise
@@ -1738,7 +1741,7 @@ class LoadBalancer implements ILoadBalancer {
                                        $this->queryLogger->warning( $fname . ": found writes pending." );
                                        $fnames = implode( ', ', $conn->pendingWriteAndCallbackCallers() );
                                        $this->queryLogger->warning(
-                                               $fname . ": found writes pending ($fnames).",
+                                               "$fname: found writes pending ($fnames).",
                                                [
                                                        'db_server' => $conn->getServer(),
                                                        'db_name' => $conn->getDBname()
@@ -1747,7 +1750,7 @@ class LoadBalancer implements ILoadBalancer {
                                } elseif ( $conn->trxLevel() ) {
                                        // A callback from another handle read from this one and DBO_TRX is set,
                                        // which can easily happen if there is only one DB (no replicas)
-                                       $this->queryLogger->debug( $fname . ": found empty transaction." );
+                                       $this->queryLogger->debug( "$fname: found empty transaction." );
                                }
                                try {
                                        $conn->commit( $fname, $conn::FLUSHING_ALL_PEERS );
@@ -1838,15 +1841,22 @@ class LoadBalancer implements ILoadBalancer {
        }
 
        /**
+        * Assure that if this instance is owned, the caller is either the owner or is internal
+        *
+        * If an LBFactory owns the LoadBalancer, then certain methods should only called through
+        * that LBFactory to avoid broken contracts. Otherwise, those methods can publically be
+        * called by anything. In any case, internal methods from the LoadBalancer itself should
+        * always be allowed.
+        *
         * @param string $fname
         * @param int|null $owner Owner ID of the caller
         * @throws DBTransactionError
         */
        private function assertOwnership( $fname, $owner ) {
-               if ( $this->ownerId !== null && $owner !== $this->ownerId ) {
+               if ( $this->ownerId !== null && $owner !== $this->ownerId && $owner !== $this->id ) {
                        throw new DBTransactionError(
                                null,
-                               "$fname: LoadBalancer is owned by LBFactory #{$this->ownerId} (got '$owner')."
+                               "$fname: LoadBalancer is owned by ID '{$this->ownerId}' (got '$owner')."
                        );
                }
        }
@@ -1893,13 +1903,15 @@ class LoadBalancer implements ILoadBalancer {
                }
        }
 
-       public function flushReplicaSnapshots( $fname = __METHOD__ ) {
+       public function flushReplicaSnapshots( $fname = __METHOD__, $owner = null ) {
+               $this->assertOwnership( $fname, $owner );
                $this->forEachOpenReplicaConnection( function ( IDatabase $conn ) use ( $fname ) {
                        $conn->flushSnapshot( $fname );
                } );
        }
 
-       public function flushMasterSnapshots( $fname = __METHOD__ ) {
+       public function flushMasterSnapshots( $fname = __METHOD__, $owner = null ) {
+               $this->assertOwnership( $fname, $owner );
                $this->forEachOpenMasterConnection( function ( IDatabase $conn ) use ( $fname ) {
                        $conn->flushSnapshot( $fname );
                } );
@@ -2317,7 +2329,7 @@ class LoadBalancer implements ILoadBalancer {
        }
 
        public function redefineLocalDomain( $domain ) {
-               $this->closeAll();
+               $this->closeAll( __METHOD__, $this->id );
 
                $this->setLocalDomain( DatabaseDomain::newFromId( $domain ) );
        }
@@ -2379,7 +2391,7 @@ class LoadBalancer implements ILoadBalancer {
 
        function __destruct() {
                // Avoid connection leaks for sanity
-               $this->disable();
+               $this->disable( __METHOD__, $this->ownerId );
        }
 }
 
index 1016f28..c789e83 100644 (file)
@@ -32,6 +32,7 @@ use Wikimedia\Rdbms\LoadBalancer;
 use Wikimedia\Rdbms\ChronologyProtector;
 use Wikimedia\Rdbms\MySQLMasterPos;
 use Wikimedia\Rdbms\DatabaseDomain;
+use Wikimedia\Rdbms\LoadMonitorNull;
 
 /**
  * @group Database
@@ -110,7 +111,6 @@ class LBFactoryTest extends MediaWikiTestCase {
                $this->assertSame( $factory->getLocalDomainID(), $factory->resolveDomainID( false ) );
 
                $factory->shutdown();
-               $lb->closeAll();
        }
 
        public function testLBFactorySimpleServers() {
@@ -160,7 +160,6 @@ class LBFactoryTest extends MediaWikiTestCase {
                        'cluster master set' );
 
                $factory->shutdown();
-               $lb->closeAll();
        }
 
        public function testLBFactoryMultiConns() {
index 58f6c05..4419533 100644 (file)
@@ -2239,7 +2239,7 @@ class DatabaseSQLTest extends PHPUnit\Framework\TestCase {
                } catch ( DBUnexpectedError $ex ) {
                        $this->assertSame(
                                'Wikimedia\Rdbms\Database::close: ' .
-                               'mass commit/rollback of peer transaction required (DBO_TRX set)',
+                               'expected mass rollback of all peer transactions (DBO_TRX set)',
                                $ex->getMessage()
                        );
                }