From: daniel <daniel.kinzler@wikimedia.de>
Date: Thu, 14 Apr 2016 19:02:31 +0000 (+0200)
Subject: Allow resources to be salvaged across service resets.
X-Git-Tag: 1.31.0-rc.0~6916^2
X-Git-Url: http://git.cyclocoop.org/%22%20.%20generer_url_ecrire%28%22sites_tous%22%29%20.%20%22?a=commitdiff_plain;h=bca436db920721302565801d47f370c17756de66;p=lhc%2Fweb%2Fwiklou.git

Allow resources to be salvaged across service resets.

NOTE: This also changes the semantics of MediaWikiServices::resetGlobalInstance
to only reset services instances, not service wiring. The wiring will be copied
from the old global MediaWikiServices instance to the new one.

Bug: T132707
Change-Id: Ie2ca3ff99aa74fffa9eb6c8faccab857dc0874f7
---

bca436db920721302565801d47f370c17756de66
diff --cc autoload.php
index 1e656e49fb,c7f89840cf..bdba1477b6
--- a/autoload.php
+++ b/autoload.php
@@@ -800,6 -799,6 +800,7 @@@ $wgAutoloadLocalClasses = 
  	'MediaWiki\\Services\\CannotReplaceActiveServiceException' => __DIR__ . '/includes/Services/CannotReplaceActiveServiceException.php',
  	'MediaWiki\\Services\\ContainerDisabledException' => __DIR__ . '/includes/Services/ContainerDisabledException.php',
  	'MediaWiki\\Services\\DestructibleService' => __DIR__ . '/includes/Services/DestructibleService.php',
++	'MediaWiki\\Services\\SalvageableService' => __DIR__ . '/includes/Services/SalvageableService.php',
  	'MediaWiki\\Services\\NoSuchServiceException' => __DIR__ . '/includes/Services/NoSuchServiceException.php',
  	'MediaWiki\\Services\\ServiceAlreadyDefinedException' => __DIR__ . '/includes/Services/ServiceAlreadyDefinedException.php',
  	'MediaWiki\\Services\\ServiceContainer' => __DIR__ . '/includes/Services/ServiceContainer.php',
diff --cc includes/MediaWikiServices.php
index 5bb5597b78,6c650aa641..3d4841903c
--- a/includes/MediaWikiServices.php
+++ b/includes/MediaWikiServices.php
@@@ -10,6 -9,6 +10,7 @@@ use Hooks
  use LBFactory;
  use Liuggio\StatsdClient\Factory\StatsdDataFactory;
  use LoadBalancer;
++use MediaWiki\Services\SalvageableService;
  use MediaWiki\Services\ServiceContainer;
  use MWException;
  use ResourceLoader;
@@@ -87,7 -82,7 +88,7 @@@ class MediaWikiServices extends Service
  			// even if it's just a file name or database credentials to load
  			// configuration from.
  			$bootstrapConfig = new GlobalVarConfig();
--			self::$instance = self::newInstance( $bootstrapConfig );
++			self::$instance = self::newInstance( $bootstrapConfig, 'load' );
  		}
  
  		return self::$instance;
@@@ -120,15 -113,13 +121,15 @@@
  	/**
  	 * Creates a new instance of MediaWikiServices and sets it as the global default
  	 * instance. getInstance() will return a different MediaWikiServices object
--	 * after every call to resetGlobalServiceLocator().
++	 * after every call to resetGlobalInstance().
 +	 *
 +	 * @since 1.28
  	 *
  	 * @warning This should not be used during normal operation. It is intended for use
  	 * when the configuration has changed significantly since bootstrap time, e.g.
  	 * during the installation process or during testing.
  	 *
--	 * @warning Calling resetGlobalServiceLocator() may leave the application in an inconsistent
++	 * @warning Calling resetGlobalInstance() may leave the application in an inconsistent
  	 * state. Calling this is only safe under the ASSUMPTION that NO REFERENCE to
  	 * any of the services managed by MediaWikiServices exist. If any service objects
  	 * managed by the old MediaWikiServices instance remain in use, they may INTERFERE
@@@ -149,11 -140,11 +150,14 @@@
  	 *        was no previous instance, a new GlobalVarConfig object will be used to
  	 *        bootstrap the services.
  	 *
++	 * @param string $quick Set this to "quick" to allow expensive resources to be re-used.
++	 * See SalvageableService for details.
++	 *
  	 * @throws MWException If called after MW_SERVICE_BOOTSTRAP_COMPLETE has been defined in
  	 *         Setup.php (unless MW_PHPUNIT_TEST or MEDIAWIKI_INSTALL or RUN_MAINTENANCE_IF_MAIN
  	 *          is defined).
  	 */
