LockManagerGroupFactory to replace singletons

100% test coverage of code that appears to be working and used, in both
LockManagerGroupFactory and also LockManagerGroup. Where possible I
wrote it as unit tests. One preexisting code path seems to be broken and
I marked the test as skipped. Two methods look unused and perhaps not
especially helpful, so I didn't write tests for them yet in case we want
to just get rid of them instead.

Change-Id: Iaa7354f31c451b87773468609c674a3bf1d4382f

autoload.php

@@ -882,6 +882,7 @@ $wgAutoloadLocalClasses = [
 	'MediaWiki\\Diff\\ComplexityException' => __DIR__ . '/includes/diff/ComplexityException.php',
 	'MediaWiki\\Diff\\WordAccumulator' => __DIR__ . '/includes/diff/WordAccumulator.php',
 	'MediaWiki\\FileBackend\\FSFile\\TempFSFileFactory' => __DIR__ . '/includes/libs/filebackend/fsfile/TempFSFileFactory.php',
+	'MediaWiki\\FileBackend\\LockManager\\LockManagerGroupFactory' => __DIR__ . '/includes/filebackend/lockmanager/LockManagerGroupFactory.php',
 	'MediaWiki\\HeaderCallback' => __DIR__ . '/includes/HeaderCallback.php',
 	'MediaWiki\\Http\\HttpRequestFactory' => __DIR__ . '/includes/http/HttpRequestFactory.php',
 	'MediaWiki\\Installer\\InstallException' => __DIR__ . '/includes/installer/InstallException.php',

includes/ForkController.php

@@ -154,7 +154,6 @@ class ForkController {
 		// Don't share DB, storage, or memcached connections
 		MediaWikiServices::resetChildProcessServices();
 		FileBackendGroup::destroySingleton();
-		LockManagerGroup::destroySingletons();
 		JobQueueGroup::destroySingletons();
 		ObjectCache::clear();
 		RedisConnectionPool::destroySingletons();

includes/MediaWikiServices.php

@@ -17,6 +17,7 @@ use Liuggio\StatsdClient\Factory\StatsdDataFactoryInterface;
 use MediaWiki\Block\BlockManager;
 use MediaWiki\Block\BlockRestrictionStore;
 use MediaWiki\FileBackend\FSFile\TempFSFileFactory;
+use MediaWiki\FileBackend\LockManager\LockManagerGroupFactory;
 use MediaWiki\Http\HttpRequestFactory;
 use MediaWiki\Page\MovePageFactory;
 use MediaWiki\Permissions\PermissionManager;
@@ -658,6 +659,14 @@ class MediaWikiServices extends ServiceContainer {
 		return $this->getService( 'LocalServerObjectCache' );
 	}
 
+	/**
+	 * @since 1.34
+	 * @return LockManagerGroupFactory
+	 */
+	public function getLockManagerGroupFactory() : LockManagerGroupFactory {
+		return $this->getService( 'LockManagerGroupFactory' );
+	}
+
 	/**
 	 * @since 1.32
 	 * @return MagicWordFactory

includes/ServiceWiring.php

@@ -45,6 +45,7 @@ use MediaWiki\Block\BlockRestrictionStore;
 use MediaWiki\Config\ConfigRepository;
 use MediaWiki\Config\ServiceOptions;
 use MediaWiki\FileBackend\FSFile\TempFSFileFactory;
+use MediaWiki\FileBackend\LockManager\LockManagerGroupFactory;
 use MediaWiki\Http\HttpRequestFactory;
 use MediaWiki\Interwiki\ClassicInterwikiLookup;
 use MediaWiki\Interwiki\InterwikiLookup;
@@ -289,6 +290,14 @@ return [
 		return \ObjectCache::newFromParams( $config->get( 'ObjectCaches' )[$cacheId] );
 	},
 
+	'LockManagerGroupFactory' => function ( MediaWikiServices $services ) : LockManagerGroupFactory {
+		return new LockManagerGroupFactory(
+			WikiMap::getCurrentWikiDbDomain()->getId(),
+			$services->getMainConfig()->get( 'LockManagers' ),
+			$services->getDBLoadBalancerFactory()
+		);
+	},
+
 	'MagicWordFactory' => function ( MediaWikiServices $services ) : MagicWordFactory {
 		return new MagicWordFactory( $services->getContentLanguage() );
 	},

includes/filebackend/lockmanager/LockManagerGroup.php

@@ -22,6 +22,7 @@
  */
 use MediaWiki\MediaWikiServices;
 use MediaWiki\Logger\LoggerFactory;
+use Wikimedia\Rdbms\LBFactory;
 
 /**
  * Class to handle file lock manager registration
@@ -30,62 +31,27 @@ use MediaWiki\Logger\LoggerFactory;
  * @since 1.19
  */
 class LockManagerGroup {
-	/** @var LockManagerGroup[] (domain => LockManagerGroup) */
-	protected static $instances = [];
+	/** @var string domain (usually wiki ID) */
+	protected $domain;
 
-	protected $domain; // string; domain (usually wiki ID)
+	/** @var LBFactory */
+	protected $lbFactory;
 
 	/** @var array Array of (name => ('class' => ..., 'config' => ..., 'instance' => ...)) */
 	protected $managers = [];
 
 	/**
-	 * @param string $domain Domain (usually wiki ID)
-	 */
-	protected function __construct( $domain ) {
-		$this->domain = $domain;
-	}
-
-	/**
-	 * @param bool|string $domain Domain (usually wiki ID). Default: false.
-	 * @return LockManagerGroup
-	 */
-	public static function singleton( $domain = false ) {
-		if ( $domain === false ) {
-			$domain = WikiMap::getCurrentWikiDbDomain()->getId();
-		}
-
-		if ( !isset( self::$instances[$domain] ) ) {
-			self::$instances[$domain] = new self( $domain );
-			self::$instances[$domain]->initFromGlobals();
-		}
-
-		return self::$instances[$domain];
-	}
-
-	/**
-	 * Destroy the singleton instances
-	 */
-	public static function destroySingletons() {
-		self::$instances = [];
-	}
-
-	/**
-	 * Register lock managers from the global variables
-	 */
-	protected function initFromGlobals() {
-		global $wgLockManagers;
-
-		$this->register( $wgLockManagers );
-	}
-
-	/**
-	 * Register an array of file lock manager configurations
+	 * Do not call this directly. Use LockManagerGroupFactory.
 	 *
-	 * @param array $configs
-	 * @throws Exception
+	 * @param string $domain Domain (usually wiki ID)
+	 * @param array[] $lockManagerConfigs In format of $wgLockManagers
+	 * @param LBFactory $lbFactory
 	 */
-	protected function register( array $configs ) {
-		foreach ( $configs as $config ) {
+	public function __construct( $domain, array $lockManagerConfigs, LBFactory $lbFactory ) {
+		$this->domain = $domain;
+		$this->lbFactory = $lbFactory;
+
+		foreach ( $lockManagerConfigs as $config ) {
 			$config['domain'] = $this->domain;
 			if ( !isset( $config['name'] ) ) {
 				throw new Exception( "Cannot register a lock manager with no name." );
@@ -104,6 +70,26 @@ class LockManagerGroup {
 		}
 	}
 
+	/**
+	 * @deprecated since 1.34, use LockManagerGroupFactory
+	 *
+	 * @param bool|string $domain Domain (usually wiki ID). Default: false.
+	 * @return LockManagerGroup
+	 */
+	public static function singleton( $domain = false ) {
+		return MediaWikiServices::getInstance()->getLockManagerGroupFactory()
+			->getLockManagerGroup( $domain );
+	}
+
+	/**
+	 * Destroy the singleton instances
+	 *
+	 * @deprecated since 1.34, use resetServiceForTesting() on LockManagerGroupFactory
+	 */
+	public static function destroySingletons() {
+		MediaWikiServices::getInstance()->resetServiceForTesting( 'LockManagerGroupFactory' );
+	}
+
 	/**
 	 * Get the lock manager object with a given name
 	 *
@@ -120,8 +106,7 @@ class LockManagerGroup {
 			$class = $this->managers[$name]['class'];
 			$config = $this->managers[$name]['config'];
 			if ( $class === DBLockManager::class ) {
-				$lbFactory = MediaWikiServices::getInstance()->getDBLoadBalancerFactory();
-				$lb = $lbFactory->getMainLB( $config['domain'] );
+				$lb = $this->lbFactory->getMainLB( $config['domain'] );
 				$config['dbServers']['localDBMaster'] = $lb->getLazyConnectionRef(
 					DB_MASTER,
 					[],
@@ -132,6 +117,11 @@ class LockManagerGroup {
 			}
 			$config['logger'] = LoggerFactory::getInstance( 'LockManager' );
 
+			// XXX Looks like phan is right, we are trying to instantiate an abstract class and it
+			// throws. Did this ever work? Presumably we need to detect the right subclass? Or
+			// should we just get rid of this? It looks like it never worked since it was first
+			// introduced by 0cf832a3394 in 2016, so if no one's complained until now, clearly it
+			// can't be very useful?
 			// @phan-suppress-next-line PhanTypeInstantiateAbstract
 			$this->managers[$name]['instance'] = new $class( $config );
 		}
@@ -159,6 +149,8 @@ class LockManagerGroup {
 	 * Get the default lock manager configured for the site.
 	 * Returns NullLockManager if no lock manager could be found.
 	 *
+	 * XXX This looks unused, should we just get rid of it?
+	 *
 	 * @return LockManager
 	 */
 	public function getDefault() {
@@ -172,6 +164,8 @@ class LockManagerGroup {
 	 * or at least some other effective configured lock manager.
 	 * Throws an exception if no lock manager could be found.
 	 *
+	 * XXX This looks unused, should we just get rid of it?
+	 *
 	 * @return LockManager
 	 * @throws Exception
 	 */

includes/filebackend/lockmanager/LockManagerGroupFactory.php

@@ -0,0 +1,54 @@
+<?php
+
+namespace MediaWiki\FileBackend\LockManager;
+
+use LockManagerGroup;
+use Wikimedia\Rdbms\LBFactory;
+
+/**
+ * Service to construct LockManagerGroups.
+ */
+class LockManagerGroupFactory {
+	/** @var string */
+	private $defaultDomain;
+
+	/** @var array */
+	private $lockManagerConfigs;
+
+	/** @var LBFactory */
+	private $lbFactory;
+
+	/** @var LockManagerGroup[] (domain => LockManagerGroup) */
+	private $instances = [];
+
+	/**
+	 * Do not call directly, use MediaWikiServices.
+	 *
+	 * @param string $defaultDomain
+	 * @param array $lockManagerConfigs In format of $wgLockManagers
+	 * @param LBFactory $lbFactory
+	 */
+	public function __construct( $defaultDomain, array $lockManagerConfigs, LBFactory $lbFactory ) {
+		$this->defaultDomain = $defaultDomain;
+		$this->lockManagerConfigs = $lockManagerConfigs;
+		$this->lbFactory = $lbFactory;
+	}
+
+	/**
+	 * @param string|bool $domain Domain (usually wiki ID). false for the default (normally the
+	 *   current wiki's domain).
+	 * @return LockManagerGroup
+	 */
+	public function getLockManagerGroup( $domain = false ) : LockManagerGroup {
+		if ( $domain === false ) {
+			$domain = $this->defaultDomain;
+		}
+
+		if ( !isset( $this->instances[$domain] ) ) {
+			$this->instances[$domain] =
+				new LockManagerGroup( $domain, $this->lockManagerConfigs, $this->lbFactory );
+		}
+
+		return $this->instances[$domain];
+	}
+}

tests/parser/ParserTestRunner.php

@@ -313,7 +313,7 @@ class ParserTestRunner {
 			'class' => NullLockManager::class,
 		] ];
 		$reset = function () {
-			LockManagerGroup::destroySingletons();
+			MediaWikiServices::getInstance()->resetServiceForTesting( 'LockManagerGroupFactory' );
 		};
 		$setup[] = $reset;
 		$teardown[] = $reset;

tests/phpunit/unit/includes/filebackend/lockmanager/LockManagerGroupFactoryTest.php

@@ -0,0 +1,34 @@
+<?php
+
+use MediaWiki\FileBackend\LockManager\LockManagerGroupFactory;
+use Wikimedia\Rdbms\LBFactory;
+
+/**
+ * @covers MediaWiki\FileBackend\LockManager\LockManagerGroupFactory
+ * @todo Should we somehow test that the LockManagerGroup objects are as we expect? How do we do
+ *   that without getting into testing LockManagerGroup itself?
+ */
+class LockManagerGroupFactoryTest extends MediaWikiUnitTestCase {
+	public function testGetLockManagerGroup() {
+		$mockLbFactory = $this->createMock( LBFactory::class );
+		$mockLbFactory->expects( $this->never() )->method( $this->anything() );
+
+		$factory = new LockManagerGroupFactory( 'defaultDomain', [], $mockLbFactory );
+		$lbmUnspecified = $factory->getLockManagerGroup();
+		$lbmFalse = $factory->getLockManagerGroup( false );
+		$lbmDefault = $factory->getLockManagerGroup( 'defaultDomain' );
+		$lbmOther = $factory->getLockManagerGroup( 'otherDomain' );
+
+		$this->assertSame( $lbmUnspecified, $lbmFalse );
+		$this->assertSame( $lbmFalse, $lbmDefault );
+		$this->assertSame( $lbmDefault, $lbmUnspecified );
+		$this->assertNotEquals( $lbmUnspecified, $lbmOther );
+		$this->assertNotEquals( $lbmFalse, $lbmOther );
+		$this->assertNotEquals( $lbmDefault, $lbmOther );
+
+		$this->assertSame( $lbmUnspecified, $factory->getLockManagerGroup() );
+		$this->assertSame( $lbmFalse, $factory->getLockManagerGroup( false ) );
+		$this->assertSame( $lbmDefault, $factory->getLockManagerGroup( 'defaultDomain' ) );
+		$this->assertSame( $lbmOther, $factory->getLockManagerGroup( 'otherDomain' ) );
+	}
+}

tests/phpunit/unit/includes/filebackend/lockmanager/LockManagerGroupTest.php

@@ -0,0 +1,88 @@
+<?php
+
+use Wikimedia\Rdbms\LBFactory;
+use Wikimedia\TestingAccessWrapper;
+
+/**
+ * Since this is a unit test, we don't test the singleton() or destroySingletons() methods. We also
+ * can't test get() with a valid argument, because that winds up calling static methods of
+ * ObjectCache and LoggerFactory that aren't yet compatible with proper unit tests. Those will be
+ * tested in the integration test for now.
+ *
+ * @covers LockManagerGroup
+ */
+class LockManagerGroupTest extends MediaWikiUnitTestCase {
+	private function getMockLBFactory() {
+		$mock = $this->createMock( LBFactory::class );
+		$mock->expects( $this->never() )->method( $this->anythingBut( '__destruct' ) );
+		return $mock;
+	}
+
+	public function testConstructorNoConfigs() {
+		new LockManagerGroup( 'domain', [], $this->getMockLBFactory() );
+		$this->assertTrue( true, 'No exception thrown' );
+	}
+
+	public function testConstructorConfigWithNoName() {
+		$this->setExpectedException( Exception::class,
+			'Cannot register a lock manager with no name.' );
+
+		new LockManagerGroup( 'domain',
+			[ [ 'name' => 'a', 'class' => 'b' ], [ 'class' => 'c' ] ], $this->getMockLBFactory() );
+	}
+
+	public function testConstructorConfigWithNoClass() {
+		$this->setExpectedException( Exception::class,
+			'Cannot register lock manager `c` with no class.' );
+
+		new LockManagerGroup( 'domain',
+			[ [ 'name' => 'a', 'class' => 'b' ], [ 'name' => 'c' ] ], $this->getMockLBFactory() );
+	}
+
+	public function testGetUndefined() {
+		$this->setExpectedException( Exception::class,
+			'No lock manager defined with the name `c`.' );
+
+		$lmg = new LockManagerGroup( 'domain', [ [ 'name' => 'a', 'class' => 'b' ] ],
+			$this->getMockLBFactory() );
+		$lmg->get( 'c' );
+	}
+
+	public function testConfigUndefined() {
+		$this->setExpectedException( Exception::class,
+			'No lock manager defined with the name `c`.' );
+
+		$lmg = new LockManagerGroup( 'domain', [ [ 'name' => 'a', 'class' => 'b' ] ],
+			$this->getMockLBFactory() );
+		$lmg->config( 'c' );
+	}
+
+	public function testConfig() {
+		$lmg = new LockManagerGroup( 'domain', [ [ 'name' => 'a', 'class' => 'b', 'foo' => 'c' ] ],
+			$this->getMockLBFactory() );
+		$this->assertSame(
+			[ 'class' => 'b', 'name' => 'a', 'foo' => 'c', 'domain' => 'domain' ],
+			$lmg->config( 'a' )
+		);
+	}
+
+	public function testGetDefaultNull() {
+		$lmg = new LockManagerGroup( 'domain', [], $this->getMockLBFactory() );
+		$expected = new NullLockManager( [] );
+		$actual = $lmg->getDefault();
+		// Have to get rid of the $sessions for equality check to work
+		TestingAccessWrapper::newFromObject( $actual )->session = null;
+		TestingAccessWrapper::newFromObject( $expected )->session = null;
+		$this->assertEquals( $expected, $actual );
+	}
+
+	public function testGetAnyException() {
+		// XXX Isn't the name 'getAny' misleading if we don't get whatever's available?
+		$this->setExpectedException( Exception::class,
+			'No lock manager defined with the name `fsLockManager`.' );
+
+		$lmg = new LockManagerGroup( 'domain', [ [ 'name' => 'a', 'class' => 'b' ] ],
+			$this->getMockLBFactory() );
+		$lmg->getAny();
+	}
+}