--	public static function resetGlobalInstance( Config $bootstrapConfig = null ) {
++	public static function resetGlobalInstance( Config $bootstrapConfig = null, $quick = '' ) {
  		if ( self::$instance === null ) {
  			// no global instance yet, nothing to reset
  			return;
@@@ -165,9 -156,9 +169,38 @@@
  			$bootstrapConfig = self::$instance->getBootstrapConfig();
  		}
  
--		self::$instance->destroy();
++		$oldInstance = self::$instance;
  
  		self::$instance = self::newInstance( $bootstrapConfig );
++		self::$instance->importWiring( $oldInstance, [ 'BootstrapConfig' ] );
++
++		if ( $quick === 'quick' ) {
++			self::$instance->salvage( $oldInstance );
++		} else {
++			$oldInstance->destroy();
++		}
++
++	}
++
++	/**
++	 * Salvages the state of any salvageable service instances in $other.
++	 *
++	 * @note $other will have been destroyed when salvage() returns.
++	 *
++	 * @param MediaWikiServices $other
++	 */
++	private function salvage( self $other ) {
++		foreach ( $this->getServiceNames() as $name ) {
++			$oldService = $other->peekService( $name );
++
++			if ( $oldService instanceof SalvageableService ) {
++				/** @var SalvageableService $newService */
++				$newService = $this->getService( $name );
++				$newService->salvage( $oldService );
++			}
++		}
++
++		$other->destroy();
  	}
  
  	/**
@@@ -176,21 -167,21 +209,23 @@@
  	 * ServiceWiringFiles setting are loaded, and the MediaWikiServices hook is called.
  	 *
  	 * @param Config|null $bootstrapConfig The Config object to be registered as the
--	 *        'BootstrapConfig' service. This has to contain at least the information
--	 *        needed to set up the 'ConfigFactory' service. If not provided, any call
--	 *        to getBootstrapConfig(), getConfigFactory, or getMainConfig will fail.
--	 *        A MediaWikiServices instance without access to configuration is called
--	 *        "primordial".
++	 *        'BootstrapConfig' service.
++	 *
++	 * @param string $loadWiring set this to 'load' to load the wiring files specified
++	 *        in the 'ServiceWiringFiles' setting in $bootstrapConfig.
  	 *
  	 * @return MediaWikiServices
  	 * @throws MWException
++	 * @throws \FatalError
  	 */
--	private static function newInstance( Config $bootstrapConfig ) {
++	private static function newInstance( Config $bootstrapConfig, $loadWiring = '' ) {
  		$instance = new self( $bootstrapConfig );
  
  		// Load the default wiring from the specified files.
--		$wiringFiles = $bootstrapConfig->get( 'ServiceWiringFiles' );
--		$instance->loadWiringFiles( $wiringFiles );
++		if ( $loadWiring === 'load' ) {
++			$wiringFiles = $bootstrapConfig->get( 'ServiceWiringFiles' );
++			$instance->loadWiringFiles( $wiringFiles );
++		}
  
  		// Provide a traditional hook point to allow extensions to configure services.
  		Hooks::run( 'MediaWikiServices', [ $instance ] );
diff --cc includes/Services/SalvageableService.php
index 0000000000,0000000000..a613050df1
new file mode 100644
--- /dev/null
+++ b/includes/Services/SalvageableService.php
@@@ -1,0 -1,0 +1,58 @@@
++<?php
++namespace MediaWiki\Services;
++
++/**
++ * Interface for salvageable services.
++ *
++ * This program is free software; you can redistribute it and/or modify
++ * it under the terms of the GNU General Public License as published by
++ * the Free Software Foundation; either version 2 of the License, or
++ * (at your option) any later version.
++ *
++ * This program is distributed in the hope that it will be useful,
++ * but WITHOUT ANY WARRANTY; without even the implied warranty of
++ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
++ * GNU General Public License for more details.
++ *
++ * You should have received a copy of the GNU General Public License along
++ * with this program; if not, write to the Free Software Foundation, Inc.,
++ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
++ * http://www.gnu.org/copyleft/gpl.html
++ *
++ * @file
++ *
++ * @since 1.28
++ */
++
++/**
++ * SalvageableService defines an interface for services that are able to salvage state from a
++ * previous instance of the same class. The intent is to allow new service instances to re-use
++ * resources that would be expensive to re-create, such as cached data or network connections.
++ *
++ * @note There is no expectation that services will be destroyed when the process (or web request)
++ * terminates.
++ */
++interface SalvageableService {
++
++	/**
++	 * Re-uses state from $other. $other must not be used after being passed to salvage(),
++	 * and should be considered to be destroyed.
++	 *
++	 * @note Implementations are responsible for determining what parts of $other can be re-used
++	 * safely. In particular, implementations should check that the relevant configuration of
++	 * $other is the same as in $this before re-using resources from $other.
++	 *
++	 * @note Implementations must take care to detach any re-used resources from the original
++	 * service instance. If $other is destroyed later, resources that are now used by the
++	 * new service instance must not be affected.
++	 *
++	 * @note If $other is a DestructibleService, implementations should make sure that $other
++	 * is in destroyed state after salvage finished. This may be done by calling $other->destroy()
++	 * after carefully detaching all relevant resources.
++	 *
++	 * @param SalvageableService $other The object to salvage state from. $other must have the
++	 * exact same type as $this.
++	 */
++	public function salvage( SalvageableService $other );
++
++}
diff --cc includes/Services/ServiceContainer.php
index 66ee918491,66ee918491..b336795ee4
--- a/includes/Services/ServiceContainer.php
+++ b/includes/Services/ServiceContainer.php
@@@ -55,6 -55,6 +55,11 @@@ class ServiceContainer implements Destr
  	 */
  	private $serviceInstantiators = [];
  
++	/**
++	 * @var boolean[] disabled status, per service name
++	 */
++	private $disabled = [];
++
  	/**
  	 * @var array
  	 */
@@@ -126,6 -126,6 +131,28 @@@
  		}
  	}
  
++	/**
++	 * Imports all wiring defined in $container. Wiring defined in $container
++	 * will override any wiring already defined locally. However, already
++	 * existing service instances will be preserved.
++	 *
++	 * @since 1.28
++	 *
++	 * @param ServiceContainer $container
++	 * @param string[] $skip A list of service names to skip during import
++	 */
++	public function importWiring( ServiceContainer $container, $skip = [] ) {
++		$newInstantiators = array_diff_key(
++			$container->serviceInstantiators,
++			array_flip( $skip )
++		);
++
++		$this->serviceInstantiators = array_merge(
++			$this->serviceInstantiators,
++			$newInstantiators
++		);
++	}
++
  	/**
  	 * Returns true if a service is defined for $name, that is, if a call to getService( $name )
  	 * would return a service instance.
@@@ -220,6 -220,6 +247,7 @@@
  		}
  
  		$this->serviceInstantiators[$name] = $instantiator;
++		unset( $this->disabled[$name] );
  	}
  
  	/**
@@@ -244,9 -244,9 +272,7 @@@
  	public function disableService( $name ) {
  		$this->resetService( $name );
  
--		$this->redefineService( $name, function() use ( $name ) {
--			throw new ServiceDisabledException( $name );
--		} );
++		$this->disabled[$name] = true;
  	}
  
  	/**
@@@ -282,6 -282,6 +308,7 @@@
  		}
  
  		unset( $this->services[$name] );
++		unset( $this->disabled[$name] );
  	}
  
  	/**
@@@ -299,7 -299,7 +326,8 @@@
  	 * @param string $name The service name
  	 *
  	 * @throws NoSuchServiceException if $name is not a known service.
--	 * @throws ServiceDisabledException if this container has already been destroyed.
++	 * @throws ContainerDisabledException if this container has already been destroyed.
++	 * @throws ServiceDisabledException if the requested service has been disabled.
  	 *
  	 * @return object The service instance
  	 */
@@@ -308,6 -308,6 +336,10 @@@
  			throw new ContainerDisabledException();
  		}
  
++		if ( isset( $this->disabled[$name] ) ) {
++			throw new ServiceDisabledException( $name );
++		}
++
  		if ( !isset( $this->services[$name] ) ) {
  			$this->services[$name] = $this->createService( $name );
  		}
@@@ -327,6 -327,6 +359,7 @@@
  				$this->serviceInstantiators[$name],
  				array_merge( [ $this ], $this->extraInstantiationParams )
  			);
++			// NOTE: when adding more wiring logic here, make sure copyWiring() is kept in sync!
  		} else {
  			throw new NoSuchServiceException( $name );
  		}
diff --cc includes/Setup.php
index 9db997adc5,9db997adc5..39ee4045f9
--- a/includes/Setup.php
+++ b/includes/Setup.php
@@@ -501,7 -501,7 +501,7 @@@ if ( !class_exists( 'AutoLoader' ) ) 
  
  // Reset the global service locator, so any services that have already been created will be
  // re-created while taking into account any custom settings and extensions.
--MediaWikiServices::resetGlobalInstance( new GlobalVarConfig() );
++MediaWikiServices::resetGlobalInstance( new GlobalVarConfig(), 'quick' );
  
  // Define a constant that indicates that the bootstrapping of the service locator
  // is complete.
diff --cc includes/config/ConfigFactory.php
index 09b0baa7be,09b0baa7be..cd25352dac
--- a/includes/config/ConfigFactory.php
+++ b/includes/config/ConfigFactory.php
@@@ -20,13 -20,13 +20,15 @@@
   *
   * @file
   */
++use MediaWiki\Services\SalvageableService;
++use Wikimedia\Assert\Assert;
  
  /**
   * Factory class to create Config objects
   *
   * @since 1.23
   */
--class ConfigFactory {
++class ConfigFactory implements SalvageableService {
  
  	/**
  	 * Map of config name => callback
@@@ -50,6 -50,6 +52,41 @@@
  		return \MediaWiki\MediaWikiServices::getInstance()->getConfigFactory();
  	}
  
++	/**
++	 * Re-uses existing Cache objects from $other. Cache objects are only re-used if the
++	 * registered factory function for both is the same. Cache config is not copied,
++	 * and only instances of caches defined on this instance with the same config
++	 * are copied.
++	 *
++	 * @see SalvageableService::salvage()
++	 *
++	 * @param SalvageableService $other The object to salvage state from. $other must have the
++	 * exact same type as $this.
++	 */
++	public function salvage( SalvageableService $other ) {
++		Assert::parameterType( self::class, $other, '$other' );
++
++		/** @var ConfigFactory $other */
++		foreach ( $other->factoryFunctions as $name => $otherFunc ) {
++			if ( !isset( $this->factoryFunctions[$name] ) ) {
++				continue;
++			}
++
++			// if the callback function is the same, salvage the Cache object
++			// XXX: Closures are never equal!
++			if ( isset( $other->configs[$name] )
++				&& $this->factoryFunctions[$name] == $otherFunc
++			) {
++				$this->configs[$name] = $other->configs[$name];
++				unset( $other->configs[$name] );
++			}
++		}
++
++		// disable $other
++		$other->factoryFunctions = [];
++		$other->configs = [];
++	}
++
  	/**
  	 * @return string[]
  	 */
@@@ -67,23 -67,23 +104,11 @@@
  	 * @throws InvalidArgumentException If an invalid callback is provided
  	 */
  	public function register( $name, $callback ) {
--		if ( $callback instanceof Config ) {
--			$instance = $callback;
--
--			// Register a callback anyway, for consistency. Note that getConfigNames()
--			// relies on $factoryFunctions to have all config names.
--			$callback = function() use ( $instance ) {
--				return $instance;
--			};
--		} else {
--			$instance = null;
--		}
--
--		if ( !is_callable( $callback ) ) {
++		if ( !is_callable( $callback ) && !( $callback instanceof Config ) ) {
  			throw new InvalidArgumentException( 'Invalid callback provided' );
  		}
  
--		$this->configs[$name] = $instance;
++		unset( $this->configs[$name] );
  		$this->factoryFunctions[$name] = $callback;
  	}
  
@@@ -105,7 -105,7 +130,13 @@@
  			if ( !isset( $this->factoryFunctions[$key] ) ) {
  				throw new ConfigException( "No registered builder available for $name." );
  			}
--			$conf = call_user_func( $this->factoryFunctions[$key], $this );
++
++			if ( $this->factoryFunctions[$key] instanceof Config ) {
++				$conf = $this->factoryFunctions[$key];
++			} else {
++				$conf = call_user_func( $this->factoryFunctions[$key], $this );
++			}
++
  			if ( $conf instanceof Config ) {
  				$this->configs[$name] = $conf;
  			} else {
diff --cc tests/phpunit/includes/MediaWikiServicesTest.php
index 467a2adff6,6c38d503f7..cbb6059957
--- a/tests/phpunit/includes/MediaWikiServicesTest.php
+++ b/tests/phpunit/includes/MediaWikiServicesTest.php
@@@ -1,6 -1,6 +1,8 @@@
  <?php
  use Liuggio\StatsdClient\Factory\StatsdDataFactory;
  use MediaWiki\MediaWikiServices;
++use MediaWiki\Services\DestructibleService;
++use MediaWiki\Services\SalvageableService;
  use MediaWiki\Services\ServiceDisabledException;
  
  /**
@@@ -8,7 -8,7 +10,7 @@@
   *
   * @group MediaWiki
   */
--class MediaWikiServicesTest extends PHPUnit_Framework_TestCase {
++class MediaWikiServicesTest extends MediaWikiTestCase {
  
  	/**
  	 * @return Config
@@@ -65,9 -65,9 +67,69 @@@
  		$newServices = $this->newMediaWikiServices();
  		$oldServices = MediaWikiServices::forceGlobalInstance( $newServices );
  
++		$service1 = $this->getMock( SalvageableService::class );
++		$service1->expects( $this->never() )
++			->method( 'salvage' );
++
++		$newServices->defineService(
++			'Test',
++			function() use ( $service1 ) {
++				return $service1;
++			}
++		);
++
++		// force instantiation
++		$newServices->getService( 'Test' );
++
  		MediaWikiServices::resetGlobalInstance( $this->newTestConfig() );
  		$theServices = MediaWikiServices::getInstance();
  
++		$this->assertSame(
++			$service1,
++			$theServices->getService( 'Test' ),
++			'service definition should survive reset'
++		);
++
++		$this->assertNotSame( $theServices, $newServices );
++		$this->assertNotSame( $theServices, $oldServices );
++
++		MediaWikiServices::forceGlobalInstance( $oldServices );
++	}
++
++	public function testResetGlobalInstance_quick() {
++		$newServices = $this->newMediaWikiServices();
++		$oldServices = MediaWikiServices::forceGlobalInstance( $newServices );
++
++		$service1 = $this->getMock( SalvageableService::class );
++		$service1->expects( $this->never() )
++			->method( 'salvage' );
++
++		$service2 = $this->getMock( SalvageableService::class );
++		$service2->expects( $this->once() )
++			->method( 'salvage' )
++			->with( $service1 );
++
++		// sequence of values the instantiator will return
++		$instantiatorReturnValues = [
++			$service1,
++			$service2,
++		];
++
++		$newServices->defineService(
++			'Test',
++			function() use ( &$instantiatorReturnValues ) {
++				return array_shift( $instantiatorReturnValues );
++			}
++		);
++
++		// force instantiation
++		$newServices->getService( 'Test' );
++
++		MediaWikiServices::resetGlobalInstance( $this->newTestConfig(), 'quick' );
++		$theServices = MediaWikiServices::getInstance();
++
++		$this->assertSame( $service2, $theServices->getService( 'Test' ) );
++
  		$this->assertNotSame( $theServices, $newServices );
  		$this->assertNotSame( $theServices, $oldServices );
  
@@@ -109,35 -109,35 +171,42 @@@
  		}
  
  		MediaWikiServices::forceGlobalInstance( $oldServices );
++		$newServices->destroy();
  	}
  
  	public function testResetChildProcessServices() {
  		$newServices = $this->newMediaWikiServices();
  		$oldServices = MediaWikiServices::forceGlobalInstance( $newServices );
  
--		$lbFactory = $this->getMockBuilder( 'LBFactorySimple' )
--			->disableOriginalConstructor()
--			->getMock();
++		$service1 = $this->getMock( DestructibleService::class );
++		$service1->expects( $this->once() )
++			->method( 'destroy' );
  
--		$lbFactory->expects( $this->once() )
++		$service2 = $this->getMock( DestructibleService::class );
++		$service2->expects( $this->never() )
  			->method( 'destroy' );
  
--		$newServices->redefineService(
--			'DBLoadBalancerFactory',
--			function() use ( $lbFactory ) {
--				return $lbFactory;
++		// sequence of values the instantiator will return
++		$instantiatorReturnValues = [
++			$service1,
++			$service2,
++		];
++
++		$newServices->defineService(
++			'Test',
++			function() use ( &$instantiatorReturnValues ) {
++				return array_shift( $instantiatorReturnValues );
  			}
  		);
  
  		// force the service to become active, so we can check that it does get destroyed
--		$oldLBFactory = $newServices->getService( 'DBLoadBalancerFactory' );
++		$oldTestService = $newServices->getService( 'Test' );
  
  		MediaWikiServices::resetChildProcessServices();
  		$finalServices = MediaWikiServices::getInstance();
  
--		$newLBFactory = $finalServices->getService( 'DBLoadBalancerFactory' );
--
--		$this->assertNotSame( $oldLBFactory, $newLBFactory );
++		$newTestService = $finalServices->getService( 'Test' );
++		$this->assertNotSame( $oldTestService, $newTestService );
  
  		MediaWikiServices::forceGlobalInstance( $oldServices );
  	}
diff --cc tests/phpunit/includes/Services/ServiceContainerTest.php
index 933777c33c,933777c33c..f22e123856
--- a/tests/phpunit/includes/Services/ServiceContainerTest.php
+++ b/tests/phpunit/includes/Services/ServiceContainerTest.php
@@@ -166,6 -166,6 +166,55 @@@ class ServiceContainerTest extends PHPU
  		$this->assertSame( 'Bar!', $services->getService( 'Bar' ) );
  	}
  
++	public function testImportWiring() {
++		$services = $this->newServiceContainer();
++
++		$wiring = [
++			'Foo' => function() {
++				return 'Foo!';
++			},
++			'Bar' => function() {
++				return 'Bar!';
++			},
++			'Car' => function() {
++				return 'FUBAR!';
++			},
++		];
++
++		$services->applyWiring( $wiring );
++
++		$newServices = $this->newServiceContainer();
++
++		// define a service before importing, so we can later check that
++		// existing service instances survive importWiring()
++		$newServices->defineService( 'Car', function() {
++			return 'Car!';
++		} );
++
++		// force instantiation
++		$newServices->getService( 'Car' );
++
++		// Define another service, so we can later check that extra wiring
++		// is not lost.
++		$newServices->defineService( 'Xar', function() {
++			return 'Xar!';
++		} );
++
++		// import wiring, but skip `Bar`
++		$newServices->importWiring( $services, [ 'Bar' ] );
++
++		$this->assertNotContains( 'Bar', $newServices->getServiceNames(), 'Skip `Bar` service' );
++		$this->assertSame( 'Foo!', $newServices->getService( 'Foo' ) );
++
++		// import all wiring, but preserve existing service instance
++		$newServices->importWiring( $services );
++
++		$this->assertContains( 'Bar', $newServices->getServiceNames(), 'Import all services' );
++		$this->assertSame( 'Bar!', $newServices->getService( 'Bar' ) );
++		$this->assertSame( 'Car!', $newServices->getService( 'Car' ), 'Use existing service instance' );
++		$this->assertSame( 'Xar!', $newServices->getService( 'Xar' ), 'Predefined services are kept' );
++	}
++
  	public function testLoadWiringFiles() {
  		$services = $this->newServiceContainer();
  
@@@ -220,6 -220,6 +269,27 @@@
  		$this->assertSame( $theService1, $services->getService( $name ) );
  	}
  
++	public function testRedefineService_disabled() {
++		$services = $this->newServiceContainer( [ 'Foo' ] );
++
++		$theService1 = new stdClass();
++		$name = 'TestService92834576';
++
++		$services->defineService( $name, function() {
++			return 'Foo';
++		} );
++
++		// disable the service. we should be able to redefine it anyway.
++		$services->disableService( $name );
++
++		$services->redefineService( $name, function() use ( $theService1 ) {
++			return $theService1;
++		} );
++
++		// force instantiation, check result
++		$this->assertSame( $theService1, $services->getService( $name ) );
++	}
++
  	public function testRedefineService_fail_undefined() {
  		$services = $this->newServiceContainer();
  
@@@ -294,13 -294,13 +364,6 @@@
  		$this->assertContains( 'Bar', $services->getServiceNames() );
  		$this->assertContains( 'Qux', $services->getServiceNames() );
  
--		// re-enable Bar service
--		$services->redefineService( 'Bar', function() {
--			return new stdClass();
--		} );
--
--		$services->getService( 'Bar' );
--
  		$this->setExpectedException( 'MediaWiki\Services\ServiceDisabledException' );
  		$services->getService( 'Qux' );
  	}
diff --cc tests/phpunit/includes/config/ConfigFactoryTest.php
index 2c1d1e6f0e,2c1d1e6f0e..f42cb95921
--- a/tests/phpunit/includes/config/ConfigFactoryTest.php
+++ b/tests/phpunit/includes/config/ConfigFactoryTest.php
@@@ -44,6 -44,6 +44,42 @@@ class ConfigFactoryTest extends MediaWi
  		$this->assertNotSame( $config1, $config2 );
  	}
  
++	/**
++	 * @covers ConfigFactory::register
++	 */
++	public function testSalvage() {
++		$oldFactory = new ConfigFactory();
++		$oldFactory->register( 'foo', 'GlobalVarConfig::newInstance' );
++		$oldFactory->register( 'bar', 'GlobalVarConfig::newInstance' );
++		$oldFactory->register( 'quux', 'GlobalVarConfig::newInstance' );
++
++		// instantiate two of the three defined configurations
++		$foo = $oldFactory->makeConfig( 'foo' );
++		$bar = $oldFactory->makeConfig( 'bar' );
++		$quux = $oldFactory->makeConfig( 'quux' );
++
++		// define new config instance
++		$newFactory = new ConfigFactory();
++		$newFactory->register( 'foo', 'GlobalVarConfig::newInstance' );
++		$newFactory->register( 'bar', function() {
++			return new HashConfig();
++		} );
++
++		// "foo" and "quux" are defined in the old and the new factory.
++		// The old factory has instances for "foo" and "bar", but not "quux".
++		$newFactory->salvage( $oldFactory );
++
++		$newFoo = $newFactory->makeConfig( 'foo' );
++		$this->assertSame( $foo, $newFoo, 'existing instance should be salvaged' );
++
++		$newBar = $newFactory->makeConfig( 'bar' );
++		$this->assertNotSame( $bar, $newBar, 'don\'t salvage if callbacks differ' );
++
++		// the new factory doesn't have quux defined, so the quux instance should not be salvaged
++		$this->setExpectedException( 'ConfigException' );
++		$newFactory->makeConfig( 'quux' );
++	}
++
  	/**
  	 * @covers ConfigFactory::register
  	 */